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

8257145: Performance regression with -XX:-ResizePLAB after JDK-8079555 #1474

Closed
wants to merge 6 commits into from

Conversation

@dongbohe
Copy link
Member

@dongbohe dongbohe commented Nov 27, 2020

Hi,

this is the continuation of the review of the implementation for:

https://bugs.openjdk.java.net/browse/JDK-8257145


Progress

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

Issue

  • JDK-8257145: Performance regression with -XX:-ResizePLAB after JDK-8079555

Reviewers

Contributors

  • Junjun Lin <linjunjun@huawei.com>

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 27, 2020

👋 Welcome back dongbohe! 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 openjdk bot added the rfr label Nov 27, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 27, 2020

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

  • hotspot-gc

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-gc label Nov 27, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 27, 2020

@@ -1406,8 +1406,8 @@ G1CollectedHeap::G1CollectedHeap() :
_summary_bytes_used(0),
_bytes_used_during_gc(0),
_archive_allocator(NULL),
_survivor_evac_stats("Young", YoungPLABSize, PLABWeight),
_old_evac_stats("Old", OldPLABSize, PLABWeight),
_survivor_evac_stats("Young", YoungPLABSize * ParallelGCThreads, PLABWeight),
Copy link
Contributor

@tschatzl tschatzl Nov 27, 2020

Choose a reason for hiding this comment

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

Lgtm, will do some perf measurements and get back to you.
Could you add a comment like the following in the G1EvacStats constructor?

// desired_plab_size_ should be the total PLAB size for all threads.

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.

Just one note, if we run with UseDynamicNumberOfGCThreads the PLAB size might still be "resized" between GCs if they use different number of threads. This is still an improvement since the old value is wrong and will give a way to small PLAB size. But we might want to look at a solution that will always return a fixed PLAB size with -ResizePLAB.

Copy link
Contributor

@tschatzl tschatzl Nov 27, 2020

Choose a reason for hiding this comment

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

I agree and noted that in the CR already. As you mention, it is still an improvement, and with -XX:+ResizePLAB G1 will automatically resize the PLABs back to more reasonable if needed (as G1 previously used a way too small PLAB initially).

Giving that typical number or threads correlate somewhat with heap size, and the policy for -XX:+UseDynamicNumberOfThreads is fairly conservative anyway, this change will "always" return the (correct) fixed PLAB size with -XX:-ResizePLAB now.

Copy link
Contributor

@tschatzl tschatzl Nov 27, 2020

Choose a reason for hiding this comment

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

I.e. if you want (and think it is a good idea), we can do both changes, first the one suggested originally, and this one so that we do not need to review + test twice.

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.

I agree, but to be able to do both we need to store the "default size" for the PLAB in the PLABStats as well, right? What would we otherwise return in the -ResizePLAB case.

But storing both the default and desired size in PLABStats would be ok with me.

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Nov 29, 2020

/test

@openjdk
Copy link

@openjdk openjdk bot commented Nov 29, 2020

@dongbohe you need to get approval to run the tests in tier1 for commits up until 04102cb

@openjdk openjdk bot added the test-request label Nov 29, 2020
Copy link
Contributor

@kstefanj kstefanj left a comment

Looks good, just a few small things.

_survivor_evac_stats("Young", YoungPLABSize, YoungPLABSize * ParallelGCThreads, PLABWeight),
_old_evac_stats("Old", OldPLABSize, OldPLABSize * ParallelGCThreads, PLABWeight),
Copy link
Contributor

@kstefanj kstefanj Nov 30, 2020

Choose a reason for hiding this comment

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

I think you could leave those two unchanged and in the constructor for G1EvacStats do the multiplication with ParallelGCThreads for the desired value.

Copy link
Contributor

@tschatzl tschatzl Nov 30, 2020

Choose a reason for hiding this comment

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

And rename the default_plab_sz parameter to default_per_thread_plab_size (and remove the desired_net_plab_sz_ as suggested).

@@ -169,12 +170,13 @@ class PLABStats : public CHeapObj<mtGC> {
virtual size_t compute_desired_plab_sz();

public:
PLABStats(const char* description, size_t desired_net_plab_sz_, unsigned wt) :
PLABStats(const char* description, size_t default_plab_sz, size_t desired_net_plab_sz_, unsigned wt) :
Copy link
Contributor

@kstefanj kstefanj Nov 30, 2020

Choose a reason for hiding this comment

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

Since you are touching this line it would be nice to remove the trailing _ in desired_net_plab_sz_.

_survivor_evac_stats("Young", YoungPLABSize, YoungPLABSize * ParallelGCThreads, PLABWeight),
_old_evac_stats("Old", OldPLABSize, OldPLABSize * ParallelGCThreads, PLABWeight),
Copy link
Contributor

@tschatzl tschatzl Nov 30, 2020

Choose a reason for hiding this comment

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

And rename the default_plab_sz parameter to default_per_thread_plab_size (and remove the desired_net_plab_sz_ as suggested).

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 1, 2020

/test

@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2020

@dongbohe you need to get approval to run the tests in tier1 for commits up until dd3f9b7

@kstefanj
Copy link
Contributor

@kstefanj kstefanj commented Dec 1, 2020

I think the move to use ParallelGCThreads in g1EvacStats.cpp is good, please also add:

#include "runtime/globals.hpp"

To not rely on other includes.

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 2, 2020

I think the move to use ParallelGCThreads in g1EvacStats.cpp is good, please also add:

#include "runtime/globals.hpp"

To not rely on other includes.

Do you mean adding #include "runtime/globals.hpp" to plab.hpp on Refactor the code?

@kstefanj
Copy link
Contributor

@kstefanj kstefanj commented Dec 2, 2020

Please add it to g1EvacStats.cpp, the refactoring was good.

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 2, 2020

Please add it to g1EvacStats.cpp, the refactoring was good.

When I add it to g1EvacStates.cpp on Refactor the code , like this:

diff --git a/src/hotspot/share/gc/g1/g1EvacStats.cpp b/src/hotspot/share/gc/g1/g1EvacStats.cpp
index f8851b55dda..3f0f1b76cea 100644
--- a/src/hotspot/share/gc/g1/g1EvacStats.cpp
+++ b/src/hotspot/share/gc/g1/g1EvacStats.cpp
@@ -27,6 +27,7 @@
 #include "gc/shared/gcId.hpp"
 #include "logging/log.hpp"
 #include "memory/allocation.inline.hpp"
+#include "runtime/globals.hpp"

 void G1EvacStats::log_plab_allocation() {
   PLABStats::log_plab_allocation();

I still get an error in the build:

=== Output from failing command(s) repeated here ===
* For target hotspot_variant-server_libjvm_objs_g1EvacStats.o:
In file included from /home/hedongbo/temp/jdk/src/hotspot/share/gc/g1/g1EvacStats.hpp:28:0,
                 from /home/hedongbo/temp/jdk/src/hotspot/share/gc/g1/g1EvacStats.cpp:26:
/home/hedongbo/temp/jdk/src/hotspot/share/gc/shared/plab.hpp: In constructor 'PLABStats::PLABStats(const char*, size_t, unsigned int)':
/home/hedongbo/temp/jdk/src/hotspot/share/gc/shared/plab.hpp:180:57: error: 'ParallelGCThreads' was not declared in this scope
     _desired_net_plab_sz(default_per_thread_plab_size * ParallelGCThreads),
                                                         ^~~~~~~~~~~~~~~~~
At global scope:
cc1plus: error: unrecognized command line option '-Wno-cast-function-type' [-Werror]

but it's OK to add it to plab.hpp. So, I do not know whether I have understood your meaning correctly. Thank you for your patient reply.

@kstefanj
Copy link
Contributor

@kstefanj kstefanj commented Dec 2, 2020

As I said:

I think the move to use ParallelGCThreads in g1EvacStats.cpp is good...

I liked that refactoring of moving the use of ParallelGCThreads to G1EvacStats, I just want you to also add the include there.

Copy link
Contributor

@kstefanj kstefanj left a comment

Looks good, thanks for fixing this.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

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

8257145: Performance regression with -XX:-ResizePLAB after JDK-8079555

Co-authored-by: Junjun Lin <linjunjun@huawei.com>
Reviewed-by: tschatzl, sjohanss

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

  • fa20186: 8257676: Simplify WeakProcessorPhase
  • b90b7f5: 8196090: javax/swing/JComboBox/6559152/bug6559152.java fails
  • 1d15ebe: 8243205: Modularize JVM flags declaration
  • 8befc32: 8258073: x86_32 build broken after JDK-8257731
  • 37dc675: 8247402: Documentation for Map::compute contains confusing implementation requirements
  • d4282b0: 8257731: Remove excessive include of stubRoutines.hpp
  • 80dac5a: 8257912: Convert enum iteration to use range-based for loops
  • 164c55b: 8258056: jdk/javadoc/doclet/testHtmlTableTags/TestHtmlTableTags.java fails against jdk17
  • 42264b2: 8257971: (fs) Remove unused code from WindowsPath.subpath(begin, end)
  • 3342eca: 8258054: runtime/sealedClasses/GetPermittedSubclassesTest.java fails w/ jdk17
  • ... and 256 more: https://git.openjdk.java.net/jdk/compare/2215e5a47e1985782f5c4085b16ddb8512d1d654...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tschatzl, @kstefanj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 2, 2020
@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 3, 2020

Thank you for your review, kstefanj.

As we saw in the test, this change will cause ./test/hotspot/jtreg/gc/g1/plab/TestPLABPromotion.java to fail. I'm working on this case and will push it here for review when the work is done.

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 8, 2020

The failed cases are:
test/hotspot/jtreg/gc/g1/plab/TestPLABPromotion.java:100
test/hotspot/jtreg/gc/g1/plab/TestPLABPromotion.java:106

The error message is as follows:

STDERR:
java.lang.RuntimeException: Expect that Survivor direct allocation are similar to all mem consumed
	at gc.g1.plab.TestPLABPromotion.checkLiveObjectsPromotion(TestPLABPromotion.java:168)
	at gc.g1.plab.TestPLABPromotion.checkResults(TestPLABPromotion.java:140)
	at gc.g1.plab.TestPLABPromotion.main(TestPLABPromotion.java:102)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:564)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
	at java.base/java.lang.Thread.run(Thread.java:831)

If except direct allocated, OBJECT_SIZE/word_size should bigger than PLAB_SIZE*WASTE_PCT.

Worked correctly after this patch.

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm.

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 11, 2020

/contributor add Junjun Lin linjunjun@huawei.com

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@dongbohe
Contributor Junjun Lin <linjunjun@huawei.com> successfully added.

@dongbohe
Copy link
Member Author

@dongbohe dongbohe commented Dec 11, 2020

/integrate

@openjdk openjdk bot added the sponsor label Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@dongbohe
Your change (at version 5aabcc3) is now ready to be sponsored by a Committer.

@RealFYang
Copy link
Member

@RealFYang RealFYang commented Dec 11, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@RealFYang @dongbohe Since your change was applied there have been 266 commits pushed to the master branch:

  • fa20186: 8257676: Simplify WeakProcessorPhase
  • b90b7f5: 8196090: javax/swing/JComboBox/6559152/bug6559152.java fails
  • 1d15ebe: 8243205: Modularize JVM flags declaration
  • 8befc32: 8258073: x86_32 build broken after JDK-8257731
  • 37dc675: 8247402: Documentation for Map::compute contains confusing implementation requirements
  • d4282b0: 8257731: Remove excessive include of stubRoutines.hpp
  • 80dac5a: 8257912: Convert enum iteration to use range-based for loops
  • 164c55b: 8258056: jdk/javadoc/doclet/testHtmlTableTags/TestHtmlTableTags.java fails against jdk17
  • 42264b2: 8257971: (fs) Remove unused code from WindowsPath.subpath(begin, end)
  • 3342eca: 8258054: runtime/sealedClasses/GetPermittedSubclassesTest.java fails w/ jdk17
  • ... and 256 more: https://git.openjdk.java.net/jdk/compare/2215e5a47e1985782f5c4085b16ddb8512d1d654...master

Your commit was automatically rebased without conflicts.

Pushed as commit b28b094.

💡 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
4 participants