8369219: JNI::RegisterNatives causes a memory leak in CodeCache #27742

fandreuz · 2025-10-10T11:50:50Z

I propose to amend nmethod::is_cold to let GC collect not-entrant native nmethod instances.

Passes tier1 and tier2 (fastdebug).

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8369219: JNI::RegisterNatives causes a memory leak in CodeCache (Bug - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27742/head:pull/27742
$ git checkout pull/27742

Update a local copy of the PR:
$ git checkout pull/27742
$ git pull https://git.openjdk.org/jdk.git pull/27742/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27742

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27742.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-10T11:51:53Z

👋 Welcome back fandreuzzi! 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 · 2025-10-10T11:51:59Z

@fandreuz This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-10-10T11:52:49Z

@fandreuz 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.

mlbridge · 2025-10-11T14:56:38Z

Webrevs

test/hotspot/jtreg/gc/NativeWrapperCollection/NativeWrapperCollection.java

dholmes-ora · 2025-10-13T01:56:42Z

/label add hotspot-compiler

openjdk · 2025-10-13T01:57:39Z

@dholmes-ora
The hotspot-compiler label was successfully added.

dean-long · 2025-10-16T02:36:11Z

src/hotspot/share/code/nmethod.cpp

 // nmethods that don't seem to be all that relevant any longer.
 bool nmethod::is_cold() {
-  if (!MethodFlushing || is_native_method() || is_not_installed()) {
+  if (!MethodFlushing || (is_native_method() && is_in_use()) || is_not_installed()) {


So I guess we need to decide what to do about native wrappers that are still "in use", but are "cold" because they haven't been called in a while. The above change would keep them around forever. We could instead allow them to be cleaned up like regular nmethods.

We could instead allow them to be cleaned up like regular nmethods.

That sounds reasonable to me, native methods seem to be tracked like all other nmethods.

Removing is_native_method() altogether from the condition was the first implementation I had, and as far as I remember there was no failure in tier1 or tier2. Should I propose this alternative implementation as part of this PR?

I am tempted to say yes, for consistency, but it probably won't make much of a difference either way. But now I am wondering, if these cold native wrappers continue to be immortal, then do they really need to give them nmethod entry barriers? Removing the barrier could remove some overhead. Whatever direction we decide to go, it would be good to add a comment here explaining the decision and/or trade-offs.

Is it actually possible to remove entry barriers for any garbage collectable nmethod? How can we know an nmethod is not used anymore, even when it is made not entrant? is_cold() bails out when an nmethod does not support entry barriers:

// On platforms that don't support nmethod entry barriers, we can't // trust the temporal aspect of the gc epochs. So we can't detect // cold nmethods on such platforms.

So, the decision of removing entry barriers for native nmethods would make the memory leak I'm trying to fix here effectively unfixable? Let me know if I'm missing something.

If we mark them as not-entrant, then the is_not_entrant() check below will still catch them, right?

I see, I assumed an nmethod couldn't be marked as on-stack without entry barriers, but that doesn't seem to be the case.

But on second thought, do you agree with the fix I'm proposing in this PR? I think the following two work items could be implemented and reviewed in separate changesets:

Allow not-entrant nmethod to be collected during GC (I removed is_static_method() from L2599, so native nmethods are treated just like normal nmethods)

Evaluate the implications of removing entry barriers for native nmethods, thus letting GC reclaim them whenever !is_maybe_on_stack() && is_not_entrant(), but without the overhead of entry barriers.

I'm proposing this because I guess the latter will need more discussion and is technically not needed to fix the memory leak I address in this PR. Do you agree @dean-long ? I could create another ticket to handle the second item.

Yes, I'm fine with it being a separate issue.

https://bugs.openjdk.org/browse/JDK-8370472

shipilev

Looks reasonable, but the test needs a bit more work.

/reviewers 2

shipilev · 2025-10-24T13:39:19Z

test/hotspot/jtreg/gc/NativeWrapperCollection/NativeWrapperCollection.java

+
+        WB.enqueueMethodForCompilation(method, 1 /* compLevel */);
+        while (WB.isMethodQueuedForCompilation(method)) {
+            Thread.onSpinWait();


We are just waiting for compilation here. It is counter-productive to wait with a busy-loop. Insert a sleep for ~10...100ms instead. Same thing for the loop below.

Thanks, that sounds reasonable. I got this pattern from another test, but it looks counterproductive here indeed.

5d0c705

openjdk · 2025-10-24T13:48:10Z

@shipilev
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

fix and test

fb88acf

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 10, 2025

fandreuz changed the title ~~8369219: Let GC reclaim cold native wrappers~~ 8369219: JNI::RegisterNatives can cause a memory leak in CodeCache Oct 10, 2025

fandreuz added 6 commits October 11, 2025 10:49

fix check

273c514

cc

5358cc0

comments and fix test

a5ec01e

stuff

867af27

othervm

8b88b55

nn

38e8a7b

fandreuz marked this pull request as ready for review October 11, 2025 14:51

trigger

ab6b805

openjdk bot added the rfr Pull request is ready for review label Oct 11, 2025

Merge branch 'master' into JDK-8369219

0323b33

fandreuz added 2 commits October 11, 2025 14:59

nn

b3eae89

fix summary

6bba768

apangin reviewed Oct 11, 2025

View reviewed changes

test/hotspot/jtreg/gc/NativeWrapperCollection/NativeWrapperCollection.java Show resolved Hide resolved

update foundOne

8ea13b6

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 13, 2025

fandreuz changed the title ~~8369219: JNI::RegisterNatives can cause a memory leak in CodeCache~~ 8369219: JNI::RegisterNatives causes a memory leak in CodeCache Oct 13, 2025

dean-long reviewed Oct 16, 2025

View reviewed changes

fandreuz marked this pull request as draft October 18, 2025 09:51

openjdk bot removed the rfr Pull request is ready for review label Oct 18, 2025

fandreuz marked this pull request as ready for review October 18, 2025 10:03

openjdk bot added the rfr Pull request is ready for review label Oct 18, 2025

fandreuz added 3 commits October 22, 2025 20:53

nn

98754ee

Merge branch 'master' into JDK-8369219

f269b7e

cc

b6d94cf

shipilev approved these changes Oct 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 24, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Oct 24, 2025

sleep

5d0c705

shipilev approved these changes Oct 24, 2025

View reviewed changes

8369219: JNI::RegisterNatives causes a memory leak in CodeCache #27742

Are you sure you want to change the base?

8369219: JNI::RegisterNatives causes a memory leak in CodeCache #27742

Conversation

fandreuz commented Oct 10, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 10, 2025

Uh oh!

openjdk bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

dholmes-ora commented Oct 13, 2025

Uh oh!

openjdk bot commented Oct 13, 2025

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!

fandreuz Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fandreuz Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

shipilev 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!

openjdk bot commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

fandreuz commented Oct 10, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 10, 2025 •

edited

Loading

openjdk bot commented Oct 10, 2025 •

edited

Loading

mlbridge bot commented Oct 11, 2025 •

edited

Loading

fandreuz Oct 18, 2025 •

edited

Loading

fandreuz Oct 22, 2025 •

edited

Loading