8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests #1993

dcubed-ojdk · 2021-01-07T23:14:17Z

Adding support for a diagnostic option called NoAsyncDeflationProgressMax
with a default value of 3. If we have three async monitor deflation cycles in a
row with zero monitors deflated, then we adjust the in_use_list_ceiling up.

I've locally built and tested this fix on my MBP13 using the
MonitorUsedDeflationThresholdTest.java test that I wrote when this issue
first came up in June of 2019. I will be including this fix in my next Mach5
Tier[1-3] testing batch.

Progress

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

Issue

JDK-8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 6957c9d
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 6957c9d

Download

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

…ut_rank_check()

…tion requests

bridgekeeper · 2021-01-07T23:14:58Z

👋 Welcome back dcubed! 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 · 2021-01-07T23:15:52Z

@dcubed-ojdk The following label will be automatically applied to this pull request:

hotspot-runtime

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.

dcubed-ojdk · 2021-01-07T23:15:57Z

Note: I'm testing out stacking patches in Git so there are commits for
https://bugs.openjdk.java.net/browse/JDK-8259397 and
https://bugs.openjdk.java.net/browse/JDK-8259349 that show up in this PR.

This is the correct commit for this PR review: af5c3fe

mlbridge · 2021-01-07T23:20:02Z

Webrevs

openjdk · 2021-01-08T03:08:27Z

⚠️ @dcubed-ojdk This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

mlbridge · 2021-01-08T04:38:56Z

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Dan,

On 8/01/2021 9:20 am, Daniel D.Daugherty wrote:

On Thu, 7 Jan 2021 23:14:17 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

Adding support for a diagnostic option called NoAsyncDeflationProgressMax
with a default value of 3. If we have three async monitor deflation cycles in a
row with zero monitors deflated, then we adjust the in_use_list_ceiling up.

I've locally built and tested this fix on my MBP13 using the
MonitorUsedDeflationThresholdTest.java test that I wrote when this issue
first came up in June of 2019. I will be including this fix in my next Mach5
Tier[1-3] testing batch.

Note: I'm testing out stacking patches in Git so there are commits for
https://bugs.openjdk.java.net/browse/JDK-8259397 and
https://bugs.openjdk.java.net/browse/JDK-8259349 that show up in this PR.

This is the correct commit for this PR review: https://github.com/openjdk/jdk/commit/af5c3fec96d1d45af5881947d366e22cb8962dc3

I'm not quite sure what you are trying to test out here. All three
commits are part of this PR and will be integrated when you integrate
this PR. If you wanted this to be handled separately then they should be
in distinct branches and PRs. If you want all three reviewed together
that is fine too.

Cheers,
David

dcubed-ojdk · 2021-01-08T13:57:24Z

I was hoping that each subsequent PR would be baselined on the previous
branch so that each PR would only have a single commit in it. It didn't work
out that way so that's why I included this blurb in a comment:

This is the correct commit for this PR review: af5c3fe

…; fix bad assert tripped by the now working AvgMonitorsPerThreadEstimate option.

…c in monitors_used_above_threshold(); add MonitorUsedDeflationThresholdTest.java.

…st_ceiling from jint to size_t.

…ress_cnt from 'intx' to 'uintx'.

dcubed-ojdk · 2021-01-12T20:13:22Z

I've updated this fix with similar changes that were requested in the
JDK-8259349 code review cycle. I've also taken the standalone test
created back in June 2019 and converted it into a JTREG style test
with automated checks for the proper passing conditions.

Any takers for a review?

Note: At this point, the other two fixes that preceded this one have
been integrated so only this fix shows up in the review links.

This fix passed Mach5 Tier[1-3] testing.

… info about no-progress async monitor deflation cycles and the NoAsyncDeflationProgressMax option.

dholmes-ora

Hi Dan,
Thanks for the additional explanatory comments. The fix looks good to me.

A couple of comments on the test.

Thanks,
David

dholmes-ora · 2021-01-13T02:04:01Z

src/hotspot/share/runtime/synchronizer.cpp

+      _no_progress_cnt >= NoAsyncDeflationProgressMax) {
+    float remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
+    size_t new_ceiling = ceiling + (ceiling * remainder) + 1;
+    size_t old_ceiling = ObjectSynchronizer::in_use_list_ceiling();


You could capture:

size_t old_ceiling = ceiling;

back after line 1157, rather than re-reading it.

dholmes-ora · 2021-01-13T02:08:23Z

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java

+            return;
+        }
+
+        Object obj = new Object();


This is a thread-local object, which means the synchronization has no affect, so I think C2 could elide the following sync-block. Using a static field would make it less likely that this can happen.

But it has to be a static array of Objects since we want a new
ObjectMonitor per recursion.

dholmes-ora · 2021-01-13T02:14:34Z

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java

+            if (too_many == null) {
+                throw new RuntimeException("Did not find too_many string in output.\n");
+            }


You should call output_detail.reportDiagnosticSummary() before throwing the exception. That emulates what OutputAnalyzer.shouldContain would do. (or you could just use shouldContain).

I added a call to output_detail.reportDiagnosticSummary().
I didn't use OutputAnalyzer.shouldContain() intentionally
because of this output line:

System.out.println("too_many='" + too_many + "'");

which is there for diagnostic purposes.

Manually tested the use of output_detail.reportDiagnosticSummary()
by temporarily setting too_many = null; before the check. It caused
nice diagnostic output to be printed. I like this technique!

For the record, when the test passes, the output looks like this:

[2021-01-13T16:12:47.673259Z] Gathering output for process 15104 Found beginning of a deflation cycle. too_many='Too many deflations without progress; bumping in_use_list_ceiling from 36 to 40' PASSED. ----------System.err:(1/15)---------- STATUS:Passed. ----------rerun:(34/5039)*----------

so there's just a bit of diagnostic output from the test in the passing case.

dholmes-ora · 2021-01-13T02:16:42Z

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java

+            System.out.println("PASSED.");
+            return;
+        }
+


Please add a comment e.g.
`// else we are the exec'd java subprocess, so run the actual test

dcubed-ojdk · 2021-01-13T16:43:53Z

@dholmes-ora - Please re-review when you get the chance.

I could use a second reviewer...

dholmes-ora

A couple of minor comments.

Thanks,
David

dholmes-ora · 2021-01-13T23:17:07Z

src/hotspot/share/runtime/synchronizer.cpp

@@ -1167,7 +1167,7 @@ static bool monitors_used_above_threshold(MonitorList* list) {
      _no_progress_cnt >= NoAsyncDeflationProgressMax) {
    float remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
    size_t new_ceiling = ceiling + (ceiling * remainder) + 1;
-    size_t old_ceiling = ObjectSynchronizer::in_use_list_ceiling();
+    size_t old_ceiling = ceiling;


Is that really what you want? ceiling may have been assigned again at line 1160.

I misread your comment... Fixing it now...

dholmes-ora · 2021-01-13T23:26:45Z

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java

@@ -45,6 +45,7 @@
 public class MonitorUsedDeflationThresholdTest {
    public static final int DELAY_SECS = 10;
    public static int inflate_count = 0;
+    public static Object monitors[];


Style nit: Object[] monitors;

Ahhh.... fixing it.

openjdk · 2021-01-13T23:28:22Z

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

8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests

Reviewed-by: dholmes, coleenp

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

51e14f2: Merge
5926d75: 8259719: ProblemList runtime/cds/appcds/jigsaw/modulepath/ModulePathAndCP_JFR.java on Windows
8abefde: 8259720: ProblemList java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest1.java on Windows
fb8ac24: 8259722: ProblemList two jdk/jfr/startupargs tests on Windows
ac4cd2e: 8231461: static/instance overload leads to 'unexpected static method found in unbound lookup' when resolving method reference
42d2d6d: 8259063: Possible deadlock with vtable/itable creation vs concurrent class unloading
6bb6093: 8259657: typo in generated HELP page prevents localization
5567530: 8258272: LoadVectorMaskedNode can't be replaced by zero con
a99df45: 8259560: Zero m68k: "static assertion failed: align" after JDK-8252049
efc36be: 8258985: Parallel WeakProcessor may use too few threads
... and 30 more: https://git.openjdk.java.net/jdk/compare/c338f1167f2ab0c1a35876419db914dce844d5e8...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.

dcubed-ojdk · 2021-01-14T01:09:08Z

@dholmes-ora - let's try this again.

Still need one more reviewer...

dholmes-ora

LGTM! :)

Thanks,
David

coleenp

LGTM.

coleenp · 2021-01-14T01:23:57Z

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java

+import java.io.FileInputStream;
+import java.io.InputStreamReader;
+import java.util.regex.Pattern;
+*/


Why are these imports commented out?

Oops. I should have deleted those.

dcubed-ojdk · 2021-01-14T01:36:54Z

@dholmes-ora and @coleenp - Thanks for the reviews! I'll integrate
tomorrow when I can watch the CI...

dcubed-ojdk · 2021-01-14T14:22:55Z

/integrate

openjdk · 2021-01-14T14:23:52Z

@dcubed-ojdk Since your change was applied there have been 48 commits pushed to the master branch:

c822eda: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset)
ff3e6e4: 8259773: Incorrect encoding of AVX-512 kmovq instruction
b8ef2ba: 8259563: The CPU model name is printed multiple times when using -Xlog:os+cpu
b040a3d: 8259631: Reapply pattern match instanceof use in HttpClientImpl
3462f7a: 8256955: Move includes of events.hpp out of header files
8b8b1f9: 8259706: C2 compilation fails with assert(vtable_index == Method::invalid_vtable_index) failed: correct sentinel value
ae9187d: 8256109: Create implementation for NSAccessibilityButton protocol
5513f98: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy
51e14f2: Merge
5926d75: 8259719: ProblemList runtime/cds/appcds/jigsaw/modulepath/ModulePathAndCP_JFR.java on Windows
... and 38 more: https://git.openjdk.java.net/jdk/compare/c338f1167f2ab0c1a35876419db914dce844d5e8...master

Your commit was automatically rebased without conflicts.

Pushed as commit be57cf1.

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

dcubed-ojdk added 3 commits January 7, 2021 15:45

8259397: ThreadsSMRSupport::print_info_on() should use try_lock_witho…

d4acfcf

…ut_rank_check()

8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right

0fe88c0

8226416: MonitorUsedDeflationThreshold can cause repeated async defla…

af5c3fe

…tion requests

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 7, 2021

dcubed-ojdk marked this pull request as ready for review January 7, 2021 23:16

openjdk bot added the rfr Pull request is ready for review label Jan 7, 2021

dcubed-ojdk added 2 commits January 7, 2021 22:04

Fix build error on windows.

5b57bd9

Merge branch 'JDK-8259349' into JDK-8226416

eca5066

dcubed-ojdk added 12 commits January 8, 2021 11:27

Merge branch 'master' into JDK-8259349

42343e8

dholmes CR - change AvgMonitorsPerThreadEstimate from 'intx' to 'int'…

e68a5e5

…; fix bad assert tripped by the now working AvgMonitorsPerThreadEstimate option.

Merge branch 'JDK-8259349' into JDK-8226416

e88b2cf

change NoAsyncDeflationProgressMax from 'intx' to 'int'; clarify logi…

efe3e16

…c in monitors_used_above_threshold(); add MonitorUsedDeflationThresholdTest.java.

delete bad assert found by linux build.

f0af3c7

Merge branch 'JDK-8259349' into JDK-8226416

da8d41a

dholmes CR - delete a stale comment.

1fa734f

Merge branch 'JDK-8259349' into JDK-8226416

b1978f5

change AvgMonitorsPerThreadEstimate from int to size_t and _in_use_li…

7737506

…st_ceiling from jint to size_t.

Merge branch 'JDK-8259349' into JDK-8226416

788b693

change NoAsyncDeflationProgressMax from 'int' to 'uintx' and _no_prog…

b1e7db6

…ress_cnt from 'intx' to 'uintx'.

Merge branch 'master' into JDK-8226416

2efe7b4

Update comments for monitors_used_above_threshold() policy to include…

f53c0ea

… info about no-progress async monitor deflation cycles and the NoAsyncDeflationProgressMax option.

dholmes-ora suggested changes Jan 13, 2021

View reviewed changes

address dholmes CR comments.

1d9b6b0

dholmes-ora approved these changes Jan 13, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 13, 2021

address more dholmes CR comments.

6957c9d

dholmes-ora approved these changes Jan 14, 2021

View reviewed changes

coleenp approved these changes Jan 14, 2021

View reviewed changes

coleenp CR - remove commented out imports.

aece28d

openjdk bot closed this Jan 14, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 14, 2021

dcubed-ojdk deleted the JDK-8226416 branch January 14, 2021 17:50

8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests #1993

8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests #1993

Uh oh!

Conversation

dcubed-ojdk commented Jan 7, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Jan 7, 2021

Uh oh!

openjdk bot commented Jan 7, 2021

Uh oh!

dcubed-ojdk commented Jan 7, 2021

Uh oh!

mlbridge bot commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Jan 8, 2021

Uh oh!

mlbridge bot commented Jan 8, 2021

Uh oh!

dcubed-ojdk commented Jan 8, 2021

Uh oh!

dcubed-ojdk commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcubed-ojdk commented Jan 13, 2021

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcubed-ojdk commented Jan 14, 2021

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dcubed-ojdk commented Jan 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 7, 2021 •

edited

Loading

dcubed-ojdk commented Jan 12, 2021 •

edited

Loading

openjdk bot commented Jan 13, 2021 •

edited

Loading