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

[Ioi Lam]

On 4/18/20 12:15 AM, Thomas Stüfe wrote:
> Hi Ioi,
>
> I am working on a small patch and have some more questions.
>
> - First, a simple one, in 
> DynamicArchiveBuilder::reserve_space_and_init_buffer_to_target_delta(), 
> the space does not have anything to do with metaspace, as you wrote, 
> so the alignment could be anything, right?
>
I think so.

> - Out of curiousity, when you pack the different regions 
> (DumpRegion::pack) you align the end to page size. Why? Why could the 
> next region not simply follow immediately? I looked if any code needs 
> a region to be page aligned, but may have missed it.

We map RO read-only and MC/RW in read-write. If the regions are not 
aligned, you will have a page that wants half to be read-only and half 
to be read-write.

I guess we can adjust the mapping to be more lenient (if a page wants 
half read-write, we map it read-write), but that's no done today.

>
> - void MetaspaceShared::initialize_dumptime_shared_and_meta_spaces() :
>
> I assume this code has to work for all three cases right
> 1) lp32.
> 2) lp64 with and without UseCompressedClassPointers?
> 3) lp64 without UseCompressedClassPointers?
>
> If yes, does the setting for UseCompressedClassPointers have to be the 
> same at run time?

Yes. The value of UseCompressedOops and UseCompressedClassPointers must 
be the same between dump time and run time.

>
>
> In this layout:
>   // On 64-bit VM, the heap and class space layout will be the same as if
>   // you're running in -Xshare:on mode:
>   //
>   //                              +-- SharedBaseAddress (default = 
> 0x800000000)
>   //                              v
>   // +-..---------+---------+ ... +----+----+----+--------------------+
>   // |    Heap    | Archive |     | MC | RW | RO |  class space     |
>   // +-..---------+---------+ ... +----+----+----+--------------------+
>   // |<--   MaxHeapSize  -->|     |<-- UnscaledClassSpaceMax = 4GB -->|
>   //
>
> Why does the class space has to follow mc+rw+ro? Could it come before?
>
>
Compressed klass pointers are stored in archived objects. If the class 
space is now lower than SharedBaseAddress, you will need to rebase all 
of the compressed klass pointers. This is not efficient and will slow 
down start-up.


>
> Actually, does it have to be in the same space at all, or could it 
> live somewhere completely different?

It can be higher. You just need to ensure that the distance between 
SharedBaseAddress to the end of the class space is within max compressed 
klass space size.

But, I am wondering why you're asking this :-)

> To ask in a more precise way: I understand that both the mc+rw+ro 
> archives and the ccs have to live in an area encompassed by the 
> compressed class pointers encoding scheme. I wonder whether there are 
> any restrictions beyond that.
>
> Could there be a gap between archives and ccs? 
Yes

> Can the order be reversed? 
No.

> Do the relative positions between archives and ccs have to be the same 
> between dump time and runtime?
No. All the pointers stored inside CDS point to inside of the MC/MW/RO 
regions, so it doesn't retain any knowledge of where the CCS was at dump 
time.


Thanks
- Ioi


>
> Thanks!
>
> On Thu, Apr 16, 2020 at 8:31 PM Ioi Lam <ioi.lam at oracle.com 
> <mailto: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
>>     <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.
>>>
>>
>