RFR: 8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region

[Stefan Karlsson]

On Fri, 13 Dec 2024 19:51:47 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> What testing with `UseCompressedClassPointers` set to false, this assert is hit due to memory being released twice during the failure path of CDS archive mapping. This patch makes it so the RW and RO regions are only released once at the end in `MetaspaceShared::release_reserved_spaces()`. Verified with tier 1-4 tests.

Nice solution for fixing the difference between Windows and the rest of the platforms!

I've added one nit below that you can ignore if you want to. I've also added a request/wish for an assert that.

src/hotspot/share/cds/filemap.cpp line 2423:

> 2421:       // Treat this region as if it has been unmapped
> 2422:       region_at(idx)->set_mapped_base(nullptr);
> 2423:     }

Personal taste, but I tend to think that the code becomes easier to read if you remove the negation in if/else statements.
Suggestion:

    // If the region is inside an active ReservedSpace, its memory and address space will be
    // freed when the ReservedSpace is released.
    if (rs.is_reserved()) {
      // Treat this region as if it has been unmapped
      region_at(idx)->set_mapped_base(nullptr);
    } else {
      unmap_region(idx);
    }


It would also be good to have an assert to check that the unmapped region actually is within the provided ReservedSpace.

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

Marked as reviewed by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22743#pullrequestreview-2505569837
PR Review Comment: https://git.openjdk.org/jdk/pull/22743#discussion_r1886438991