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

8264682: MemProfiling does not own Heap_lock when using G1 #3340

Closed
wants to merge 1 commit into from

Conversation

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Apr 5, 2021

Trivial fix for JDK-8264682.

Universe::heap()->used() calls G1Allocator::used_in_alloc_regions() when G1 enabled, it checks whether Heap_lock was owned on this thread's behalf.

/label add hotspot-gc
/label add hotspot-runtime


Progress

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

Issue

  • JDK-8264682: MemProfiling does not own Heap_lock when using G1

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3340

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 5, 2021

👋 Welcome back yyang! 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 openjdk bot commented Apr 5, 2021

@kelthuzadx
The hotspot-gc label was successfully added.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 5, 2021

/label add hotspot-runtime

@openjdk
Copy link

@openjdk openjdk bot commented Apr 5, 2021

@kelthuzadx
The hotspot-runtime label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 5, 2021

@kelthuzadx The hotspot-runtime label was already applied.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 5, 2021

Webrevs

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Apr 5, 2021

Trivial fix for JDK-8264682.

No, not trivial at all. Dropping a GC-specific blob like this into shared code is highly questionable. I'm not sure how this issue should be addressed, but I don't think the proposed change is the right approach.

@iklam
Copy link
Member

@iklam iklam commented Apr 5, 2021

Trivial fix for JDK-8264682.

No, not trivial at all. Dropping a GC-specific blob like this into shared code is highly questionable. I'm not sure how this issue should be addressed, but I don't think the proposed change is the right approach.

It looks like the following APIs can be reliably called without holding the Heap_lock. However, I am not sure if they would give the same result. I.e., can you get Universe::heap()->unused() by calling Universe::heap()->capacity() - Universe::heap()->unused().

JVM_ENTRY_NO_ENV(jlong, JVM_TotalMemory(void))
  size_t n = Universe::heap()->capacity();
  return convert_size_t_to_jlong(n);
JVM_END

JVM_ENTRY_NO_ENV(jlong, JVM_FreeMemory(void))
  size_t n = Universe::heap()->unused();
  return convert_size_t_to_jlong(n);
JVM_END
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Sorry but I have to agree with Kim that this is the wrong fix for this problem.

And the test needs adjusting.

Thanks,
David

"UseG1GC",
"UseSerialGC",
"UseParallelGC",
"UseShenandoahGC",
"UseZGC",
"UseEpsilonGC",
Comment on lines +39 to +44

This comment has been minimized.

@dholmes-ora

dholmes-ora Apr 6, 2021
Member

You have to allow for the fact that these GC's may not have all been built into the JVM under test.

// used() calls G1Allocator::used_in_alloc_regions() when G1 enabled,
// it checks whether Heap_lock was owned on this thread's behalf.
G1GC_ONLY(MutexLocker ml(Heap_lock);)
Comment on lines +118 to +120

This comment has been minimized.

@dholmes-ora

dholmes-ora Apr 6, 2021
Member

This seems really out of place - this code should not need to know anything about the specific locking requirements of different GC's. That should be handled inside the appropriate chunk of GC code.

@coleenp
Copy link

@coleenp coleenp commented Apr 6, 2021

Unless we can imagine a reason why this code is needed, I think the old memprofiler should be removed.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 7, 2021

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

On 7/04/2021 12:34 am, Coleen Phillimore wrote:

On Tue, 6 Apr 2021 02:04:57 GMT, David Holmes <dholmes at openjdk.org> wrote:

Trivial fix for JDK-8264682.

`Universe::heap()->used()` calls G1Allocator::used_in_alloc_regions() when G1 enabled, it checks whether Heap_lock was owned on this thread's behalf.

Sorry but I have to agree with Kim that this is the wrong fix for this problem.

And the test needs adjusting.

Thanks,
David

Unless we can imagine a reason why this code is needed, I think the old memprofiler should be removed.

Why was it added? I've no idea who might be using this, or for what. But
I think removing it is a separate issue.

Cheers,
David

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 7, 2021

I'd like to propose a new PR to remove the memprofiler if there are no strong objections.

@coleenp
Copy link

@coleenp coleenp commented Apr 7, 2021

Sure, open a new issue if you want, and link this issue to it.
The "feature" was added here:

^As 00018/00000/00000
^Ad D 1.1 98/04/14 13:18:32 renes 1 0
^Ac date and time created 98/04/14 13:18:32 by renes
^Ae

@kelthuzadx kelthuzadx closed this Apr 9, 2021
@kelthuzadx kelthuzadx deleted the kelthuzadx:memprofile_crash branch Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants