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

8208257: [mlvm] Add randomness keyword to vm/mlvm/meth/func/jdi/breakpointOtherStratum #309

Closed
wants to merge 1 commit into from

Conversation

@lepestock
Copy link
Contributor

@lepestock lepestock commented Sep 22, 2020

Pre-Scara thread: link.

I tried to reproduce the test multiple times with different VM parameters, but it always passes. I suggest removing it from ProblemList.txt.

Second change is marking the test with randomness keyword from the JDK-8243427 (using reproducible random for mlvm tests).

Tested using mach5 on the 4 platforms, 50 runs each.


Progress

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

Issue

  • JDK-8208257: [mlvm] Add randomness keyword to vm/mlvm/meth/func/jdi/breakpointOtherStratum

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 22, 2020

👋 Welcome back enikitin! 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 Sep 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@lepestock The following label will be automatically applied to this pull request: hotspot-compiler.

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 (add|remove) "label" command.

@openjdk openjdk bot added the hotspot-compiler label Sep 22, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 22, 2020

Webrevs

@lepestock
Copy link
Contributor Author

@lepestock lepestock commented Sep 22, 2020

A question by Igor Ignatyev:

looks good to me, you will need to update 8208257's title in JBS and close 8058176 as CNR.

The answer:

8058176 is creating lots of i2/c2i adapters and eats the code cache very intensively. I added a code cache monitor to the test (0e99acf), and it eats 8M of cache almost immediately:

00:00.001 Cache Monitor started
00:00.009 Cache Monitor: 16% (1.3 MB) used
00:00.131 Cache Monitor: 20% (1.6 MB) used
00:00.232 Cache Monitor: 27% (2.2 MB) used
00:00.332 Cache Monitor: 36% (2.9 MB) used
00:00.434 Cache Monitor: 45% (3.6 MB) used
00:00.535 Cache Monitor: 60% (4.9 MB) used
00:00.637 Cache Monitor: 71% (5.7 MB) used
00:00.745 Cache Monitor: 79% (6.3 MB) used
00:00.847 Cache Monitor: 86% (7 MB) used
00:00.982 Cache Monitor: 92% (7.4 MB) used
00:01.084 Cache Monitor: 84% (6.8 MB) used
00:01.188 Cache Monitor: 89% (7.2 MB) used
00:01.292 Cache Monitor: 88% (7.1 MB) used
00:01.394 Cache Monitor: 89% (7.2 MB) used
00:01.496 Cache Monitor: 92% (7.4 MB) used
00:01.598 Cache Monitor: 96% (7.7 MB) used
00:01.700 Cache Monitor: 98% (7.9 MB) used
[1.865s][warning][codecache] CodeCache is full. Compiler has been disabled.
[1.865s][warning][codecache] Try increasing the code cache size using -XX:ReservedCodeCacheSize=

breakpointTheOtherStratum, in its turn, was hanging, I guess, because of some logical problems (debuggee hanged, debugger crashed, etc.). Not a performance/cache exhaustion. Please check similar output for the breakpointTheOtherStratum (b826254):

binder> Launching debugee
00:00.240 Cache Monitor: 24% (1.9 MB) used
00:00.341 Cache Monitor: 26% (2.1 MB) used
binder> Waiting for VM initialized
Initial VMStartEvent received: VMStartEvent in thread main
00:00.442 Cache Monitor: 29% (2.4 MB) used
00:00.543 Cache Monitor: 32% (2.6 MB) used
00:00.644 Cache Monitor: 33% (2.7 MB) used
00:00.745 Cache Monitor: 34% (2.7 MB) used
00:00.845 Cache Monitor: 35% (2.8 MB) used
00:00.946 Cache Monitor: 35% (2.8 MB) used
00:01.047 Cache Monitor: 33% (2.7 MB) used
00:01.149 Cache Monitor: 34% (2.7 MB) used
00:01.250 Cache Monitor: 34% (2.7 MB) used
00:01.351 Cache Monitor: 36% (3 MB) used
00:01.452 Cache Monitor: 39% (3.2 MB) used
00:01.554 Cache Monitor: 41% (3.3 MB) used
00:01.655 Cache Monitor: 43% (3.5 MB) used
00:01.756 Cache Monitor: 45% (3.6 MB) used
00:01.858 Cache Monitor: 47% (3.8 MB) used
00:01.959 Cache Monitor: 49% (4 MB) used
debugee.stdout> ### TRACE 1: DEBUGGEE PASSED

The second one does not eat memory that aggressively, the difference between lowest and highest amounts is just 2MB.

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 22, 2020

A question by Igor Ignatyev:

looks good to me, you will need to update 8208257's title in JBS and close 8058176 as CNR.

The answer:

8058176 is creating lots of i2/c2i adapters and eats the code cache very intensively. I added a code cache monitor to the test (0e99acf), and it eats 8M of cache almost immediately:
<...>

breakpointTheOtherStratum, in its turn, was hanging, I guess, because of some logical problems (debuggee hanged, debugger crashed, etc.). Not a performance/cache exhaustion. Please check similar output for the breakpointTheOtherStratum (b826254):

<...>

the reason I suggested to close 8058176 as CNR is b/c this test is problem-listed due to 8208257 and 8058176. so the absence of the failure in your rerun would mean that both 8058176 and 8208257 can not be reproduced, hence both should be closed.

I also would like to change my statement about changing 8208257's title, I think it would be better to restore the original title, open a new issue to unproblem-list the test, close both 8208257 and 8058176 as CNR. adding randomness k/w can be done as a part of this new issue or by a separate RFE.

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 22, 2020

or are you saying that breakpointOtherStratum test is not (and most probably has never been) affected by 8058176?

@lepestock
Copy link
Contributor Author

@lepestock lepestock commented Sep 22, 2020

or are you saying that breakpointOtherStratum test is not (and most probably has never been) affected by 8058176?

Yep. Well, I am still not very confident in how the code cache works, but in case of i2c_c2i I can clearly see how the adapters are created and the memory gets claimed. The breakpointOtherStratum does something different, and uses the code cache modestly.

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 22, 2020

or are you saying that breakpointOtherStratum test is not (and most probably has never been) affected by 8058176?

Yep. Well, I am still not very confident in how the code cache works, but in case of i2c_c2i I can clearly see how the adapters are created and the memory gets claimed. The breakpointOtherStratum does something different, and uses the code cache modestly.

I see, when, although, I still think it would be somewhat cleaner to restore 8208257's title and use separate issue(s) to unproblem-list and add k/w, I wouldn't insist on that. the patch looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 2020

@lepestock 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 more details.

After integration, the commit message for the final commit will be:

8208257: [mlvm] Add randomness keyword to vm/mlvm/meth/func/jdi/breakpointOtherStratum

Reviewed-by: iignatyev

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

  • 812b39f: 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary
  • 5f1d612: 8253397: Ensure LogTag types are sorted
  • b8ea80a: 8253457: Remove unimplemented register stack functions
  • e4d0e5a: 8253516: ZGC: Remove card table functions
  • 3fe5886: 8252696: Loop unswitching may cause out of bound array load to be executed
  • 226faa5: 8253241: Update comment on java_suspend_self_with_safepoint_check()
  • bd67975: 8253349: Remove unimplemented SharedRuntime::native_method_throw_unsupported_operation_exception_entry
  • bddb822: 8253240: No javadoc for DecimalFormatSymbols.hashCode()
  • c68a31d: 8253499: Problem list runtime/cds/DeterministicDump.java
  • 93a2018: 8252195: AWT Accessibility API nested classes rely on default constructors
  • ... and 114 more: https://git.openjdk.java.net/jdk/compare/68da63dcdead52418b41f80d381b105ce71a8162...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 (@iignatev) 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).

@lepestock
Copy link
Contributor Author

@lepestock lepestock commented Sep 24, 2020

Closed in favour of a new bug (the JDK-8253607) and it's PR #345.

@lepestock lepestock closed this Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.