Re: JDK-8171119: Low-Overhead Heap Profiling

[JC Beyler]

Hi Robbin,

So I did the changes to move most of the code into the AllocTracer and
you can see it incrementally here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.04_05/
And the full webrev here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05/

Now the issues I see here:

- AllocTracer now does "things" instead of just having a send_*_event
API, that is a change in concept for that class
- Collectedheap still needs to call AllocTracer to see if it is to be
sampled, I can't hide everything in it without a bigger refactor (want
me to try?)
- The ordering is now important: AllocTracer has to get called before
tlab().fill
   - Otherwise the fact that the allocation has to get sampled will
get lost when the tlab gets replaced

If this still looks better to you or in a better direction, let me
know. I can do the end part of it and add an event for a sample since
now that is easy and makes sense to probably add such an event, and I
can add a few more tests.

Thanks!

On Mon, Jan 29, 2018 at 1:29 AM, Robbin Ehn  wrote:
> Hi JC, thanks!
>
> I'm happy with current state, looks good!
>
> Truncated:
>
> On 01/27/2018 05:01 AM, JC Beyler wrote:
>>
>> This is strange but I'm assuming it is because we are not working on
>> the same repo?
>>
>> I used:
>> hg clone http://hg.openjdk.java.net/jdk/hs jdkhs-heap
>>
>> I'll try a new clone on Monday and see. My new version moved hard_end
>> back to public so it should work now.
>
>
> Sorry this compile error was in closed code.
> Now the closed part compiles, thanks.
>
>>
>> Fair enough, hopefully Thomas will chime in. Are you saying that this
>> first version could go in and we can work on a refinement? Or are you
>> saying I should work on this now at the same time and fix it before
>> this V1 goes in? (Just so I know :))
>
>
> We may have to change this before integration, but for now keep it as is.
>
>> I'll look at this on Monday then!
>
>
> Great!
>
> /Robbin
>
>
>>
>> Thanks for the reply and have a great weekend!
>> Jc
>>
>>>

> 
> Minor nit, when declaring pointer there is a little mix of having the
> pointer adjacent by type name and data name. (Most hotspot code is by
> type
> name)
> E.g.
> heapMonitoring.cpp:711 jvmtiStackTrace *trace = 
> heapMonitoring.cpp:733 Method* m = vfst.method();
> (not just this file)
>

 Done!

> 
> HeapMonitorThreadOnOffTest.java:77
> I would make g_tmp volatile, otherwise the assignment in loop may
> theoretical be skipped.
>

 Also done!
>>>
>>>
>>>
>>> Looks good, thanks!
>>>
>>> /Robbin
>>>

 Thanks again!
 Jc

>>>
>

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

That’s one reviewer who’s ok with a short term patch. Anyone else? And, any 
reviewers for said short term patch? :)

Thanks,

Paul

From: mandy chung 
Organization: Oracle Corporation
Date: Monday, January 29, 2018 at 1:41 PM
To: "Hohensee, Paul" 
Cc: "serviceability-dev@openjdk.java.net" 
, "hotspot-gc-...@openjdk.java.net" 

Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results

I created  JDK-8196362 to look into whether it makes sense to provide some 
categorization to differentiate eden space vs the heap space for long-lived 
objects.

W.r.t. to JDK-8195115, I have to defer to GC team to comment on the mixed 
collection update.  If they are okay, I have no objection to implement a 
short-term fix and do the proper G1 memory pools as a separate patch.

Mandy
On 1/29/18 1:02 PM, Hohensee, Paul wrote:
We don’t use getType, and you guessed correctly: we use the memory pool name as 
an indicator of the specific characteristics of a memory pool, in particular 
eden.

What we want is an indication of long term heap occupancy. We calculate it 
using CollectionUsage for non-eden heap memory pools, regardless of collector. 
We don’t use JMX notification, rather we periodically poll CollectionUsage for 
memory pools with names that contain “Old”, “Tenured”, or “Survivor”. We get 
the memory pools from the GarbageCollectorMXBeans (we don’t care what the 
collector names are). For the named memory pools, we sum CollectionUsage.used 
and divide by the sum of CollectionUsage.max to get a long term heap occupancy 
percentage. We don’t want to include eden because it’s really just an 
allocation buffer and not part of the storage for long-lived objects. I suppose 
we could use a negative test instead by using all memory pools with names that 
don’t include “Eden”.

The bug is that the “G1 Old Gen” memory pool isn’t being updated when the “G1 
Young Generation” collector runs a mixed collection. As far as JMX is 
concerned, that collector only knows about eden and the survivor space. The 
patch adds the old gen to the memory pools it knows about and has mixed 
collections update the old gen’s CollectionUsage.

I managed to get a submit repo run to succeed last week and it found a problem. 
I’ve uploaded a new webrev that fixes the failure of the jtreg test 
TestMemoryMXBeansAndPoolsPresence.java due to the young gen collector being 
expected to know only about eden and the survivor space.

http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/

Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

From: mandy chung 
Organization: Oracle Corporation
Date: Monday, January 29, 2018 at 10:52 AM
To: "Hohensee, Paul" , Erik 
Helin , David Holmes 

Cc: 
"serviceability-dev@openjdk.java.net"
 
,
 "hotspot-gc-...@openjdk.java.net" 

Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/29/18 10:35 AM, mandy chung wrote:
Thanks for the reply Paul.   Try to understand a little more on the specific 
from gc-specific memory pool you depend on.
On 1/29/18 8:27 AM, Hohensee, Paul wrote:
A name change would affect Amazon’s heap monitoring, and thus I expect it would 
affect other users as well.

As long as there are gc-specific memory pools, we’re going to need to be able 
to identify them, and right now that’s done via name.

MemoryPoolMXBean::getType returns "heap" memory type for GC-specific memory 
pools.  Are you using this method?  Do you use the name to build in specific 
characteristic of a memory pool (e.g. eden vs old gen)?




All the mxbeans are identified by name, so that’s a general design principle. 
The only way I can think of to get rid of name dependency would be to figure 
out what abstract metrics users want to monitor and implement them for all 
collectors. HeapUsage (instantaneous occupancy) is one, CollectionUsage 
(long-lived occupancy) is another, both of these for the entire heap, not just 
particular memory pools.

The sum of HeapUsage and CollectionUsage of all heap memory pools was expected 
to give an incorrect approximation for the entire heap usage.  Are you seeing 
issue/bug with the sum result?

typo: s/an incorrect approximation/an approximation.

Mandy



Mandy



That said, imo there will always be a demand for the ability to get collector 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Kirk Pepperdine]

> On Jan 29, 2018, at 5:27 PM, Hohensee, Paul  wrote:
> 
> A name change would affect Amazon’s heap monitoring, and thus I expect it 
> would affect other users as well.

I can name a number of tools that would be disrupted by this type of change. 
Additionally tooling would be complicated by the need to support both the old 
and new versions. The current names have been with us for years and they are 
well known, well documented and well understood. Given the level of disruption 
this change is likely to cause IMHO you’d need a very very good reason to want 
to make it.
>  
> As long as there are gc-specific memory pools, we’re going to need to be able 
> to identify them, and right now that’s done via name. All the mxbeans are 
> identified by name, so that’s a general design principle. The only way I can 
> think of to get rid of name dependency would be to figure out what abstract 
> metrics users want to monitor and implement them for all collectors. 
> HeapUsage (instantaneous occupancy) is one, CollectionUsage (long-lived 
> occupancy) is another, both of these for the entire heap, not just particular 
> memory pools. That said, imo there will always be a demand for the ability to 
> get collector and memory pool specific details, so I don’t see a way to get 
> around providing named entities.

Agreed…tuning strategies are implementation dependent and sensitive to specific 
versions. One is always going to need to know this information.

Kind regards,
Kirk Pepperdine

Re: RFR: 8196361: JTReg failure in serviceability/sa/ClhsdbInspect.java

[David Holmes]

Hi Daniel,

Serviceability issues should go to serviceability-dev@openjdk.java.net - 
now cc'd.


On 30/01/2018 7:53 AM, stewartd.qdt wrote:

Please review this webrev [1] which attempts to fix a test error in serviceability/sa/ClhsdbInspect.java when 
it is run under an AArch64 system (not necessarily exclusive to this system, but it was the system under 
test). The bug report [2] provides further details. Essentially the line "waiting to re-lock in 
wait" never actually occurs. Instead I have the line "waiting to lock" which occurs for the 
referenced item of /java/lang/ref/ReferenceQueue$Lock. Unfortunately the test is written such that only the 
first "waiting to lock" occurrence is seen (for java/lang/Class), which is already accounted for in 
the test.


I can't tell exactly what the test expects, or why, but it would be 
extremely hard to arrange for "waiting to re-lock in wait" to be seen 
for the ReferenceQueue lock! That requires acquiring the lock yourself, 
issuing a notify() to unblock the wait(), and then issuing the jstack 
command while still holding the lock!


David
-


I'm not overly happy with this approach as it actually removes a test line. However, the 
test line does not actually appear in the output (at least on my system) and the test is 
not currently written to look for the second occurrence of the line "waiting to 
lock". Perhaps the original author could chime in and provide further guidance as to 
the intention of the test.

I am happy to modify the patch as necessary.

Regards,
Daniel Stewart


[1] -  http://cr.openjdk.java.net/~dstewart/8196361/webrev.00/
[2] - https://bugs.openjdk.java.net/browse/JDK-8196361

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

I created  JDK-8196362 to look into whether it makes sense to provide 
some categorization to differentiate eden space vs the heap space for 
long-lived objects.


W.r.t. to JDK-8195115, I have to defer to GC team to comment on the 
mixed collection update.  If they are okay, I have no objection to 
implement a short-term fix and do the proper G1 memory pools as a 
separate patch.


Mandy

On 1/29/18 1:02 PM, Hohensee, Paul wrote:


We don’t use getType, and you guessed correctly: we use the memory 
pool name as an indicator of the specific characteristics of a memory 
pool, in particular eden.


What we want is an indication of long term heap occupancy. We 
calculate it using CollectionUsage for non-eden heap memory pools, 
regardless of collector. We don’t use JMX notification, rather we 
periodically poll CollectionUsage for memory pools with names that 
contain “Old”, “Tenured”, or “Survivor”. We get the memory pools from 
the GarbageCollectorMXBeans (we don’t care what the collector names 
are). For the named memory pools, we sum CollectionUsage.used and 
divide by the sum of CollectionUsage.max to get a long term heap 
occupancy percentage. We don’t want to include eden because it’s 
really just an allocation buffer and not part of the storage for 
long-lived objects. I suppose we could use a negative test instead by 
using all memory pools with names that don’t include “Eden”.


The bug is that the “G1 Old Gen” memory pool isn’t being updated when 
the “G1 Young Generation” collector runs a mixed collection. As far as 
JMX is concerned, that collector only knows about eden and the 
survivor space. The patch adds the old gen to the memory pools it 
knows about and has mixed collections update the old gen’s 
CollectionUsage.


I managed to get a submit repo run to succeed last week and it found a 
problem. I’ve uploaded a new webrev that fixes the failure of the 
jtreg test TestMemoryMXBeansAndPoolsPresence.java due to the young gen 
collector being expected to know only about eden and the survivor space.


http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/ 



Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

*From: *mandy chung 
*Organization: *Oracle Corporation
*Date: *Monday, January 29, 2018 at 10:52 AM
*To: *"Hohensee, Paul" , Erik Helin 
, David Holmes 
*Cc: *"serviceability-dev@openjdk.java.net" 
, 
"hotspot-gc-...@openjdk.java.net" 
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


On 1/29/18 10:35 AM, mandy chung wrote:

Thanks for the reply Paul.   Try to understand a little more on
the specific from gc-specific memory pool you depend on.

On 1/29/18 8:27 AM, Hohensee, Paul wrote:

A name change would affect Amazon’s heap monitoring, and thus
I expect it would affect other users as well.

As long as there are gc-specific memory pools, we’re going to
need to be able to identify them, and right now that’s done
via name.


MemoryPoolMXBean::getType returns "heap" memory type for
GC-specific memory pools.  Are you using this method?  Do you use
the name to build in specific characteristic of a memory pool
(e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general
design principle. The only way I can think of to get rid of
name dependency would be to figure out what abstract metrics
users want to monitor and implement them for all collectors.
HeapUsage (instantaneous occupancy) is one, CollectionUsage
(long-lived occupancy) is another, both of these for the
entire heap, not just particular memory pools.


The sum of HeapUsage and CollectionUsage of all heap memory pools
was expected to give an incorrect approximation for the entire
heap usage.  Are you seeing issue/bug with the sum result?


typo: s/an incorrect approximation/an approximation.

Mandy


Mandy


That said, imo there will always be a demand for the ability
to get collector and memory pool specific details, so I don’t
see a way to get around providing named entities.

Paul

*From: *mandy chung 

*Organization: *Oracle Corporation
*Date: *Friday, January 26, 2018 at 2:38 PM
*To: *"Hohensee, Paul" 
, Erik Helin
 , David
Holmes  
*Cc: *"serviceability-dev@openjdk.java.net"

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[David Holmes]

On the CSR question, yes this would need a CSR just to ensure the 
compatibility issues have been covered.


David

On 25/01/2018 11:20 PM, Erik Helin wrote:

Hi Paul,

thanks for your interest in this area and for your patch! The 
GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of 
some updates, so thanks for working on this.


Looking at your patch, I'm not sure that this is the direction we want 
to go in. I discussed this a bit with Thomas and Stefan J, and our 
current line of thinking is the following:


- Memory pools (MemoryMXBean):
   - "G1 Eden Regions"
   - "G1 Survivor Regions"
   - "G1 Old Regions"
   - "G1 Humongous Regions"
   - "G1 Archive Regions" (if CDS and/or AppCDS is used)

`init` for these pools would be 0, `used` would be total size of the 
"live" objects in the used regions of that type, `committed` the total 
size of the used regions of that that type and `max` would be 
MaxHeapSize. Note that "live" here means live from the GCs point of 
view, i.e. an object might be dead in an old region but the GC will 
consider that object live until a concurrent cycle has marked through 
the heap and deemed it dead.


- Collectors (GarbageCollectorMXBean):
   - "G1 Young Collector" with the pools
     - "G1 Eden Regions"
     - "G1 Survivor Regions"
     - "G1 Humongous Regions" (due to early reclamation)
   - "G1 Mixed Collector" with the pools
     - "G1 Eden Regions"
     - "G1 Survivor Regions"
     - "G1 Old Regions"
     - "G1 Humongous Regions" (due to early reclamation)
   - "G1 Full Collector" with the pools
     - "G1 Eden Regions"
     - "G1 Survivor Regions"
     - "G1 Old Regions"
     - "G1 Humongous Regions" (can collect empty humongous regions)
   - "G1 Concurrent Cycle" with the pools
     - "G1 Old Regions" (can collect empty old regions)
     - "G1 Humongous Regions" (can collect empty humongous regions)

Note that with this design, the 
GarbageCollectorMXBean::getCollectionTime() method for "G1 Concurrent 
Cycle" would be the wall clock time from start of the first initial mark 
to end of the last cleanup (also including the time of any eventual 
young collection during the concurrent cycle). So 
GarbageCollectorMXBean::getCollectionTime() would be a mix of concurrent 
and STW time for the GarbageCollectorMXBean with name "G1 Concurrent 
Cycle".


Also note that the MemoryPoolMXBean with name "G1 Archive Regions" will 
not be attached to any GarbageCollectorMXBean, since those regions will 
never be collected.


What do you think about this design, would it work for your use case?

If we want to go ahead with this design, then I think we might have to 
file a CSR. David (who is the HotSpot CSR representative), do we have to 
file a CSR for changing the names of MemoryPoolMXBeans and 
GarbageCollectorMXBeans?


Thanks,
Erik

On 01/20/2018 12:40 AM, Hohensee, Paul wrote:

I’d appreciate a review please.

Bug: https://bugs.openjdk.java.net/browse/JDK-8195115

Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.00/

The bug is that from the JMX point of view, G1’s incremental collector 
(misnamed as the “G1 Young Generation” collector) only affects G1’s 
survivor and eden spaces. In fact, mixed collections run by this 
collector also affect the G1 old generation.


This proposed fix is to record, for each of a JMX garbage collector's 
memory pools, whether that memory pool is affected by all collections 
using that collector. And, for each collection, record whether or not 
all the collector's memory pools are affected. After each collection, 
for each memory pool, if either all the collector's memory pools were 
affected or the memory pool is affected for all collections, record 
CollectionUsage for that pool.


For collectors other than G1 Young Generation, all pools are recorded 
as affected by all collections and every collection is recorded as 
affecting all the collector’s memory pools. For the G1 Young 
Generation collector, the G1 Old Gen pool is recorded as not being 
affected by all collections, and non-mixed collections are recorded as 
not affecting all memory pools. The result is that for non-mixed 
collections, CollectionUsage is recorded after a collection only the 
G1 Eden Space and G1 Survivor Space pools, while for mixed collections 
CollectionUsage is recorded for G1 Old Gen as well.


Other than the effect of the fix on G1 Old Gen MemoryPool. 
CollectionUsage, the only external behavior change is that 
GarbageCollectorMXBean.getMemoryPoolNames will now return 3 pool names 
rather than 2.


With this fix, a collector’s memory pools can be divided into two 
disjoint subsets, one that participates in all collections and one 
that doesn’t. This is a bit more general than the minimum necessary to 
fix G1, but not by much. Because I expect it to apply to other 
incremental region-based collectors, I went with the more general 
solution. I minimized the amount of code I had to touch by using 
default parameters for 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

We don’t use getType, and you guessed correctly: we use the memory pool name as 
an indicator of the specific characteristics of a memory pool, in particular 
eden.

What we want is an indication of long term heap occupancy. We calculate it 
using CollectionUsage for non-eden heap memory pools, regardless of collector. 
We don’t use JMX notification, rather we periodically poll CollectionUsage for 
memory pools with names that contain “Old”, “Tenured”, or “Survivor”. We get 
the memory pools from the GarbageCollectorMXBeans (we don’t care what the 
collector names are). For the named memory pools, we sum CollectionUsage.used 
and divide by the sum of CollectionUsage.max to get a long term heap occupancy 
percentage. We don’t want to include eden because it’s really just an 
allocation buffer and not part of the storage for long-lived objects. I suppose 
we could use a negative test instead by using all memory pools with names that 
don’t include “Eden”.

The bug is that the “G1 Old Gen” memory pool isn’t being updated when the “G1 
Young Generation” collector runs a mixed collection. As far as JMX is 
concerned, that collector only knows about eden and the survivor space. The 
patch adds the old gen to the memory pools it knows about and has mixed 
collections update the old gen’s CollectionUsage.

I managed to get a submit repo run to succeed last week and it found a problem. 
I’ve uploaded a new webrev that fixes the failure of the jtreg test 
TestMemoryMXBeansAndPoolsPresence.java due to the young gen collector being 
expected to know only about eden and the survivor space.

http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/

Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

From: mandy chung 
Organization: Oracle Corporation
Date: Monday, January 29, 2018 at 10:52 AM
To: "Hohensee, Paul" , Erik Helin , 
David Holmes 
Cc: "serviceability-dev@openjdk.java.net" 
, "hotspot-gc-...@openjdk.java.net" 

Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/29/18 10:35 AM, mandy chung wrote:
Thanks for the reply Paul.   Try to understand a little more on the specific 
from gc-specific memory pool you depend on.
On 1/29/18 8:27 AM, Hohensee, Paul wrote:
A name change would affect Amazon’s heap monitoring, and thus I expect it would 
affect other users as well.

As long as there are gc-specific memory pools, we’re going to need to be able 
to identify them, and right now that’s done via name.

MemoryPoolMXBean::getType returns "heap" memory type for GC-specific memory 
pools.  Are you using this method?  Do you use the name to build in specific 
characteristic of a memory pool (e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general design principle. 
The only way I can think of to get rid of name dependency would be to figure 
out what abstract metrics users want to monitor and implement them for all 
collectors. HeapUsage (instantaneous occupancy) is one, CollectionUsage 
(long-lived occupancy) is another, both of these for the entire heap, not just 
particular memory pools.

The sum of HeapUsage and CollectionUsage of all heap memory pools was expected 
to give an incorrect approximation for the entire heap usage.  Are you seeing 
issue/bug with the sum result?

typo: s/an incorrect approximation/an approximation.

Mandy


Mandy


That said, imo there will always be a demand for the ability to get collector 
and memory pool specific details, so I don’t see a way to get around providing 
named entities.

Paul

From: mandy chung 
Organization: Oracle Corporation
Date: Friday, January 26, 2018 at 2:38 PM
To: "Hohensee, Paul" , Erik 
Helin , David Holmes 

Cc: 
"serviceability-dev@openjdk.java.net"
 
,
 "hotspot-gc-...@openjdk.java.net" 

Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:

> The JMX API spec doesn’t specify what the memory pool or garbage > collector 
> names are, but the current names are de-facto part of the > API, so if we 
> change the existing ones, imo a CSR should be filed.

The names are implementation details but I can see how an application

might be impacted if they depend on it.  CSR approval is not strictly

necessary while I think filing one to 

Re: PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

[David Holmes]

Added in hotspot-gc-dev. Although this is in the SA it is about the SA 
interaction with G1 and so likely needs someone familiar with G1 to 
review it.


David

On 28/01/2018 10:41 PM, Yasumasa Suenaga wrote:

PING: Could you review it?


   http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/


This webrev has been reviewed by Jini.
I need a Reviewer and sponsor.


Yasumasa


On 2018/01/22 19:53, Yasumasa Suenaga wrote:

Hi Jini,

Thank you for your review!
I will update the copyright year in this changeset.

I'm waiting for Reviewer and sponsor.


Yasumasa


On 2018/01/22 13:14, Jini George wrote:

Hi Yasumasa,

The changes look good to me. Please do update the copyright year to 
2018.


Thanks!
Jini (Not a Reviewer).



On 12/31/2017 10:03 AM, Yasumasa Suenaga wrote:

Hi David,



How did you submit to mach5 ???


I'm using Submit Repo for testing:
   https://wiki.openjdk.java.net/display/Build/Submit+Repo



Anyway the failure is with:


Thanks!
I've fixed them in new webrev:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/

This webrev has passed Mach 5 tier 1 tests in Submit Repo:
http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171231-0202-8291 




Yasumasa


On 2017/12/30 10:31, David Holmes wrote:

Hi Yasumasa,

Not a review ...

On 29/12/2017 11:16 PM, Yasumasa Suenaga wrote:

Hi all,

G1HeapRegionTable#getByAddress() returns incorrect HeapRegion 
which contains incorrect address. We can see it in Stack Memory 
window on HSDB. Some oop addresses are shown as Free Region 
(attached image).


G1HeapRegion#getByAddress() should create HeapRegion instance from 
the address in _biased_base array.


I uploaded webrev. Could you review it?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.00/

I've tested this change with test/hotspot/jtreg/serviceability/sa, 
it works fine.
But I received some failure from Mach 5. I also tested this change 
via submit repos.


http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8194249-20171228-0605-8272 



I cannot access this URL. Could you share the result?


How did you submit to mach5 ???

Anyway the failure is with:

test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java

On linux and OS X:

  stderr: [Exception in thread "main" java.lang.NullPointerException
 at 
TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70) 

 at 
jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121) 


 at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
 at TestG1HeapRegion.main(TestG1HeapRegion.java:129)

On Solaris sparcv9:

  stderr: [Exception in thread "main" java.lang.RuntimeException: 
Address of HeapRegion does not match.: expected 0x0007afb0 
to equal 0x0007afc0

 at jdk.test.lib.Asserts.fail(Asserts.java:594)
 at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
 at 
TestG1HeapRegion$G1HeapRegionTestClosure.doSpace(TestG1HeapRegion.java:70) 

 at 
jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.G1CollectedHeap.heapRegionIterate(G1CollectedHeap.java:121) 


 at TestG1HeapRegion.scanHeapRegion(TestG1HeapRegion.java:81)
 at TestG1HeapRegion.main(TestG1HeapRegion.java:129)
]

David
-


Also I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

On 1/29/18 10:35 AM, mandy chung wrote:
Thanks for the reply Paul.   Try to understand a little more on the 
specific from gc-specific memory pool you depend on.


On 1/29/18 8:27 AM, Hohensee, Paul wrote:


A name change would affect Amazon’s heap monitoring, and thus I 
expect it would affect other users as well.


As long as there are gc-specific memory pools, we’re going to need to 
be able to identify them, and right now that’s done via name.




MemoryPoolMXBean::getType returns "heap" memory type for GC-specific 
memory pools.  Are you using this method?  Do you use the name to 
build in specific characteristic of a memory pool (e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general design 
principle. The only way I can think of to get rid of name dependency 
would be to figure out what abstract metrics users want to monitor 
and implement them for all collectors. HeapUsage (instantaneous 
occupancy) is one, CollectionUsage (long-lived occupancy) is another, 
both of these for the entire heap, not just particular memory pools.




The sum of HeapUsage and CollectionUsage of all heap memory pools was 
expected to give an incorrect approximation for the entire heap 
usage.  Are you seeing issue/bug with the sum result?




typo: s/an incorrect approximation/an approximation.

Mandy


Mandy

That said, imo there will always be a demand for the ability to get 
collector and memory pool specific details, so I don’t see a way to 
get around providing named entities.


Paul

*From: *mandy chung 
*Organization: *Oracle Corporation
*Date: *Friday, January 26, 2018 at 2:38 PM
*To: *"Hohensee, Paul" , Erik Helin 
, David Holmes 
*Cc: *"serviceability-dev@openjdk.java.net" 
, 
"hotspot-gc-...@openjdk.java.net" 
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:


> The JMX API spec doesn’t specify what the memory pool or garbage > 
collector names are, but the current names are de-facto part of the > 
API, so if we change the existing ones, imo a CSR should be filed.


The names are implementation details but I can see how an application
might be impacted if they depend on it.  CSR approval is not strictly
necessary while I think filing one to document the change would be
good.

Does the name change impact any application you know of?  I'm trying to
understand if any improvement to API is needed so that applications
don't need to depend on the names.


Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

Thanks for the reply Paul.   Try to understand a little more on the 
specific from gc-specific memory pool you depend on.


On 1/29/18 8:27 AM, Hohensee, Paul wrote:


A name change would affect Amazon’s heap monitoring, and thus I expect 
it would affect other users as well.


As long as there are gc-specific memory pools, we’re going to need to 
be able to identify them, and right now that’s done via name.




MemoryPoolMXBean::getType returns "heap" memory type for GC-specific 
memory pools.  Are you using this method?  Do you use the name to build 
in specific characteristic of a memory pool (e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general design 
principle. The only way I can think of to get rid of name dependency 
would be to figure out what abstract metrics users want to monitor and 
implement them for all collectors. HeapUsage (instantaneous occupancy) 
is one, CollectionUsage (long-lived occupancy) is another, both of 
these for the entire heap, not just particular memory pools.




The sum of HeapUsage and CollectionUsage of all heap memory pools was 
expected to give an incorrect approximation for the entire heap usage.  
Are you seeing issue/bug with the sum result?


Mandy

That said, imo there will always be a demand for the ability to get 
collector and memory pool specific details, so I don’t see a way to 
get around providing named entities.


Paul

*From: *mandy chung 
*Organization: *Oracle Corporation
*Date: *Friday, January 26, 2018 at 2:38 PM
*To: *"Hohensee, Paul" , Erik Helin 
, David Holmes 
*Cc: *"serviceability-dev@openjdk.java.net" 
, 
"hotspot-gc-...@openjdk.java.net" 
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:


> The JMX API spec doesn’t specify what the memory pool or garbage > 
collector names are, but the current names are de-facto part of the > 
API, so if we change the existing ones, imo a CSR should be filed.


The names are implementation details but I can see how an application
might be impacted if they depend on it.  CSR approval is not strictly
necessary while I think filing one to document the change would be
good.

Does the name change impact any application you know of?  I'm trying to
understand if any improvement to API is needed so that applications
don't need to depend on the names.


Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

A name change would affect Amazon’s heap monitoring, and thus I expect it would 
affect other users as well.

As long as there are gc-specific memory pools, we’re going to need to be able 
to identify them, and right now that’s done via name. All the mxbeans are 
identified by name, so that’s a general design principle. The only way I can 
think of to get rid of name dependency would be to figure out what abstract 
metrics users want to monitor and implement them for all collectors. HeapUsage 
(instantaneous occupancy) is one, CollectionUsage (long-lived occupancy) is 
another, both of these for the entire heap, not just particular memory pools. 
That said, imo there will always be a demand for the ability to get collector 
and memory pool specific details, so I don’t see a way to get around providing 
named entities.

Paul

From: mandy chung 
Organization: Oracle Corporation
Date: Friday, January 26, 2018 at 2:38 PM
To: "Hohensee, Paul" , Erik Helin , 
David Holmes 
Cc: "serviceability-dev@openjdk.java.net" 
, "hotspot-gc-...@openjdk.java.net" 

Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:

> The JMX API spec doesn’t specify what the memory pool or garbage > collector 
> names are, but the current names are de-facto part of the > API, so if we 
> change the existing ones, imo a CSR should be filed.

The names are implementation details but I can see how an application

might be impacted if they depend on it.  CSR approval is not strictly

necessary while I think filing one to document the change would be

good.


Does the name change impact any application you know of?  I'm trying to

understand if any improvement to API is needed so that applications

don't need to depend on the names.

Mandy

[8u] RFR for 'JDK-8195088: [TEST_BUG] StartManagementAgent got unexpected exception'

[Shafi Ahmad]

Hi,

Please review the trivial code change for the fix of 'JDK-8195088: [TEST_BUG] 
StartManagementAgent got unexpected exception' to jdk8u-dev.

Summary:
This test case is expected to fail but the expected exception message is 
different.
This fix 
-if (!ex.getMessage().contains("Invalid 
com.sun.management.jmxremote.port number")) {
+if (!ex.getMessage().contains("NumberFormatException: For input 
string: \"apa\"")) {

is checking for the correct message.

This is fixed in jdk10 under bug JDK-8165736 [Error message should be shown 
when JVMTI agent cannot be attache] 
http://hg.openjdk.java.net/jdk/hs/rev/bc1cffa26561#l10.1

Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8195088
webrev link: http://cr.openjdk.java.net/~shshahma/8195088/webrev.00/

Regards,
Shafi

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

[Harsha Wardhana B]

Hi Alan,

I am not fully aware about Java launcher or how it passes options to VM. 
Let me check with some other folks and get back to you.


Thanks
Harsha

On Monday 29 January 2018 01:55 PM, Alan Bateman wrote:



On 29/01/2018 05:20, Harsha Wardhana B wrote:

Hi Mandy,Alan,

Thanks for your inputs.
If I keep it as launcher option, it may need to know JMX agent flags 
which may need to be extended in future.
I would prefer making it a VM option. I will make the required 
changes and send out an updated webrev.
I think Mandy's suggestion is to just transform --management  
so a form that the VM can read. The launcher will need to replace the 
space anyway as the VM only accepts "=".


-Alan

Re: JDK-8171119: Low-Overhead Heap Profiling

[Robbin Ehn]

Hi JC, thanks!

I'm happy with current state, looks good!

Truncated:

On 01/27/2018 05:01 AM, JC Beyler wrote:

This is strange but I'm assuming it is because we are not working on
the same repo?

I used:
hg clone http://hg.openjdk.java.net/jdk/hs jdkhs-heap

I'll try a new clone on Monday and see. My new version moved hard_end
back to public so it should work now.


Sorry this compile error was in closed code.
Now the closed part compiles, thanks.



Fair enough, hopefully Thomas will chime in. Are you saying that this
first version could go in and we can work on a refinement? Or are you
saying I should work on this now at the same time and fix it before
this V1 goes in? (Just so I know :))


We may have to change this before integration, but for now keep it as is.


I'll look at this on Monday then!


Great!

/Robbin



Thanks for the reply and have a great weekend!
Jc







Minor nit, when declaring pointer there is a little mix of having the
pointer adjacent by type name and data name. (Most hotspot code is by
type
name)
E.g.
heapMonitoring.cpp:711 jvmtiStackTrace *trace = 
heapMonitoring.cpp:733 Method* m = vfst.method();
(not just this file)



Done!



HeapMonitorThreadOnOffTest.java:77
I would make g_tmp volatile, otherwise the assignment in loop may
theoretical be skipped.



Also done!



Looks good, thanks!

/Robbin



Thanks again!
Jc

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

[Alan Bateman]

On 29/01/2018 05:20, Harsha Wardhana B wrote:

Hi Mandy,Alan,

Thanks for your inputs.
If I keep it as launcher option, it may need to know JMX agent flags 
which may need to be extended in future.
I would prefer making it a VM option. I will make the required changes 
and send out an updated webrev.
I think Mandy's suggestion is to just transform --management  
so a form that the VM can read. The launcher will need to replace the 
space anyway as the VM only accepts "=".


-Alan