Question about ccs reservation, CDS and aarch64 specifics

[Ioi Lam]

On 4/16/20 11:14 AM, Thomas Stüfe wrote:
> Hi Ioi,
>
> On Thu, Apr 16, 2020 at 7:49 PM Ioi Lam <ioi.lam at oracle.com 
> <mailto:ioi.lam at oracle.com>> wrote:
>
>     (I suppose you mean "compressed class space" by "ccs" :-)
>
>
> Yes, I think I stole this from Stefan Karlsson :)
>
>     <snip>
>
>     I am not even sure if case (C) can happen at all.
>
>     I admit that I've been guilty of making the interface even more
>     complicated
>     with JDK-8231610
>     <https://bugs.openjdk.java.net/browse/JDK-8231610>(Relocate the
>     CDS archive if it cannot be mapped to the
>     requested address). Looks now is a good time to clean up.
>
>
> The coding has been complicated to begin with, and then it usually 
> only gets worse since no-one has time for a revamp :( A clean up would 
> be very helpful.
>
> One reason I look at this coding now, beside the aarch64 problem, was 
> that I try to disentangle CDS from Metaspace, especially the alignment 
> policy. Remember, I tried to tackle this last summer? but it keeps 
> biting me. For such a small problem this is weirdly complicated.
>
>     One thing that can be cleaned up is the call to
>     Metaspace::allocate_metaspace_compressed_klass_ptrs:
>
>     (a) when CDS is enabled:
>
>         Metaspace::global_initialize()
>         -> MetaspaceShared::initialize_runtime_shared_and_meta_spaces()
>            -> ... MetaspaceShared::map_archives()
>              -> ... reserve the space, eventually calling
>     Metaspace::reserve_space
>              -> call Metaspace::allocate_metaspace_compressed_klass_ptrs()
>
>     (b) when CDS is disabled
>
>         Metaspace::global_initialize()
>     -> allocate_metaspace_compressed_klass_ptrs
>            -> (if cds is not enabled) Metaspace::reserve_space()
>
>
>     In case (b), we should first reserve the space, and then call into
>     allocate_metaspace_compressed_klass_ptrs. This will simplify the
>     arguments
>     of allocate_metaspace_compressed_klass_ptrs, and will also limit
>     the variations
>     of calls to Metaspace::reserve_space(). I think this will make it
>     possible to
>     drop the use_requested_addr argument and rely simply on
>     (requested_addr != NULL)
>
>
> So, in all cases we'd pre-reserve the ReservedSpace and hand it down 
> to Metaspace::allocate_metaspace_compressed_klass_ptrs()?
>
> This would melt down 
> Metaspace::allocate_metaspace_compressed_klass_ptrs() to just 
> "initialize compressed class space from a pre-arranged ReservedSpace, 
> and set up base + shift".
>
> We could probably rename that thing 
> to Metaspace::set_up_compressed_klass_space(ReservedSpace* rs, cds_base);
>
> We even could move set_narrow_klass_base_and_shift() out of 
> Metaspace::set_up_compressed_klass_space, then it becomes a series of 
> three simple operations:
> 1) obtain a ReservedSpace however you see fit
> 2) register it with Metaspace as address space for ccs,
> 3) set_narrow_klass_base_and_shift. We would not have to hand down 
> cds_base to Metaspace, only for it to be used as base address 
> in set_narrow_klass_base_and_shift.
>

Yes, that seems the right thing to do. That will hopefully make the 
aarch64 initialization code a little simpler as well.

> One question which came to me today was:
>
> In AppCDS, DynamicArchiveBuilder::do_it() calls 
> Metaspace::reserve_space(). Is that really needed, does a DumpRegion 
> have anything to do with ccs? Don't they just need some space to dump 
> into? Hope that question is not dumb.
>
Do you mean:

DynamicArchiveBuilder::reserve_space_and_init_buffer_to_target_delta()
-> MetaspaceShared::reserve_shared_space
     -> Metaspace::reserve_space

That's not necessary. When I wrote the code I thought 
Metaspace::reserve_space was a general function for reserving spaces :-) 
but as you said, this function is probably intended only for 
initializing the CCS.

Thanks
- Ioi

> Thanks, Thomas
>
>     Thanks
>     - Ioi
>
>
>>>     Does that make sense? In other words, if the whole point of
>>>     Metaspace::reserve_preferred_space() is "OS knows better, let it try
>>>     to find a good address", would it not make sense to just try a low
>>>     address as part of the try-addresses-loop?
>>     We certainly don't want to have to use a dedicated heapbase register
>>     or a shift. Just give us a multiple of 4*G and we're happy.
>>
>