Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8256406: G1 x86 C1/Interpreter post write barrier always uses 32 bit to access variable sized PtrQueue::_index #1257

Closed
wants to merge 2 commits into from

Conversation

tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Nov 17, 2020

Hi all,

can I have reviews for this change that fixes benign issues with the G1 C1/interpreter barriers that on x64 only used the lower 32 bit word of the 64 bit PtrQueue::_index member?

Testing: tier1, linux-x86 builds

Thanks,
Thomas


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256406: G1 x86 C1/Interpreter post write barrier always uses 32 bit to access variable sized PtrQueue::_index

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1257/head:pull/1257
$ git checkout pull/1257

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 17, 2020

👋 Welcome back tschatzl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

@tschatzl The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Nov 17, 2020
@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Nov 17, 2020

/label remove hotspot
/label add hotspot-gc

@openjdk openjdk bot removed the hotspot label Nov 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

@tschatzl
The hotspot label was successfully removed.

@openjdk openjdk bot added the hotspot-gc label Nov 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2020

@tschatzl
The hotspot-gc label was successfully added.

@tschatzl tschatzl marked this pull request as ready for review Nov 17, 2020
@openjdk openjdk bot added the rfr label Nov 17, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 17, 2020

Webrevs

Copy link
Contributor

@shipilev shipilev left a comment

It looks basically fine, with a minor nit.

__ testptr(tmp2, tmp2);
__ jcc(Assembler::equal, runtime);
Copy link
Contributor

@shipilev shipilev Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is the same in x86-speak, but Assembler::zero would be more readable to show that we actually test queue_index == 0.

Copy link
Contributor Author

@tschatzl tschatzl Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean using __ cmpptr(tmp2, 0) or something similar? I would like to keep this that way as it's such a common and well-known idiom, even if it's only a nano-optimization.

Otherwise we should also change the pre-barrier code to be uniform.

Copy link
Contributor

@shipilev shipilev Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean testptr(tmp2, tmp2); jcc(Assembler::zero, runtime). In fact, I see the similar code on L489 in the same file.

@@ -178,7 +178,7 @@ class PtrQueue {
return byte_offset_of(Derived, _index);
}

static ByteSize byte_width_of_index() { return in_ByteSize(sizeof(size_t)); }
static constexpr ByteSize byte_width_of_index() { return in_ByteSize(sizeof(size_t)); }
Copy link
Contributor

@shipilev shipilev Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have to constexpr this for STATIC_ASSERT? No objection, just curious.

Copy link
Contributor Author

@tschatzl tschatzl Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My compiler (gcc 9.3.0) complains with

[...] g1BarrierSetAssembler_x86.cpp:269:60: error: call to non-'constexpr' function 'static ByteSize PtrQueue::byte_width_of_index()' [...]

otherwise. Probably because of the called in_ByteSize() method.

Copy link
Contributor

@shipilev shipilev Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Copy link
Contributor

@shipilev shipilev left a comment

This looks good to me, thanks.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 18, 2020

@tschatzl 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:

8256406: G1 x86 C1/Interpreter post write barrier always uses 32 bit to access variable sized PtrQueue::_index

Reviewed-by: shade

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 69 new commits pushed to the master branch:

  • cb2676c: 8256499: Zero: enable Epsilon GC
  • 8e241b5: 8256552: Let ReplayCompiles set UseDebuggerErgo
  • 4178834: 8256172: Clean up CDS handling of i2i_entry
  • cfa92a5: 8256178: Add RAII object for file lock
  • 2b15571: 8256383: PlatformMutex::try_lock has different semantics on Windows and Posix
  • 99eac53: 8225631: Consider replacing muxAcquire/Release with PlatformMonitor
  • 646c200: 8256152: tests fail because of ambiguous method resolution
  • 5912df2: 8256427: Test com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on AIX
  • 3110d58: 8256538: Fix annoying awk warning in configure for java versions
  • 03e84ef: 8256189: Exact VarHandle tests should test withInvokeBehavior() works as expected
  • ... and 59 more: https://git.openjdk.java.net/jdk/compare/1103e3374c585d4b5f6e8b3762658a392f673426...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 18, 2020
@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Nov 20, 2020

Thanks @shipilev for your review.

/integrate

@openjdk openjdk bot closed this Nov 20, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2020

@tschatzl Since your change was applied there have been 95 commits pushed to the master branch:

  • f576628: 8256633: Fix product build on Windows+Arm64
  • 8e7a855: 8255526: Enable jcheck whitespace checking of build system files
  • c45ab1a: 8256393: Github Actions build on Linux should define OS and GCC versions
  • 5fedb69: 8250888: nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java fails
  • 02adaa5: 8255885: Metaspace: freelist commit counter is not updated when purging
  • fa240f2: 8256594: Unexpected warning: SIGSEGV handler flags expected:SA_RESTART|SA_SIGINFO found:SA_RESTART|SA_SIGINFO
  • 4c09525: 8256108: Create implementation for NSAccessibilityElement protocol peer
  • 6813889: 8251317: Support for CLDR version 38
  • c816464: 4916923: In MetalRootPaneUI, MetalRootLayout does not correctly calculate minimumsize
  • fae68ff: 8256640: assert(!m->is_old() || ik()->is_being_redefined()) failed: old methods should not be in vtable
  • ... and 85 more: https://git.openjdk.java.net/jdk/compare/1103e3374c585d4b5f6e8b3762658a392f673426...master

Your commit was automatically rebased without conflicts.

Pushed as commit a25fb03.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk-notifier bot referenced this issue Nov 20, 2020
…to access variable sized PtrQueue::_index

Reviewed-by: shade
@tschatzl tschatzl deleted the 8256406-x86-c1-barrier-type branch Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
2 participants