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

Jump to bottom

8257162: Initialize ThreadLocalAllocBuffer members #1458

Closed
wants to merge 3 commits into from
Closed

8257162: Initialize ThreadLocalAllocBuffer members #1458

wants to merge 3 commits into from

Conversation

tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Nov 26, 2020 •
edited by openjdk[bot] bot

Hi all,

can I have reviews for this change that adds initialization for all ThreadLocalAllocBuffer members so that when inspecting threads' TLABs the values are not random too?

Testing: tier1,tier2

Thanks,
Thomas

Progress

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

Issue

  • JDK-8257162: Initialize ThreadLocalAllocBuffer members

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 26, 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.

@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Nov 26, 2020

/label add hotspot-gc

@openjdk openjdk bot added rfr hotspot-gc labels Nov 26, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020

@tschatzl
The hotspot-gc label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 26, 2020 •
edited

Webrevs

shipilev
shipilev approved these changes Nov 26, 2020
View changes
Copy link
Contributor

@shipilev shipilev left a comment

Looks good to me.

src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp Outdated
_allocated_size(0),
_allocation_fraction(TLABAllocationWeight) {

// do nothing. tlabs must be inited by initialize() calls
Copy link
Contributor

@shipilev shipilev Nov 26, 2020

Choose a reason for hiding this comment

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

Wording: Do nothing. TLABs must be initialized by initialize() calls.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020 •
edited

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

8257162: Initialize ThreadLocalAllocBuffer members

Reviewed-by: shade, ayang, sjohanss, pliden

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

  • 78fdb65: 8254893: Fix display of search tag results without holder information
  • 20525d2: 8257149: Improve G1 Service thread task scheduling to guarantee task delay
  • f2f3ba9: 8242652: Throw SkippedException if no JS engine availabe in TestSearchScript
  • ee99686: 8252645: Change time measurements in G1ServiceThread to only account remembered set work
  • a3eec39: 8257181: s390x builds are very noisy with gc-sections messages
  • 9a468d8: 8256757: Incorrect MachCallRuntimeNode::ret_addr_offset() for CallLeafNoFP on x86_32
  • 2215e5a: 8255351: Add detection for Graviton 2 CPUs
  • 62d72de: 8220730: sun.security.provider.SecureRandom default constructor has wrong documentation
  • 4e43b28: 8256359: AArch64: runtime/ReservedStack/ReservedStackTestCompiler.java fails
  • 6e00622: 8256488: [aarch64] Use ldpq/stpq instead of ld4/st4 for small copies in StubGenerator::copy_memory
  • ... and 3 more: https://git.openjdk.java.net/jdk/compare/bf66d734bc70d68200e8aaeb525c92c70b0b6baa...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 26, 2020
albertnetymk
albertnetymk approved these changes Nov 26, 2020
View changes
Copy link
Member

@albertnetymk albertnetymk left a comment

I believe these extra stores will not introduce much overhead.

kstefanj
kstefanj approved these changes Nov 27, 2020
View changes
Copy link
Contributor

@kstefanj kstefanj left a comment

Looks good, just this comment on the comment that you can skip if you don't agree.

src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp
_allocation_fraction(TLABAllocationWeight) {

// do nothing. TLABs must be inited by initialize() calls
}
Copy link
Contributor

@kstefanj kstefanj Nov 27, 2020

Choose a reason for hiding this comment

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

Any reason for the extra blank line here? I would also use a capital D in "Do nothing".

pliden
pliden approved these changes Nov 27, 2020
View changes
@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Nov 30, 2020

Thanks @pliden @kstefanj @albertnetymk @shipilev for your reviews

/integrate

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

@openjdk openjdk bot commented Nov 30, 2020

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

  • 337d7bc: 8257165: C2: Improve box elimination for vector masks and shuffles
  • 4e55d0f: 8257057: C2: Improve safepoint processing during vector scalarization pass
  • e77aed6: 8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant
  • 738efea: 8248564: JFR: Remote Recording Stream
  • 9bcd269: 8257221: C2: RegMask::is_bound_set split set handling broken since JDK-8221404
  • 222e943: 8257238: Cleanup include directives for precompiled.hpp
  • fdee70d: 8257237: Cleanup unused imports in the SunJSSE provider implementation
  • 816e8f8: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
  • c5d9507: 8257220: [JVMCI] option validation should not result in a heavy-weight VM crash
  • c2af27b: 8257148: Remove obsolete code in AWTView.m
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/bf66d734bc70d68200e8aaeb525c92c70b0b6baa...master

Your commit was automatically rebased without conflicts.

Pushed as commit 962f7a3.

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

@tschatzl tschatzl deleted the 8257162-Initialize-TLAB-members branch Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertnetymk albertnetymk

@shipilev shipilev

@pliden pliden

@kstefanj kstefanj

Assignees
No one assigned
Labels
hotspot-gc integrated
Milestone
No milestone
5 participants
@tschatzl @albertnetymk @shipilev @pliden @kstefanj