On Thu, 17 Nov 2022 09:24:04 GMT, Erik Österlund <[email protected]> wrote:
>> The current loom code makes some assumptions about GC that will not work
>> with generational ZGC. We should make this code more GC agnostic, and
>> provide a better interface for talking to the GC.
>>
>> In particular,
>> 1) All GCs have a way of encoding oops inside of the heap differently to
>> oops outside of the heap. For non-ZGC collectors, that is compressed oops.
>> For ZGC, that is colored pointers. With generational ZGC, pointers on-heap
>> will be colored and pointers off-heap will be "colorless". So we need to
>> generalize encoding and decoding of oops in the heap, for loom.
>>
>> 2) The cont_oop is located on a stack. In order to access it we need to
>> start_processing on that thread, if it isn't the current thread. This
>> happened to work so far for ZGC, because the stale pointers had enough
>> colors. But with generational ZGC, these on-stack oops will be colorless, so
>> we have to be more accurate here and ensure processing really has started on
>> any thread that cont_oop is used on. To make life a bit easier, I'm moving
>> the oop processing responsibility for these oops to the thread instead.
>> Currently there is no more than one of these, so doing it lazily per frame
>> seems a bit overkill.
>>
>> 3) Refactoring the stack chunk allocation code
>>
>> Tested with tier1-5 and manually running Skynet. No regressions detected. We
>> have also been running with this (yet a slightly different backend) in the
>> generational ZGC repo for a while now.
>
> Erik Österlund has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix Richard comments
I went through the changes and all looks good to me. Only minor comments.
Thanks,
Patricio
src/hotspot/share/gc/shared/memAllocator.cpp line 381:
> 379: }
> 380:
> 381: oop MemAllocator::try_allocate_in_existing_tlab() {
try_allocate_in_existing_tlab() is now unused in memAllocator.hpp.
src/hotspot/share/gc/shared/memAllocator.hpp line 98:
> 96: virtual oop initialize(HeapWord* mem) const;
> 97:
> 98: using MemAllocator::allocate;
Do we need these declarations? I thought this would be needed if allocate()
would not be public on the base class or to avoid hiding it if here we define a
method with the same name but different signature.
src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1393:
> 1391: // Guaranteed to be in young gen / newly allocated memory
> 1392: assert(!chunk->requires_barriers(), "Unfamiliar GC requires
> barriers on TLAB allocation");
> 1393: _barriers = false;
Do we need to explicitly set _barriers to false? It's already initialized to be
false (same above for the UseZGC case). That would also allow to simplify the
code a bit I think to be just an if statement that calls requires_barriers()
for the "ZGC_ONLY(!UseZGC &&) (SHENANDOAHGC_ONLY(UseShenandoahGC ||)
allocator.took_slow_path())" case, and then ZGC and the fast path could use
just separate asserts outside conditionals.
-------------
Marked as reviewed by pchilanomate (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11111
