Correctly identify use of compressed references for CAS write barrier #2335
Conversation
The `usingCompressedReferences` variable is misleading as it does not imply compressed references are actually used. What this variable actually means is that the value we are storing into the object via compare and swap is a compressed reference. Recognized CAS sequences are reduced at lower trees phase [1] where the last two arguments being the expected value and the new value are lowered into compressed objects since under compressed references objects on heap are always compressed. For concurrent scavenge we need a read barrier on the expected value hence the read barrier under compressed references need not rely on the use of the `usingCompressedReferences` variable which means something totally different. We take this opportunity to first fix the bug and to improve the meaning of some of the variables in the modified function so it is not confusing for future readers. [1] https://github.com/eclipse/openj9/blob/8a36fcd5f23005bf5028d28b60fd61aa325940a1/runtime/compiler/codegen/J9CodeGenerator.cpp#L675-L683 Signed-off-by: Filip Jeremic <[email protected]>
|
Jenkins test sanity zlinux |
|
@joransiu I'm going to have to defer to your expertise for this review. |
I believe @joransiu is away until the end of July so a review may be pending for a while. Given the scope of this bug causing issues when concurrent scavenge is enabled I would like to see it fixed sooner rahter than later. I propose one of two things:
I'm ok with either as it will alleviate the problem immediately. Please advise. |
|
I'm ok merging the change as is, as the PR does make sense to me. However, I think it would be worth getting someone from the z codegen team to also take a look at this, maybe @r30shah . |
There was a problem hiding this comment.
Looks more cleaner to me. I agree with @fjeremic , usingCompressedPointers was a confusing name to use to check if value we are storing is compressed reference. Which I assume was leading to the bug where we decide wrong instruction for guarded load.
|
@fjeremic @amicic is on vacation for until the end of the month. I can review this for you if you like? Looking at the change I like the refactoring (for names, etc.). The actual fix is: which checks to see if compressed references is running and not whether or not this value is compressed correct? |
Thanks @charliegracie. Yes this is indeed the fix. Perhaps I should have broken it into two commits to be more clear. Apologies for that. I have manually tested the fix against the broken workload and it does fix the problem. I've also ran additional testing to the above Jenkins builds and there is no problem. |
|
@fjeremic no problem it was easy enough to figure out. Just wanted to make sure I was not missing something :). Merging as the only failure is a known issue. |
The
usingCompressedReferencesvariable is misleading as it does notimply compressed references are actually used. What this variable
actually means is that the value we are storing into the object via
compare and swap is a compressed reference.
Recognized CAS sequences are reduced at lower trees phase [1] where the
last two arguments being the expected value and the new value are
lowered into compressed objects since under compressed references
objects on heap are always compressed.
For concurrent scavenge we need a read barrier on the expected value
hence the read barrier under compressed references need not rely on the
use of the
usingCompressedReferencesvariable which means somethingtotally different.
We take this opportunity to first fix the bug and to improve the
meaning of some of the variables in the modified function so it is not
confusing for future readers.
[1] https://github.com/eclipse/openj9/blob/8a36fcd5f23005bf5028d28b60fd61aa325940a1/runtime/compiler/codegen/J9CodeGenerator.cpp#L675-L683
Signed-off-by: Filip Jeremic [email protected]