RFR: 8254739: G1: Optimize evacuation failure for regions with few failed objects [v7]

[Thomas Schatzl]

On Fri, 10 Sep 2021 12:44:58 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix compilation error on windows
>
> Performance is good as is, particularly the Remove self-forwards phase is much much faster now. I added a figure to the CR [here](https://bugs.openjdk.java.net/secure/attachment/96321/20210902-remove-self-forwards-improvement.png). Really nice.
> 
> Although there are a few existing abnormalities with this change (not newly introduced, the old code is as bad), see [JDK-8273309](https://bugs.openjdk.java.net/browse/JDK-8273309).
> 
> There are a few things that need to be improved:
> * we talked about this before, but I do not think putting `G1EvacuationFailureObjsInHR`, which is something only used in evacuation failure, into `HeapRegion`, should be done. At least at the moment, it is almost never used.
> * I do not like the use and the implementation of `G1EvacuationFailureObjsInHR`: it recreates something thatt is done better elsewhere (mark stack, DCQS buffer allocator, and in particular in the remembered set cardset allocator) with less features, in a non-fitting style.
> Examples are something like reuse of available chunks, concurrent freeing of chunks, automatic resizing of blocks on demand to decrease allocations and potentially other stuff seems something we would really really want.
> * as far as I understand the implementation of `G1EvacuationFailureObjsInHR` ;) - there is zero documentation about the basic structure - it seems to allocate quite a lot of memory that is almost never used. Even in the case of evacuation failure it's likely to be almost empty.
> `G1EvacuationFailureObjsInHR` seems to basically be a preallocated `ArrayList` of chunks (the `_array_list` member).
> Afaict it uses like 1/256 of total heap size (i.e. 0.3%), that's way way too much ( 1 / (256 * sizeof(HeapWord)) * sizeof(pointer); in `G1EvacuationFailureObjsInHR.cpp:86`; the `Array` constructor in `G1EvacuationFailureObjsInHR.hpp:124)) ). We will also get into trouble about startup time about this, I'm sure. That, tbh, alone makes it a no-go if I did not miscalculate.
> 
> Let's work on renaming and reusing the infrastructure provided by the remembered set (`G1CardSetAllocator` etc) in `g1CardSetMemory.hpp`.

> @tschatzl Hi Thomas, is there any way I can reproduce this compilation error (drop_all reference) on my local? I use "bash configure; make images CONF=rel", there is no error on linux x86_64.

Probably try to compile with `--disable-precompiled-headers` (i.e. add to `configure` options). I had this issue yesterday too, but it went away after some changes (and I've been running our CI successfully with my changes).

Dug into that a bit deeper - can you try
https://github.com/tschatzl/jdk/commit/bc79db0cd8c4eff8b0273e50046b212d201467c0 ?

The `g1SegmentedArray.inline.hpp` was not included in `heapregion.inline.hpp`, and that test was missing tons of includes anyway. Also we should clean up the `.hpp` files to not use methods from `.inline.hpp` - if so, they need to be moved to the `.inline.hpp` file too.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5181