RFR: 8344831: [REDO] CDS: Parallel relocation [v6]

[Aleksey Shipilev]

On Mon, 2 Dec 2024 13:34:39 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
>> 
>>  - Rewrite FIXMEs as proper comments
>>  - Merge branch 'master' into JDK-8344831-cds-parallel-relocation-redo
>>  - Merge branch 'master' into JDK-8344831-cds-parallel-relocation-redo
>>  - Attempt at deleting the threads
>>  - Atomicity improvements
>>  - We do not actually need to track finished workers
>>  - Two-stage termination to clean up Semaphore correctly
>>  - Gross simplification: single-use ArchiveWorkers
>>  - Actual REDO increment: amendments to fix failures
>>  - Original JDK-8341334
>
> src/hotspot/share/cds/archiveUtils.cpp line 430:
> 
>> 428:   while (true) {
>> 429:     int cur = Atomic::load(&_started_workers);
>> 430:     if (cur >= _num_workers) {
> 
> Trying to understand how we could ever overshoot _num_workers. I think this should work with == too, right? In any case I'm fine with it, just puzzled.

We cannot overshoot `_started_workers` with the current code, so `==` would work. But I have a habit of coding these loops defensively, so that accidental overshoot would not lead to a runaway loop somewhere in prod.

> test/hotspot/gtest/cds/test_archiveWorkers.cpp line 46:
> 
>> 44: TEST_VM(ArchiveWorkersTest, continuous_restart) {
>> 45:   for (int c = 0; c < 1000; c++) {
>> 46:     ArchiveWorkers workers;
> 
> Maybe I am dense, but I don't understand what this test does. Is this not just object creation without anything happening? If so, should something happen here?

This thing largely tests that no invariants depend on actually executing the task. When you start messing up with asserts or states in constructors/destructors, this test start to fail in useful ways. It is easy to forget about the whole thing if test case is not present. That's why I keep it around :)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22369#discussion_r1865903145
PR Review Comment: https://git.openjdk.org/jdk/pull/22369#discussion_r1865901380