8362564: hotspot/jtreg/compiler/c2/TestLWLockingCodeGen.java fails on static JDK on x86_64 with AVX instruction extensions #26395
Conversation
…)' for size estimate. - In 'initialize_stubs()', move the assert to after stubs log_info print.
|
👋 Welcome back jiangli! A progress list of the required criteria for merging this PR into |
|
@jianglizhou This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 104 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@jianglizhou The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
@adinn ^ |
|
|
||
| // When new stubs added we need to make sure there is some space left | ||
| // to catch situation when we should increase size again. | ||
| assert(code_size == 0 || buffer.insts_remaining() > 200, "increase %s", assert_msg); |
There was a problem hiding this comment.
What is the reason to move the assert?
There was a problem hiding this comment.
@eastig Thanks for taking a look!
The log Info, stubs output provides useful details. With the assertion failure, it wouldn't show. That's why moving the assert to after the log output.
There was a problem hiding this comment.
If you need information which is printed out in the log, why not to add needed information to the assert? You can copy the format string used for logging. This will make assert's output more informative without a need for enabling logging.
There was a problem hiding this comment.
Sounds reasonable to me. I updated the PR to move back the assert and added code_size, buffer total size and free size info to the assert output. Thanks
There was a problem hiding this comment.
I actually think the change is better with the assert after the logging, as initially proposed.
I don't object to the making the assert's output more informative, but can imagine that there may be future improvements to the log message which may then leave the assert with a different message.
The risk of switching the assertion and logging seems minimal.
There was a problem hiding this comment.
@rasbold Thanks for the thought! Right, there doesn't seem to be much risk with moving the assert to after the logging. With the additional info in the assert output as @eastig suggested, it does provide enough information without needing to enable -Xlog:stubs. In the case where we ran into the particular problem in non-local testing environment, the additional size information provided in the assert could save quite some debugging time. So the current change does seem to be a bit more advantageous.
- Reverted change for moving the assert. Added code_size, buffer size and free to the assert output.
|
/integrate |
|
Going to push as commit c239c0a.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@jianglizhou Pushed as commit c239c0a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Thanks. I also updated the PR description to reflect the change. |
|
There was no GHA testing for this. |
@vnkozlov Do you refer to the PR testing? It's https://github.com/openjdk/jdk/pull/26395/checks, thanks. For the specific bug manifesting with TestLWLockingCodeGen.java failure, currently GHA doesn't run tests on debug build for static jdk jobs. It also requires the testing x86_64 machine support AVX extensions to show the issue. That's the reason why GHA has not found the issue yet. |
Yes, that. I did not realize that GitHub removes it from main PR tab after integration. Good, it was tested. I think GitHub actions use AVX512 machines. But I remember that static build testing was excluded. First, I am fine with fix you pushed. But it is strange. There should be no difference in stubs sizes running with regular JDK on avx512 machine and static JDK. I don't see how we don't trigger this issue in our testing - we regularly running on AVX512 machines. Do you have additional code somewhere in macro assembler for static build? Is it possible to compare assembler code? |
|
Also about back porting into JDK 25. You need to rise priority - only P1 and P2 are allowed in RDP2. |
Please run both, regular and static JDK, on AVX512 machine. |
|
Thanks for clarification @vnkozlov.
I was wondering the same initially, for regular JDK vs static JDK. So I did some measurements and confirmed the difference. Here is the data from the tests that I ran on machines with AVX extensions. I haven't looked into details of the generated instructions to see which ones are related, though. static JDK regular JDK
There was not static specific in macro assembler. Let me try running the tests again and update ... |
|
@vnkozlov I collected some info with
On regular JDK, the target address can be encoded within the I recall @rasbold had suspected that could be what was causing the differences between static and regular JDKs, when we briefly discussed about the issue last week. Here are the disassembled static-jdk regular jdk |
Changed to P2. |
|
@jianglizhou So it is similar to issue in AOT where we force Which means normal JDK is not affected and that is why we did not see it in our testing. |
|
@jianglizhou thank you for showing code difference. I approved JDK 25 backport. |
Thanks, @vnkozlov! |
Yes. |
|
/backport jdk25 |
|
@jianglizhou The target repository There is a branch |
|
/backport :jdk25 |
|
@jianglizhou the backport was successfully created on the branch backport-jianglizhou-c239c0ab-jdk25 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk25, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk: |
I want to correct my statement. Normal JDK could be also affected as I pointed in JBS report in my approval comment. |
Please review this fix that increases the x86 initial stubs size to
21000forNOT_PRODUCTonly case.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26395/head:pull/26395$ git checkout pull/26395Update a local copy of the PR:
$ git checkout pull/26395$ git pull https://git.openjdk.org/jdk.git pull/26395/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26395View PR using the GUI difftool:
$ git pr show -t 26395Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26395.diff
Using Webrev
Link to Webrev Comment