RFR: 8227226: Segmented array clearing for ZGC

Fri Jul 26 17:56:57 UTC 2019

Hi Per,

I like the direction of this, as I think it will help us down the road. Note though that if you go down this path, you might want to make sure zero initializing stores are not emitted by the JITs. These places can be found if you search for ZeroTLAB. Otherwise we lose out on some key synergy effects with that approach (clearing lots of memory twice)

And if we go down that route, we might want to interface this better between the GC and compiler; find a new property name that the compiler checks for, like BarrierSetC2::zero_initializing_stores() or something, which may or may not always correspond to ZeroTLAB.

Might also want to consider NUMA effects when clearing the whole relocation set. But that could be an optional step 2.

/Erik

> On 26 Jul 2019, at 14:27, Per Liden <per.liden at oracle.com> wrote:
> 
> Hi Ryan & Erik,
> 
> I had a look at this and started exploring a slightly different approach. Instead doing segmented clearing in the allocation path, we can have the concurrent GC thread clear pages when they are reclaimed and not do any clearing in the allocation path at all.
> 
> That would look like this:
> 
> http://cr.openjdk.java.net/~pliden/8227226/webrev.0-base
> 
> (I've had to temporarily comment out three lines of assert/debug code to make this work)
> 
> The relocation set selection phase will now be tasked with some potentially expensive clearing work, so we'll want to make that part parallel also.
> 
> http://cr.openjdk.java.net/~pliden/8227226/webrev.0-parallel
> 
> Moving this work from Java threads onto the concurrent GC threads means we will potentially prolong the RelocationSetSelection and Relocation phases. That might be a trade-off worth doing. In return, we get:
> 
> * Faster array allocations, as there's now less work done in the allocation path.
> * This benefits all arrays, not just those allocated in large pages.
> * No need to consider/tune a "chunk size".
> * I also tend think we'll end up with slightly less complex code, that is a bit easier to reason about. Can be debated of course.
> 
> This approach might also "survive" longer, because the YC scheme we've been loosely thinking about currently requires newly allocated pages to be cleared anyway. It's of course too early to tell if that requirement will stand in the end, but it's possible anyway.
> 
> I'll need to do some more testing and benchmarking to make sure there's no regression or bugs here. The commented out debug code also needs to be addressed of course.
> 
> Comments? Other ideas?
> 
> cheers,
> Per
> 
>> On 7/24/19 4:37 PM, Sciampacone, Ryan wrote:
>> Somehow I lost the RFR off the front and started a new thread.
>>  Now that we're both off vacation I'd like to revisit this.  Can you take a look?
>> On 7/8/19, 10:40 AM, "hotspot-gc-dev on behalf of Sciampacone, Ryan" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of sci at amazon.com> wrote:
>>          http://cr.openjdk.java.net/~phh/8227226/webrev.01/
>>          This shifts away from abusing the constructor do_zero magic in exchange for virtualizing mem_clear() and specializing for the Z version.  It does create a change in mem_clear in that it returns an updated version of mem.  It does create change outside of the Z code however it does feel cleaner.
>>          I didn't make a change to PinAllocating - looking at it, the safety of keeping it constructor / destructor based still seemed appropriate to me.  If the objection is to using the sequence numbers to pin (and instead using handles to update) - this to me seems less error prone.  I had originally discussed handles with Stefan but the proposal came down to this which looks much cleaner.
>>          On 7/8/19, 6:36 AM, "hotspot-gc-dev on behalf of Sciampacone, Ryan" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of sci at amazon.com> wrote:
>>                       1) Yes this was a conscious decision.  There was discussion on determining the optimal point for breakup but given the existing sizes this seemed sufficient.  This doesn't preclude the ability to go down that path if its deemed absolutely necessary.  The path for more complex decisions is now available.
>>         2) Agree
>>         3) I'm not clear here.  Do you mean effectively going direct to ZHeap and bypassing the single function PinAllocating?  Agree.  Otherwise I'll ask you to be a bit clearer.
>>         4) Agree
>>         5) I initially had the exact same reaction but I played around with a few other versions (including breaking up initialization points between header and body to get the desired results) and this ended up looking correct.  I'll try mixing in the mem clearer function again (fresh start) to see if it looks any better.
>>                  On 7/8/19, 5:49 AM, "Per Liden" <per.liden at oracle.com> wrote:
>>                      Hi Ryan,
>>                          A few general comments:
>>                          1) It looks like this still only work for large pages?
>>             2) The log_info stuff should be removed.
>>             3) I'm not a huge fan of single-use utilities like PinAllocating, at
>>             least not when, IMO, the alternative is more straight forward and less code.
>>             4) Please make locals const when possible.
>>             5) Duplicating _do_zero looks odd. Injecting a "mem clearer", similar to
>>             what Stefans original patch did, seems worth exploring.
>>                          cheers,
>>             /Per
>>                          (Btw, I'm on vacation so I might not be super-responsive to emails)
>>                          On 2019-07-08 12:42, Erik Österlund wrote:
>>             > Hi Ryan,
>>             >
>>             > This looks good in general. Just some stylistic things...
>>             >
>>             > 1) In the ZGC project we like the letter 'Z' so much that we put it in
>>             > front of everything we possibly can, including all class names.
>>             > 2) We also explicitly state things are private even though it's
>>             > bleedingly obvious.
>>             >
>>             > So:
>>             >
>>             > 39 class PinAllocating {
>>             > 40 HeapWord* _mem;
>>             > 41 public: -> 39 class ZPinAllocating { 40 private:    41 HeapWord* _mem;
>>             >    42
>>             >   41 public: I can be your sponsor and push this change for you. I don't
>>             > think there is a need for another webrev for my small stylistic remarks,
>>             > so I can just fix that before pushing this for you. On that note, I'll
>>             > add me and StefanK to the contributed-by section as we all worked out
>>             > the right solution to this problem collaboratively. I have run through
>>             > mach5 tier1-5, and found no issues with this patch. Thanks, /Erik
>>             >
>>             > On 2019-07-05 17:18, Sciampacone, Ryan wrote:
>>             >> http://cr.openjdk.java.net/~phh/8227226/webrev.00/
>>             >> https://bugs.openjdk.java.net/browse/JDK-8227226
>>             >>
>>             >> This patch introduces safe point checks into array clearing during
>>             >> allocation for ZGC.  The patch isolates the changes to ZGC as (in
>>             >> particular with the more modern collectors) the approach to
>>             >> incrementalizing or respecting safe point checks is going to be
>>             >> different.
>>             >>
>>             >> The approach is to keep the region holding the array in the allocating
>>             >> state (pin logic) while updating the color to the array after checks.
>>             >>
>>             >> Can I get a review?  Thanks.
>>             >>
>>             >> Ryan
>>             >
>>                                         