RFR: 8227226: Segmented array clearing for ZGC
Per Liden
per.liden at oracle.com
Thu Aug 1 14:14:25 UTC 2019
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