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

JDK-8261644: NMT: Simplifications and cleanups #2539

Closed
wants to merge 4 commits into from

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Feb 12, 2021

Hi,

may I please have reviews for this RFE?

While working on NMT I found a number of possible cleanups and simplifications. I avoided mixing these cleanups with fixed and instead put them into this cleanup RFE.

There should be no behavioral changes in this patch.

  • de-templatize AllocationSite<E> since E was used as simple data holder for child classes; the same effect can be had with traditional inheritance with less and clearer code (also IDEs get less confused)

  • AllocationSite child classes SimpleThreadStackSite, VirtualMemoryAllocationSite, MallocSite were simplified.

  • As for SimpleThreadStackSite, we can get rid of the separate data holder class SimpleThreadStack entirely by merging its members directly into SimpleThreadStackSite. In theory we could do the same for the data holder classes MemoryCounter and VirtualMemory for MallocSite and VirtualMemoryAllocationSite too but this would cause larger ripples so I stopped there.

  • removed the SimpleThreadStackSite(address base, size_t size) constructor (the one not taking a call stack) by slightly rewriting its sole user

  • made AllocationSite immutable

  • removed unused default constructors from MallocSite and MallocSiteHashTableEntry since they were not needed

  • removed unused methods set_callsite(), hash(), equals() from MallocSiteHashTableEntry

  • There was a subtle incorrectness where AllocationSite::equals() would only compare callstack and disregard the MEMFLAGS member. Theoretically, if two callstacks end with the same lowest frame, they should always reference the same single allocation, so that's okay. But if the call stack capturing was not precise enough (eg skipping too many low frames) we may accidentally lump several allocation sites together which could have different MEMFLAGS. I added an assert to check that. (Update: seems this assert really fires on s390x, so this is a real problem. I opened [1] to track this and restored the old behavior.).

  • NativeCallStack: Removed the fillStack argument from the first constructor to avoid having to evaluate it in this hot constructor. Its true in almost all cases.

  • Also removed the toSkip default value. Instead, I added an explicit default constructor.

  • Moved the malloc site table tuning statistics printing from memtracker.cpp down into a new function MallocSiteTable::print_tuning_statistics(). When implemented inside MallocSiteTable, that coding does not need a walker object anymore and becomes a lot simpler. In particular, we don't have to rely on implicit knowledge about walking order, which made the code complex and was vulnerable against subtle errors. New code is more compact and simpler. Before removing the old implementation, I ran both statistics side by side for a couple of scenarios (eg really bad hash code implementations) and the output was identical.

[1] https://bugs.openjdk.java.net/browse/JDK-8261556


Tests:

  • github GA
  • manual NMT jtreg tests (including the currently disabled runtime/NMT/CheckForProperDetailStackTrace.java)
  • Full nightlies at SAP are scheduled

Progress

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

Issue

Reviewers

Download

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

@tstuefe tstuefe changed the title start nmt: code grooming Feb 12, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 12, 2021

👋 Welcome back stuefe! 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 bot commented Feb 12, 2021

@tstuefe 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 Feb 12, 2021
@tstuefe tstuefe changed the title nmt: code grooming JDK-8261644: NMT: Simplifications and cleanups Feb 12, 2021
@tstuefe tstuefe marked this pull request as ready for review Feb 12, 2021
@openjdk openjdk bot added the rfr label Feb 12, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 12, 2021

Webrevs

@dcubed-ojdk
Copy link
Member

dcubed-ojdk commented Feb 12, 2021

@tstuefe - Thanks for keeping cleanups separated from bug fixes. That always
makes the PR reviews easier. I don't see any information about what testing was
done on this PR.

@tstuefe
Copy link
Member Author

tstuefe commented Feb 12, 2021

@tstuefe - Thanks for keeping cleanups separated from bug fixes. That always
makes the PR reviews easier. I don't see any information about what testing was
done on this PR.

Thanks Dan. I updated the description. I'm very careful after JDK-8261520. Manual tests went through, nightlies at SAP are scheduled. BTW this patch is a preparation for the fix for JDK-8261520.

Copy link
Contributor

@coleenp coleenp left a comment

These changes look great. Thank you!

@@ -138,7 +138,7 @@ MallocSite* MallocSiteTable::lookup_or_add(const NativeCallStack& key, size_t* b
MallocSiteHashtableEntry* head = _table[index];
while (head != NULL && (*pos_idx) <= MAX_BUCKET_LENGTH) {
MallocSite* site = head->data();
if (site->flag() == flags && site->equals(key)) {
if (site->equals(key, flags)) {
Copy link
Contributor

@coleenp coleenp Feb 12, 2021

Choose a reason for hiding this comment

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

Does this now assert when it used to just return false if memflags didn't match?

Copy link
Member Author

@tstuefe tstuefe Feb 13, 2021

Choose a reason for hiding this comment

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

Yes, and it seems this actually fired tonight on s390x. Which indicates that the call stack frame skipping is off and we do skip too much. I opened https://bugs.openjdk.java.net/browse/JDK-8261556 to track this, and here I will restore the old behavior for now. That is also cleaner since this RFE should not have brought behavioral changes.

AllocationSite(stack, mtThreadStack),
_base(base),
_size(size)
{}
Copy link
Contributor

@coleenp coleenp Feb 12, 2021

Choose a reason for hiding this comment

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

nit: can you put {} on the same line as _size ?

Copy link
Member Author

@tstuefe tstuefe Feb 13, 2021

Choose a reason for hiding this comment

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

Done.

@openjdk
Copy link

openjdk bot commented Feb 12, 2021

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

8261644: NMT: Simplifications and cleanups

Reviewed-by: coleenp, zgu

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

  • 78cde64: 8261860: Crash caused by lambda proxy class loaded in Shutdown hook
  • c158413: 8261098: Add clhsdb "findsym" command
  • 0c31d5b: 8261977: Fix comment for getPrefixed() in canonicalize_md.c
  • 9cf4f90: 8261473: Shenandoah: Add breakpoint support
  • c4664e6: 8261940: Fix references to IOException in BigDecimal javadoc
  • 0e9c5ae: 8075909: [TEST_BUG] The regression-swing case failed as it does not have the 'Open' button when select 'subdir' folder with NimbusLAF
  • e9f3aab: 8261912: Code IfNode::fold_compares_helper more defensively
  • fd098e7: 8261838: Shenandoah: reconsider heap region iterators memory ordering
  • f94a845: 8261600: NMT: Relax memory order for updating MemoryCounter and fix racy updating of peak values
  • 1a7adc8: 8260416: Remove unused method ReferenceProcessor::is_mt_processing_set_up()
  • ... and 81 more: https://git.openjdk.java.net/jdk/compare/1740de2a0d41d0689ae9bb069b93ab267a2b9495...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 Feb 12, 2021
Copy link
Contributor

@zhengyu123 zhengyu123 left a comment

Looks good to me

Copy link
Contributor

@coleenp coleenp left a comment

Looks good!

@tstuefe
Copy link
Member Author

tstuefe commented Feb 19, 2021

Thanks Coleen and Zhengyu!

/integrate

@openjdk openjdk bot closed this Feb 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 19, 2021
@openjdk
Copy link

openjdk bot commented Feb 19, 2021

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

  • ed93bc9: 8196301: java/awt/print/PrinterJob/Margins.java times out
  • 7e78c77: 8261905: Move implementation of OopStorage num_dead related functions
  • 78cde64: 8261860: Crash caused by lambda proxy class loaded in Shutdown hook
  • c158413: 8261098: Add clhsdb "findsym" command
  • 0c31d5b: 8261977: Fix comment for getPrefixed() in canonicalize_md.c
  • 9cf4f90: 8261473: Shenandoah: Add breakpoint support
  • c4664e6: 8261940: Fix references to IOException in BigDecimal javadoc
  • 0e9c5ae: 8075909: [TEST_BUG] The regression-swing case failed as it does not have the 'Open' button when select 'subdir' folder with NimbusLAF
  • e9f3aab: 8261912: Code IfNode::fold_compares_helper more defensively
  • fd098e7: 8261838: Shenandoah: reconsider heap region iterators memory ordering
  • ... and 83 more: https://git.openjdk.java.net/jdk/compare/1740de2a0d41d0689ae9bb069b93ab267a2b9495...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5caf686.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
4 participants