[aarch64-port-dev ] Question about ccs reservation, CDS and aarch64 specifics

[Thomas Stüfe]

On Thu, Apr 16, 2020 at 8:31 PM Ioi Lam <ioi.lam at oracle.com> wrote:

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

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.
>
>
Oh thank god :) That is good, this really tripped me off when reading the
code.

Thanks, Thomas


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