RFR: 8270489: Support archived heap objects in EpsilonGC [v2]

[Ioi Lam]

On Wed, 11 Aug 2021 08:31:40 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @iignatev comments
>
> src/hotspot/share/cds/heapShared.hpp line 152:
> 
>> 150:   // - Mapped: (G1 only) the regions are directly mapped into the heap
>> 151:   // - Loaded: At VM start-up, the objects in the heap regions are copied into the
>> 152:   //           heap's old generation. This is easier to implement than mapping but
> 
> I'd refrain from saying "heap's old generation", as the interface apparently gives the GC freedom in putting the things where it can.

I've updated the comments.

> src/hotspot/share/gc/epsilon/epsilonHeap.cpp line 3:
> 
>> 1: /*
>> 2:  * Copyright (c) 2017, 2020, Red Hat, Inc. All rights reserved.
>> 3:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> 
> Here and later: please update Red Hat's copyright year instead. (Like we do for mostly-Oracle files in other cases).

Changed as you requested.

> src/hotspot/share/gc/epsilon/epsilonHeap.cpp line 155:
> 
>> 153: 
>> 154:   // Allocation successful, update counters
>> 155:   if (Metaspace::initialized()) {
> 
> Feels odd to protect this entire block. It handles generic allocations, not only the metaspace. (I suspect this is only relevant during the initial allocation at startup, but it still looks weird). Would moving a check to `EpsilonMonitoringSupport::update_counters()` and protecting `MetaspaceCounters::update_performance_counters();` there be enough?

I can't move `if (Metaspace::initialized())` into `EpsilonMonitoringSupport::update_counters()` because the code in this function also updates the global variable `_last_counter_update`.

> test/hotspot/jtreg/runtime/cds/appcds/TestEpsilonGCWithCDS.java line 28:
> 
>> 26:  * @bug 8234679
>> 27:  * @requires vm.cds
>> 28:  * @requires vm.bits == 64
> 
> Is this test/support really 64-bit specific? I think GHA would test 32-bit Epsilon in `tier1`, not sure about this test, though.

I removed this line. 64 bit is not required by this test.

> test/hotspot/jtreg/runtime/cds/appcds/TestZGCWithCDS.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> 
> Here and later: for the files with no changes, no need to update this line, I think.

I reverted the files with only copyright year changes.

> test/hotspot/jtreg/runtime/cds/appcds/sharedStrings/FlagCombo.java line 29:
> 
>> 27:  * @summary Test relevant combinations of command line flags with shared strings
>> 28:  * @requires vm.cds.archived.java.heap & vm.hasJFR
>> 29:  * @requires vm.gc == null
> 
> Here and later: feels like these changes can be forked out into a separate testbug-only PR? Igor did quite a few touchups already for the tests that set their own options.

I will move most of the test file changes into a separate RFE. [JDK-8272348](https://bugs.openjdk.java.net/browse/JDK-8272348): Update CDS tests in anticipation of JDK-8270489

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

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