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

[Andrew Haley]

On Wed, 24 Feb 2021 06:02:19 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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.

> Thanks for review! Currently linux-aarch64, macos-aarch64(I don't know AIX) can be configured with 4K/64K page size. To make one build compatible with both, this change takes the 64K (or bigger) as the default region alignment. It is hard to make it optional I think. Will check the possibility of more options.

It's not even slightly hard to make it optional. It should be a build-time configure argument. In many cases there is no need to make one build compatible with both 64k and 4k. If you'd like the default to be 64k I'm fine with that, but it should be possible to build with 4k alignment.

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

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