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

[Hohensee, Paul]

Thanks, very much appreciated.

Paul

On 2/12/18, 7:42 AM, "Erik Helin"  wrote:

Hi Paul,

sorry, I'm just back from a really busy week with two conferences. I 
will take a look tomorrow.

Thanks,
Erik

On 02/07/2018 11:17 PM, Hohensee, Paul wrote:
> I’ve filed https://bugs.openjdk.java.net/browse/JDK-8196989 : Revamp G1 
JMX MemoryPool and GarbageCollector MXBean definitions and the corresponding 
CSR https://bugs.openjdk.java.net/browse/JDK-8196991.
> 
> Would you please comment on the CSR, and on the original CSR 
https://bugs.openjdk.java.net/browse/JDK-8196719?
> 
> Thanks,
> 
> Paul
> 
> On 2/2/18, 1:20 PM, "hotspot-gc-dev on behalf of Hohensee, Paul" 
 
wrote:
> 
>  And, can a get a reviewer or reviewers for the CSR?
>  
>  Thanks,
>  
>  Paul
>  
>  On 2/2/18, 1:14 PM, "Hohensee, Paul"  wrote:
>  
>  +hotspot-gc-use.
>  
>  I’ve filed a CSR for the current patch, see 
https://bugs.openjdk.java.net/browse/JDK-8196719. Here’s the argument in favor.
>  
>  
>  It’s possible that there are JDK8 users that rely on current G1 
old gen CollectionUsage behavior, but imo it’s unlikely because it’s of little 
use. Perhaps Kirk and others with operational experience can weigh in.
>  
>  Let’s think about use cases. G1 full GC’s happen rarely and only 
under severe pressure, so when they do external reaction is pretty much limited 
to reducing load so the JVM can get back to a usable steady state, or just 
restarting the JVM. Old gen CollectionUsage is zero until a full GC occurs, 
after which its value includes both long-lived objects and any transient data 
that was in eden and the survivor space. That value doesn’t tell you anything 
about long term old gen occupancy or survivor size because it lumps them all 
together. So, it isn’t a useful metric, nor will it be after any subsequent 
full GCs. The only information it provides is on the first zero to non-zero 
transition, which just tells you that the JVM is/was in trouble. Further, the 
effect of the runup to a full GC is SLA violations, which are noticed before 
the full GC happens, so detecting the first full GC is confirmation, not 
prediction.
>  
>  Conclusion: G1 old gen CollectionUsage is unlikely to be in use 
in its current form, so changing its definition to something usable is low risk.
>  
>  
>  “G1 Old Space” is fine, as is “G1 Archive Space”. Are you ok 
with the G1 archive space overlapping the G1 old space? Should we add an 
archive space to the other collectors? If so, how would it be defined and would 
having it overlap with the old generation as a live prefix be ok?
>  
>  "G1 Young Generation" is the currently young+mixed collector.
>  
>  You’re right, if one is iterating over all collectors, there 
will be redundancy if we keep the old ones. I’m usually leery of introducing a 
flag such as UseG1LegacyMXBeans (I changed the name, since all the interfaces 
are MXBeans, hope that’s ok) which must be either indefinitely maintained, or 
go through a deprecation cycle. I don’t see a way out of the ‘iterate over all 
collectors’ problem without it though.
>  
>  Paul
>  
>  On 1/31/18, 3:42 AM, "Erik Helin"  wrote:
>  
>  On 01/31/2018 02:30 AM, Hohensee, Paul wrote:
>  > It’s true that my patch doesn’t completely solve the 
larger problem, but it fixes the most immediately important part of it, 
particularly for JDK8 where current expected behavior is entrenched.
>  
>  Yes, your patch fixes part of the problem, but as I said, can
>  potentially lead to more confusion. I'm not sure that doing 
this
>  behavioral change for a public API in an JDK 8 update 
release is the
>  right thing. There are likely users that rely on the memory 
pool "G1 Old
>  Gen" only being updated by a full collection (even though 
that behavior
>  is not correct), those uses will encounter a new behavior in 
an update
>  release with your patch.
>  
>  The good thing is that we have very experienced engineers 
participating
>  in the CSR process that have much more experience than I 
have in
>  evaluating the impact of behavioral changes such as this 
one. Would you
>  please file a CSR request for your patch to get their 
opinion?
>  
>  See 

RFR 8196324: Update FilterMatch & FilterNoMatch to use TestScaffold instead of JDIScaffold

[Paru Somashekar]

Hi,

Please review fix for JDK-8196324

The includes the following changes:

* Use startToMain method of TestScaffold that automatically calls 
connect() and waitForVMStart()
* Since TestScaffold extends TargetAdapter, it now overrides event 
callbacks directly in the test class rather than implement an anonymous 
inner class.
* Breakpoint handling is done in breakpointReached callback instead of 
waitForRequestedEvent and subsequently removing it.


Bug : https://bugs.openjdk.java.net/browse/JDK-8196324
Review : http://cr.openjdk.java.net/~psomashe/8196324/webrev/ 



thanks,
Paru.

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

[Erik Helin]

Hi Paul,

sorry, I'm just back from a really busy week with two conferences. I 
will take a look tomorrow.


Thanks,
Erik

On 02/07/2018 11:17 PM, Hohensee, Paul wrote:

I’ve filed https://bugs.openjdk.java.net/browse/JDK-8196989 : Revamp G1 JMX 
MemoryPool and GarbageCollector MXBean definitions and the corresponding CSR 
https://bugs.openjdk.java.net/browse/JDK-8196991.

Would you please comment on the CSR, and on the original CSR 
https://bugs.openjdk.java.net/browse/JDK-8196719?

Thanks,

Paul

On 2/2/18, 1:20 PM, "hotspot-gc-dev on behalf of Hohensee, Paul" 
 wrote:

 And, can a get a reviewer or reviewers for the CSR?
 
 Thanks,
 
 Paul
 
 On 2/2/18, 1:14 PM, "Hohensee, Paul"  wrote:
 
 +hotspot-gc-use.
 
 I’ve filed a CSR for the current patch, see https://bugs.openjdk.java.net/browse/JDK-8196719. Here’s the argument in favor.
 
 
 It’s possible that there are JDK8 users that rely on current G1 old gen CollectionUsage behavior, but imo it’s unlikely because it’s of little use. Perhaps Kirk and others with operational experience can weigh in.
 
 Let’s think about use cases. G1 full GC’s happen rarely and only under severe pressure, so when they do external reaction is pretty much limited to reducing load so the JVM can get back to a usable steady state, or just restarting the JVM. Old gen CollectionUsage is zero until a full GC occurs, after which its value includes both long-lived objects and any transient data that was in eden and the survivor space. That value doesn’t tell you anything about long term old gen occupancy or survivor size because it lumps them all together. So, it isn’t a useful metric, nor will it be after any subsequent full GCs. The only information it provides is on the first zero to non-zero transition, which just tells you that the JVM is/was in trouble. Further, the effect of the runup to a full GC is SLA violations, which are noticed before the full GC happens, so detecting the first full GC is confirmation, not prediction.
 
 Conclusion: G1 old gen CollectionUsage is unlikely to be in use in its current form, so changing its definition to something usable is low risk.
 
 
 “G1 Old Space” is fine, as is “G1 Archive Space”. Are you ok with the G1 archive space overlapping the G1 old space? Should we add an archive space to the other collectors? If so, how would it be defined and would having it overlap with the old generation as a live prefix be ok?
 
 "G1 Young Generation" is the currently young+mixed collector.
 
 You’re right, if one is iterating over all collectors, there will be redundancy if we keep the old ones. I’m usually leery of introducing a flag such as UseG1LegacyMXBeans (I changed the name, since all the interfaces are MXBeans, hope that’s ok) which must be either indefinitely maintained, or go through a deprecation cycle. I don’t see a way out of the ‘iterate over all collectors’ problem without it though.
 
 Paul
 
 On 1/31/18, 3:42 AM, "Erik Helin"  wrote:
 
 On 01/31/2018 02:30 AM, Hohensee, Paul wrote:

 > It’s true that my patch doesn’t completely solve the larger 
problem, but it fixes the most immediately important part of it, particularly for 
JDK8 where current expected behavior is entrenched.
 
 Yes, your patch fixes part of the problem, but as I said, can

 potentially lead to more confusion. I'm not sure that doing this
 behavioral change for a public API in an JDK 8 update release is 
the
 right thing. There are likely users that rely on the memory pool 
"G1 Old
 Gen" only being updated by a full collection (even though that 
behavior
 is not correct), those uses will encounter a new behavior in an 
update
 release with your patch.
 
 The good thing is that we have very experienced engineers participating

 in the CSR process that have much more experience than I have in
 evaluating the impact of behavioral changes such as this one. 
Would you
 please file a CSR request for your patch to get their opinion?
 
 See https://wiki.openjdk.java.net/display/csr/Main for more details

 about CSR.
 
 On 01/31/2018 02:30 AM, Hohensee, Paul wrote:

 If we’re going to fix the larger problem, imo we should file 
another
 bug/rfe to do it. I’d be happy to fix that one too, once we have a 
spec.
 >
 > What do you think of my suggestions? To summarize:
 >
 > - Keep the existing memory pools and add humongous and archive 
pools.
 

Re: JDK-8171119: Low-Overhead Heap Profiling

[Erik Österlund]

Hi JC,

Sorry for the delayed reply.

Inlined answers:

On 2018-02-06 00:04, JC Beyler wrote:

Hi Erik,

(Renaming this to be folded into the newly renamed thread :))

First off, thanks a lot for reviewing the webrev! I appreciate it!

I updated the webrev to:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05a/

And the incremental one is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.04_05a/

It contains:
- The change for since from 9 to 11 for the jvmti.xml
- The use of the OrderAccess for initialized
- Clearing the oop

I also have inlined my answers to your comments. The biggest question
will come from the multiple *_end variables. A bit of the logic there
is due to handling the slow path refill vs fast path refill and
checking that the rug was not pulled underneath the slowpath. I
believe that a previous comment was that TlabFastRefill was going to
be deprecated.

If this is true, we could revert this code a bit and just do a : if
TlabFastRefill is enabled, disable this. And then deprecate that when
TlabFastRefill is deprecated.

This might simplify this webrev and I can work on a follow-up that
either: removes TlabFastRefill if Robbin does not have the time to do
it or add the support to the assembly side to handle this correctly.
What do you think?


I support removing TlabFastRefill, but I think it is good to not depend 
on that happening first.



Now, below, inlined are my answers:

On Fri, Feb 2, 2018 at 8:44 AM, Erik Österlund
 wrote:

Hi JC,

Hope I am reviewing the right version of your work. Here goes...

src/hotspot/share/gc/shared/collectedHeap.inline.hpp:

  159 AllocTracer::send_allocation_outside_tlab(klass, result, size *
HeapWordSize, THREAD);
  160
  161 THREAD->tlab().handle_sample(THREAD, result, size);
  162 return result;
  163   }

Should not call tlab()->X without checking if (UseTLAB) IMO.


Done!


More about this later.




src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp:

So first of all, there seems to quite a few ends. There is an "end", a "hard
end", a "slow path end", and an "actual end". Moreover, it seems like the
"hard end" is actually further away than the "actual end". So the "hard end"
seems like more of a "really definitely actual end" or something. I don't
know about you, but I think it looks kind of messy. In particular, I don't
feel like the name "actual end" reflects what it represents, especially when
there is another end that is behind the "actual end".

  413 HeapWord* ThreadLocalAllocBuffer::hard_end() {
  414   // Did a fast TLAB refill occur?
  415   if (_slow_path_end != _end) {
  416 // Fix up the actual end to be now the end of this TLAB.
  417 _slow_path_end = _end;
  418 _actual_end = _end;
  419   }
  420
  421   return _actual_end + alignment_reserve();
  422 }

I really do not like making getters unexpectedly have these kind of side
effects. It is not expected that when you ask for the "hard end", you
implicitly update the "slow path end" and "actual end" to new values.


As I said, a lot of this is due to the FastTlabRefill. If I make this
not supporting FastTlabRefill, this goes away. The reason the system
needs to update itself at the get is that you only know at that get if
things have shifted underneath the tlab slow path. I am not sure of
really better names (naming is hard!), perhaps we could do these
names:

- current_tlab_end   // Either the allocated tlab end or a sampling point
- last_allocation_address  // The end of the tlab allocation
- last_slowpath_allocated_end  // In case a fast refill occurred the
end might have changed, this is to remember slow vs fast past refills

the hard_end method can be renamed to something like:
tlab_end_pointer()// The end of the lab including a bit of
alignment reserved bytes


Those names sound better to me. Could you please provide a mapping from 
the old names to the new names so I understand which one is which please?


This is my current guess of what you are proposing:

end -> current_tlab_end
actual_end -> last_allocation_address
slow_path_end -> last_slowpath_allocated_end
hard_end -> tlab_end_pointer

I would prefer this naming:

end -> slow_path_end // the end for taking a slow path; either due to 
sampling or refilling

actual_end -> allocation_end // the end for allocations
slow_path_end -> last_slow_path_end // last address for slow_path_end 
(as opposed to allocation_end)

hard_end -> reserved_end // the end of the reserved space of the TLAB

About setting things in the getter... that still seems like a very 
unpleasant thing to me. It would be better to inspect the call hierarchy 
and explicitly update the ends where they need updating, and assert in 
the getter that they are in sync, rather than implicitly setting various 
ends as a surprising side effect in a getter. It looks like the call 
hierarchy is very small. With my new naming convention, reserved_end() 
would presumably return 

RFR: JDK-8196930 - [Testbug] serviceability/sa/ClhsdbFindPC.java fails to find expected output

[Sharath Ballal]

Hello,

 

Requesting reviews for the issue:

 

ID: https://bugs.openjdk.java.net/browse/JDK-8196930 

Webrev: http://cr.openjdk.java.net/~sballal/8196930/webrev.00/ 

 

Tests run: The SA tests pass with Mach5.

 

Thanks,

Sharath

 

 

Re: RFR: 8041626: [Event Request] Shutdown reason

[Alan Bateman]

On 12/02/2018 07:07, David Holmes wrote:


Okay, I’ve removed the code related to the status field, certainly 
makes the change a bit less intrusive.


Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/


This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

-Alan