8227226: Segmented array clearing for ZGC
Per Liden
per.liden at oracle.com
Thu Aug 8 08:42:59 UTC 2019
On 8/7/19 3:55 PM, Sciampacone, Ryan wrote:
> > By overriding finish(), the sampling/reporting remains correct and
> > unaffected, as it will never see the intermediate long[].
>
> I learned something today. Thank you.
>
> For MemAllocator, I think we all agree the flow is locked in a bit too rigidly but this helps with some of the VM/GC assumptions so we end up battling it. That said I'm with you - if there's a rewrite to be had, it's not in this patch.
>
> Otherwise, fwiw lgtm.
Thanks Ryan!
(Since you don't have an OpenJDK id I can't add you as Reviewed-by, but
Stefan, Erik and you will all be added as Contributed-by)
cheers,
Per
>
>
> On 8/7/19, 3:24 AM, "Per Liden" <per.liden at oracle.com> wrote:
>
> Hi again,
>
> On 8/7/19 11:59 AM, Per Liden wrote:
> > Hi Ryan,
> >
> > On 8/7/19 3:05 AM, Sciampacone, Ryan wrote:
> >> Although least intrusive, it goes back to some of the earlier
> >> complaints about using false in the constructor for do_zero. It also
> >> makes a fair number of
> >
> > My earlier comment about this was not about passing false to the
> > constructor, but the duplication of the _do_zero member, which I thought
> > looked a bit odd. In this patch, this was avoided by separation these
> > paths already in ZCollectedHeap::array_allocate().
> >
> >> assumptions (and goes against the hierarchies intent) on
> >> initialization logic to hide in finish(). That said, I agree that is
> >> fairly clean - and definitely addresses the missed cases of the
> >> earlier webrev.
> >>
> >
> > We've had the same discussions here and concluded that we might want to
> > restructure parts of MemAllocator to better accommodate this use case,
> > but that overriding finish() seems ok for now. A patch to restructure
> > MemAllocator could come later if we think it's needed.
> >
> >> 2 things,
> >>
> >> 1. Isn't the substitute_oop_array_klass() check too narrow? It will
> >> only detect types Object[], and not any other type of reference array
> >> (such as String[]) ? I believe there's a bug here (correct me if I'm
> >> wrong).
> >
> > On the JVM level, Object[], String[] and int[][] all have the same
> > Klass, so we should catch them all with this single check.
>
> Sorry, I'm of course wrong here. Changed the check to call
> klass->is_objArray_klass() instead. Thanks!
>
> Updated webrev.4 in-place.
>
> cheers,
> Per
>
> >
> >> 2. I'd want to see an assert() on the sizeof(long) == sizeof(void *)
> >> dependency. I realize what code base this is in but it would be
> >> properly defensive.
> >
> > Sounds good.
> >
> >>
> >> What does the reporting look like in this case? Is the long[] type
> >> reported accepted? I'm wondering if this depletes some of the
> >> simplicity.
> >
> > By overriding finish(), the sampling/reporting remains correct and
> > unaffected, as it will never see the intermediate long[].
> >
> > Updated webrev:
> >
> > http://cr.openjdk.java.net/~pliden/8227226/webrev.4
> >
> > cheers,
> > Per
> >
> >>
> >> On 8/2/19, 6:13 AM, "hotspot-gc-dev on behalf of Per Liden"
> >> <hotspot-gc-dev-bounces at openjdk.java.net on behalf of
> >> per.liden at oracle.com> wrote:
> >>
> >> Did some micro-benchmarking (on a Xeon E5-2630) with various segment
> >> sizes between 4K and 512K, and 64K seems to offer a good
> >> trade-off. For
> >> a 1G array, the allocation time increases by ~1%, but in exchange
> >> the
> >> worst case TTSP drops from ~280ms to ~0.6ms.
> >> Updated webrev using 64K:
> >> http://cr.openjdk.java.net/~pliden/8227226/webrev.3
> >> cheers,
> >> Per
> >> On 8/2/19 11:11 AM, Per Liden wrote:
> >> > Hi Erik,
> >> >
> >> > On 8/1/19 5:56 PM, Erik Osterlund wrote:
> >> >> Hi Per,
> >> >>
> >> >> I like that this approach is unintrusive, does its thing at
> >> the right
> >> >> abstraction layer, and also handles medium sized arrays.
> >> >
> >> > It even handles small arrays (i.e. arrays in small zpages) ;)
> >> >
> >> >> Looks good.
> >> >
> >> > Thanks! I'll test various segment sizes and see how that affects
> >> > performance and TTSP.
> >> >
> >> > cheers,
> >> > Per
> >> >
> >> >>
> >> >> Thanks,
> >> >> /Erik
> >> >>
> >> >>> On 1 Aug 2019, at 16:14, Per Liden <per.liden at oracle.com> wrote:
> >> >>>
> >> >>> Here's an updated webrev that should be complete, i.e. fixes the
> >> >>> issues related to allocation sampling/reporting that I
> >> mentioned.
> >> >>> This patch makes MemAllocator::finish() virtual, so that we
> >> can do
> >> >>> our thing and install the correct klass pointer before the
> >> Allocation
> >> >>> destructor executes. This seems to be the least intrusive why of
> >> >>> doing this.
> >> >>>
> >> >>> http://cr.openjdk.java.net/~pliden/8227226/webrev.2
> >> >>>
> >> >>> This passed function testing, but proper benchmarking remains
> >> to be
> >> >>> done.
> >> >>>
> >> >>> cheers,
> >> >>> Per
> >> >>>
> >> >>>> On 7/31/19 7:19 PM, Per Liden wrote:
> >> >>>> Hi,
> >> >>>> I found some time to benchmark the "GC clears
> >> pages"-approach, and
> >> >>>> it's fairly clear that it's not paying off. So ditching that
> >> idea.
> >> >>>> However, I'm still looking for something that would not just do
> >> >>>> segmented clearing of arrays in large zpages. Letting oop
> >> arrays
> >> >>>> temporarily be typed arrays while it's being cleared could
> >> be an
> >> >>>> option. I did a prototype for that, which looks like this:
> >> >>>> http://cr.openjdk.java.net/~pliden/8227226/webrev.1
> >> >>>> There's at least one issue here, the code doing allocation
> >> sampling
> >> >>>> will see that we allocated long arrays instead of oop
> >> arrays, so the
> >> >>>> reporting there will be skewed. That can be addressed if we
> >> go down
> >> >>>> this path. The code is otherwise fairly simple and
> >> contained. Feel
> >> >>>> free to spot any issues.
> >> >>>> cheers,
> >> >>>> Per
> >> >>>>> On 7/26/19 2:27 PM, Per Liden 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
> >> >>>>>> >
> >> >>>>>>
> >> >>
> >>
>
>
More information about the hotspot-gc-dev
mailing list