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

8264149 BreakpointInfo::set allocates metaspace object in VM thread #3207

Closed
wants to merge 7 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Mar 25, 2021

This change creates a Metaspace::allocate function that doesn't pass TRAPS to be used by MethodCounters. TRAPS and exceptions shouldn't be thrown from non-JavaThreads.

Tested with tier1-7.


Progress

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

Issue

  • JDK-8264149: BreakpointInfo::set allocates metaspace object in VM thread

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3207/head:pull/3207
$ git checkout pull/3207

Update a local copy of the PR:
$ git checkout pull/3207
$ git pull https://git.openjdk.java.net/jdk pull/3207/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3207

View PR using the GUI difftool:
$ git pr show -t 3207

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3207.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 25, 2021

👋 Welcome back coleenp! 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 Mar 25, 2021

@coleenp 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 hotspot-dev@openjdk.org label Mar 25, 2021
@coleenp coleenp changed the title Metaspace 8264149 BreakpointInfo::set allocates metaspace object in VM thread Mar 26, 2021
@coleenp coleenp marked this pull request as ready for review March 26, 2021 23:02
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 26, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 26, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks,
David

src/hotspot/share/memory/metaspace.cpp Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Mar 29, 2021

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

8264149: BreakpointInfo::set allocates metaspace object in VM thread

Reviewed-by: dholmes, iklam

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

  • 928fa5b: 8244540: Print more information with -XX:+PrintSharedArchiveAndExit
  • e073486: 8262093: java/util/concurrent/tck/JSR166TestCase.java failed "assert(false) failed: unexpected node"
  • 815248a: 8264148: Update spec for exceptions retrofitted for exception chaining
  • 353807c: 8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win)
  • 2bd80f9: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation
  • b08d638: 8262503: Support records in Dynalink
  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • ... and 46 more: https://git.openjdk.java.net/jdk/compare/b8122d6e3b2ce20b943997cccc3096a72bc26b49...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 Pull request is ready to be integrated label Mar 29, 2021
@coleenp
Copy link
Contributor Author

coleenp commented Mar 29, 2021

Thanks David. @tstuefe Can you review this?

@tstuefe
Copy link
Member

tstuefe commented Mar 29, 2021

Hi Coleen,

GH swallowed my in code comment, so I repeat it here...

I'm not sure this is correct. Your new non-TRAPS Metaspace::allocate() would fail every time the GC threshold is touched. Where the old TRAPS version would break through the threshold and allocate successfully.

CLMS->allocate() fails not only for "hard" OOMS, when we ran against MaxMetaspaceSize or CompressedClassSpaceSize, but also when we hit the GC threshold. satisfy_failed_xxx handles the latter case by increasing the threshold and retrying the allocation, in addition to scheduling a new GC maybe.

I guess the next "real" allocate, done with TRAPS, would then remove the blockage and increase the threshold and life goes on, but a number of metaspace allocations could have unnecessarily failed. This means that periodically we end up without profile counters on methods, and whatever the effect is of CLDG->had_metaspace_oom.

It looks like you could solve that by manually calling ClassLoaderMetaspace::expand_and_allocate after a failing ClassLoaderMetaspace::allocate in your non-TRAPS version. That may mean though that we miss out on GC possibilities, basically ignoring some instances of a triggered GC threshold without doing a GC.

Would be nice to simplify this. See also #2289. This stuff is very complex.

Cheers, Thomas

@coleenp coleenp closed this Mar 29, 2021
@coleenp coleenp deleted the metaspace branch March 29, 2021 20:03
@coleenp coleenp restored the metaspace branch March 29, 2021 20:04
@coleenp coleenp reopened this Mar 29, 2021
@coleenp
Copy link
Contributor Author

coleenp commented Mar 29, 2021

I deleted this branch by mistake, now restored.

I'm not sure this is correct. Your new non-TRAPS Metaspace::allocate() would fail every time the GC threshold is touched. Where the old TRAPS version would break through the threshold and allocate successfully.

I realize this. It's just an attempt to allocate and it's designed to be used during a safepoint for only this allocation. I could change this to only call the non-TRAPS version of MethodCounters if we're at a safepoint? Would that help? Then the only time we'll miss out on metaspace counters periodically is if they were created to set breakpoints in a safepoint.

I'd hate for this special case to know more about metaspace, ala calling ClassLoaderMetaspace::expand_and_allocate.

Comment on lines 568 to 570
if (current->is_Java_thread()) {
// For when TRAPS is JavaThread.
counters = MethodCounters::allocate(mh, current->as_Java_thread());
Copy link
Member

Choose a reason for hiding this comment

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

I'm not at all clear what we are doing here and it seems premature to anticipate the TRAPS change to JavaThread. The code will need reworking for that change because you still check for a pending exception regardless of what type of Thread current is.
I don't see why we need to call a TRAPS version of allocate when we are going to clear any exception - just call the non-traps version (and remove the assert you added). Just because we are in a JavaThread it doesn't mean we have to throw exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, if the thread is not a Java thread, say the VMThread, we cannot throw an exception because we can't allocate a Java object for that exception. That's why we call the non-TRAPS version for !JavaThread. So there's no need to check for a pending exception because it can't throw one. Also, eventually we want to enforce that only JavaThreads can throw/catch Java exceptions.
The change requested by Thomas was that if we can thrown an exception, we should call the TRAPS version because that version will adjust the GC threshold. We can't call GC with the non-TRAPS version.
So this is the simplest thing. We have two versions of this function (the TRAPS version calls the non-TRAPS version). The non-TRAPS version is the special one that can be used by the VMThread for allocating metadata, all other callers stay the same.
I can remove the as_Java_thread() and line 569 and you can add it back when we're able to change TRAPS to JavaThread.

Copy link
Member

Choose a reason for hiding this comment

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

I deleted this branch by mistake, now restored.

I'm not sure this is correct. Your new non-TRAPS Metaspace::allocate() would fail every time the GC threshold is touched. Where the old TRAPS version would break through the threshold and allocate successfully.

I realize this. It's just an attempt to allocate and it's designed to be used during a safepoint for only this allocation. I could change this to only call the non-TRAPS version of MethodCounters if we're at a safepoint? Would that help? Then the only time we'll miss out on metaspace counters periodically is if they were created to set breakpoints in a safepoint.

I think that would be better. I am unclear on what happened in this case before; did we also miss out on allocating the Counters?

I'd hate for this special case to know more about metaspace, ala calling ClassLoaderMetaspace::expand_and_allocate.

Even within Metaspace::allocate(no TRAPS)? Its in metaspace land, surely it would be fine to call expand there like this:

MetaWord* Metaspace::allocate(ClassLoaderData* loader_data, size_t word_size, MetaspaceObj::Type type) {
  MetaWord* result = loader_data->metaspace_non_null()->allocate(...);
  if (!result) {
    MetaWord* result = loader_data->metaspace_non_null()->expand_and_allocate(...);
  }

(Note that I will be gone into vacation shortly and I'm a bit short on time; I'm not sure I can finish this review. If you go with your approach, my only request would be to comment the prototypes for the two allocate functions a bit clearer and/or maybe rename one as allocate_no_exception or the other as allocate_with_exception)

Copy link
Contributor Author

@coleenp coleenp Mar 30, 2021

Choose a reason for hiding this comment

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

I think that would be better. I am unclear on what happened in this case before; did we also miss out on allocating the Counters?

Before this change, it was very unlikely that allocating metaspace counters in a breakpoint safepoint ran out of memory, so never threw the exception. Or else they did and returned NULL and all of the code around their allocation has handling for a null return. We're working on enforcing what should be a rule that only JavaThreads can throw exceptions and this was an exception to that. :)

Re: expand_and_allocate()

I didn't want to expose internal metaspace functions or more handling for this special case, and that would prevent the nice sharing of most of the Metaspace::allocate code. It's allowed for method counters to return NULL here. If it's not long term, we should move the breakpoint counters to Method (but it would increase Method by a pointer size, which isn't good).

I will rename the functions as requested. Have a nice vacation and thank you for your comments.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining! I am fine with the change in this case.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Mar 30, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Coleen,

Updated code is much clearer but I have a suggested refactoring to avoid the duplicated OOM code.

Thanks,
David

src/hotspot/share/oops/method.cpp Outdated Show resolved Hide resolved
src/hotspot/share/oops/method.cpp Outdated Show resolved Hide resolved
src/hotspot/share/oops/method.cpp Outdated Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Mar 30, 2021

Mailing list message from David Holmes on hotspot-dev:

On 30/03/2021 12:41 pm, Coleen Phillimore wrote:

On Tue, 30 Mar 2021 01:17:27 GMT, David Holmes <dholmes at openjdk.org> wrote:

Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:

Only allow non-TRAPS version of Metaspace::allocate at a safepoint (or by non-Java thread)

src/hotspot/share/oops/method.cpp line 570:

568: if (current->is_Java_thread()) {
569: // For when TRAPS is JavaThread.
570: counters = MethodCounters::allocate(mh, current->as_Java_thread());

I'm not at all clear what we are doing here and it seems premature to anticipate the TRAPS change to JavaThread. The code will need reworking for that change because you still check for a pending exception regardless of what type of Thread current is.
I don't see why we need to call a TRAPS version of allocate when we are going to clear any exception - just call the non-traps version (and remove the assert you added). Just because we are in a JavaThread it doesn't mean we have to throw exceptions.

With this change, if the thread is not a Java thread, say the VMThread, we cannot throw an exception because we can't allocate a Java object for that exception. That's why we call the non-TRAPS version for !JavaThread. So there's no need to check for a pending exception because it can't throw one. Also, eventually we want to enforce that only JavaThreads can throw/catch Java exceptions.
The change requested by Thomas was that if we *can* thrown an exception, we should call the TRAPS version because that version will adjust the GC threshold. We can't call GC with the non-TRAPS version.

Ah I see. That makes sense.

So this is the simplest thing. We have two versions of this function (the TRAPS version calls the non-TRAPS version). The non-TRAPS version is the special one that can be used by the VMThread for allocating metadata, all other callers stay the same.
I can remove the as_Java_thread() and line 569 and you can add it back when we're able to change TRAPS to JavaThread.

No that's fine. I've added feedback on the latest version.

Thanks,
David

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Looks good.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 30, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 30, 2021

Mailing list message from David Holmes on hotspot-dev:

On 31/03/2021 8:37 am, Coleen Phillimore wrote:

On Tue, 30 Mar 2021 03:53:54 GMT, David Holmes <dholmes at openjdk.org> wrote:

Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:

Make which version of MethodCounters::allocate() is called clearer.

src/hotspot/share/oops/method.cpp line 570:

568: if (current->is_Java_thread()) {
569: Thread* THREAD = current;
570: counters = MethodCounters::allocate(mh, THREAD);

Can you add a comment before this line:

// Use the TRAPS version for a JavaThread so it will adjust the GC threshold if needed.

Thanks.

ok, yes that says why.

src/hotspot/share/oops/method.cpp line 572:

570: counters = MethodCounters::allocate(mh, THREAD);
571: if (HAS_PENDING_EXCEPTION) {
572: CLEAR_PENDING_EXCEPTION; // MethodData above doesn't clear exception

I don't understand the comment.

It's sort of a question, because the above code that does the same thing for MethodData doesn't clear the exception. In this case we should clear the exception I believe. I'll remove the comment.

Ah I see. All the callers of build_interpreter_method_data clear the
exception themselves, rather than it being cleared internally. That
seems odd.

src/hotspot/share/oops/method.cpp line 575:

573: CompileBroker::log_metaspace_failure();
574: ClassLoaderDataGraph::set_metaspace_oom(true);
575: return NULL;

You could factor this out for both cases by testing "counters == NULL".

Yes, this is better.

Refactoring looks good!

Thanks,
David

@coleenp
Copy link
Contributor Author

coleenp commented Mar 30, 2021

Thanks, David!

Copy link
Member

@iklam iklam left a comment

Choose a reason for hiding this comment

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

LGTM.

@coleenp
Copy link
Contributor Author

coleenp commented Mar 31, 2021

Thanks Ioi!
/integrate

@openjdk openjdk bot closed this Mar 31, 2021
@openjdk 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 Mar 31, 2021
@openjdk
Copy link

openjdk bot commented Mar 31, 2021

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

  • 999c134: 8264417: ParallelCompactData::region_offset should not accept pointers outside the current region
  • 604b14c: 8264112: (fs) Reorder methods/constructor/fields in UnixUserDefinedFileAttributeView.java
  • 9061271: 8261957: [PPC64] Support for Concurrent Thread-Stack Processing
  • 8a4a911: 8262894: [macos_aarch64] SIGBUS in Assembler::ld_st2
  • ab6faa6: 8263582: WB_IsMethodCompilable ignores compiler directives
  • 928fa5b: 8244540: Print more information with -XX:+PrintSharedArchiveAndExit
  • e073486: 8262093: java/util/concurrent/tck/JSR166TestCase.java failed "assert(false) failed: unexpected node"
  • 815248a: 8264148: Update spec for exceptions retrofitted for exception chaining
  • 353807c: 8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win)
  • 2bd80f9: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation
  • ... and 51 more: https://git.openjdk.java.net/jdk/compare/b8122d6e3b2ce20b943997cccc3096a72bc26b49...master

Your commit was automatically rebased without conflicts.

Pushed as commit 40c3249.

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

@coleenp coleenp deleted the metaspace branch March 31, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
4 participants