RFR: 8264682: MemProfiling does not own Heap_lock when using G1

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264682: MemProfiling does not own Heap_lock when using G1

Yi Yang
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.

-------------

Commit messages:
 - memprofiling_crash

Changes: https://git.openjdk.java.net/jdk/pull/3340/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3340&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264682
  Stats: 59 lines in 2 files changed: 59 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3340.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3340/head:pull/3340

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

Kim Barrett-2
On Mon, 5 Apr 2021 07:34:30 GMT, Yi Yang <[hidden email]> wrote:

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

Ioi Lam-2
On Mon, 5 Apr 2021 14:29:11 GMT, Kim Barrett <[hidden email]> wrote:

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

David Holmes-2
In reply to this post by Yi Yang
On Mon, 5 Apr 2021 07:34:30 GMT, Yi Yang <[hidden email]> 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

src/hotspot/share/runtime/memprofiler.cpp line 120:

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

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.

test/hotspot/jtreg/runtime/MemProfiler/MemProfilingWithGC.java line 44:

> 42:             "UseShenandoahGC",
> 43:             "UseZGC",
> 44:             "UseEpsilonGC",

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

-------------

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

Coleen Phillimore-3
On Tue, 6 Apr 2021 02:04:57 GMT, David Holmes <[hidden email]> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

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

> On Tue, 6 Apr 2021 02:04:57 GMT, David Holmes <[hidden email]> 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

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3340
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

Yi Yang
In reply to this post by Coleen Phillimore-3
On Tue, 6 Apr 2021 14:31:00 GMT, Coleen Phillimore <[hidden email]> wrote:

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

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264682: MemProfiling does not own Heap_lock when using G1

Coleen Phillimore-3
On Wed, 7 Apr 2021 09:33:39 GMT, Yi Yang <[hidden email]> wrote:

>> Unless we can imagine a reason why this code is needed, I think the old memprofiler should be removed.
>
> I'd like to propose a new PR to remove the memprofiler if there are no strong objections.

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/3340
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8264682: MemProfiling does not own Heap_lock when using G1

Yi Yang
In reply to this post by Yi Yang
On Mon, 5 Apr 2021 07:34:30 GMT, Yi Yang <[hidden email]> 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.

This pull request has been closed without being integrated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3340