RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

[Thomas Stuefe]

On Tue, 23 Feb 2021 21:41:07 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> Hi, Please review
>>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>>    The archive size will increase about 300K due to the change. 
>>    Tests: tier1-4
>>               Run MacOS/X64 binary on MacOS/aarch64 
>> 
>>    Thanks
>>    Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make 64K core region alignment only for specific platforms, also fixed comments as suggestions.

As a general remark, do you think it would make sense to move all cds related files into an own sub directory? This would make identifying easier especially for casual code readers.

- Not to do with your patch, but `FileMapInfo::set_current_info()` is unused, _current_info is modified directly. Either remove the function or use it?

----

Page sizes:

I may miss something here, but could we not simply use a constant always, independent on whether we are at dump- or runtime? I don't see why this is still dependent on DumpSharedSpaces. I see that at runtime we use the alignment set down in the archive. But how we could have a different alignment between runtime and dumptime now?

Then, we could get rid of `void MetaspaceShared::init_alignments()` and return the hard coded value directly from MetaspaceShared::core_region_alignment(). The advantage would be that in my IDE I get shown the value right away instead of having to hunt down the setter.

Then, you calculate the alignment based on os::vm_allocation_granularity(). I wonder whether it would be simpler and safer to just hard code the literal values here, combined with an assert maybe. Mainly because allocation granularity just exists for the sake of Windows, where it hides a system API, but in it was never clear whether this always will return the same value across all windows versions on the same platform. Since using hard coded values is simpler too, why not just do that? (same argument goes for os::vm_page_size() though I have never seen that value change for a platform).

Finally, if all we want is to prevent on MacOS use of small pages for archives, why do we have to condition it with the CPU at all? If on ARM page size is 16K, and on x86 we want it to be 16K to be able to load archives x86 generated archives on arm, why not just set it to 16K always?

So this would mean something like this:

  static size_t core_region_alignment() {
    const size_t ret =  
#if (defined(LINUX) && defined(AARCH64)) || (defined(WINDOWS))
        64 * K
#elif defined(__APPLE__)
        16 * K
#else
        4 * K
#endif
        ;
        assert(ret >= os::vm_allocation_granularity(), "sanity");
        return ret;
  }

----

and remove _core_region_alignment and init_alignments(), as well as use of FileMapInfo::core_region_alignment() for anything other than asserting that it is the same as MetaspaceShared::core_region_alignment().

--

Apart from that, I like this patch. The new naming for the alignment is a better description for what you want to align here.

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

Changes requested by stuefe (Reviewer).

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