RFR: 8212995: Consider placing the Integer.IntegerCache and cached Integer objects in the closed archive heap region

Thu Nov 1 22:47:18 UTC 2018

Hi Ioi,

Thanks for the review. I renamed both fields as suggested and added a 
warning for closed_archive_subgraph_entry_fields. A standalone warning 
message in Integer.java could be overlooked, so I added 
HeapShared::check_closed_archive_heap_region_object() for checking the 
instances being included in the closed archive heap regions at dump time.

http://cr.openjdk.java.net/~jiangli/8212995/webrev.02/

Thanks,

Jiangli

On 10/31/18 8:52 PM, Ioi Lam wrote:
> Hi Jiangli,
>
> static ArchivableStaticFieldInfo shareable_subgraph_entry_fields[] = 
> {...}
> static ArchivableStaticFieldInfo subgraph_entry_fields[] = {...}
>
> Maybe these should be renamed to 
> {open/closed}_archive_subgraph_entry_fields?
>
> Also, I think we should add a strong warning about what objects can be 
> placed in closed_archive_subgraph_entry_fields[]. Any references 
> stored in these objects must not be modified at run time (or else we 
> could have a pointer that from the closed region to the outside, 
> violating the properties of the closed region.
>
> Maybe we should also add a warning in Integer.java, something akin to 
> "if you modify this class, check to see if it can still meet the 
> requirements in heapShared.cpp"?
>
> The rest of the code seems OK to me.
>
> As a future improvement, for all the objects whose fields are all 
> non-reference, final fields, maybe we can automatically put them in 
> the closed archive region? For example, all archived primitive box 
> objects are in this category.
>
> Thanks
> - Ioi
>
> On 10/31/18 12:45 PM, Jiangli Zhou wrote:
>> On 10/31/18 12:08 PM, Jiangli Zhou wrote:
>>
>>> Hi Ioi,
>>>
>>> Here is an updated webrev with renaming of the 'is_shared' argument. 
>>> I decided to go with your suggestion, 'is_closed_archive'.
>>>
>>> http://cr.openjdk.java.net/~jiangli/8212995/webrev.01/
>>
>> BTW, in above webrev, I also included a typo fix for the following 
>> warning that Mandy found (thanks Mandy!)
>>
>> @@ -1324,11 +1329,11 @@
>>    // header data
>>    const char* prop = 
>> Arguments::get_property("java.system.class.loader");
>>    if (prop != NULL) {
>>      warning("Archived non-system classes are disabled because the "
>>              "java.system.class.loader property is specified (value = 
>> \"%s\"). "
>> -            "To use archived non-system classes, this property must 
>> be not be set", prop);
>> +            "To use archived non-system classes, this property must 
>> not be set", prop);
>>      _has_platform_or_app_classes = false;
>>    }
>>
>> Thanks,
>> Jiangli
>>>
>>> Thanks,
>>>
>>> Jiangli
>>>
>>>
>>> On 10/30/18 4:19 PM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> On 10/30/18 3:00 PM, Ioi Lam wrote:
>>>>
>>>>> Hi Jiangli,
>>>>>
>>>>> This looks promising.
>>>>>
>>>>> Now a full review yet, but I am wondering about the name of the 
>>>>> is_shared parameter
>>>>>
>>>>>   void add_subgraph_entry_field(int static_field_offset, oop v, 
>>>>> bool is_shared);
>>>>>
>>>>> Since this is part of "heapShared", everything is "shared" in some 
>>>>> sense of the word. It could be confusing to say something is more 
>>>>> shared than other things which also shared ...
>>>>>
>>>>> How "is_closed_archive" instead?
>>>> Yes, our 'shared' has broader meaning. "is_closed_archive" or 
>>>> "is_closed_space" sounds good to me. I'll rename.
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>> On 10/30/2018 01:57 PM, Jiangli Zhou wrote:
>>>>>> Please review the following change for moving the archived 
>>>>>> Integer.IntegerCache and it's cached Integer objects (256) to the 
>>>>>> closed archiving heap region. The IntegerCache subgraph does not 
>>>>>> contain any reference that's changed at runtime (good candidate 
>>>>>> for sharing). Moving the whole subgraph into the closed archive 
>>>>>> heap region allows the memory to be shared by different JVM 
>>>>>> instances at runtime. The saving is 4K per JVM instance running 
>>>>>> the same or different java application simultaneously. Although 
>>>>>> 4K is not significant, in a larger picture the saving is much 
>>>>>> bigger (4k * (JVM_instance_num - 1) * host_num).
>>>>>>
>>>>>> As part of the change, I also restructured the code to allow us 
>>>>>> to plug in more shareable subgraphs in the closed archive heap 
>>>>>> region for runtime footprint saving in the future.
>>>>>>
>>>>>> The 'st' space is renamed to 'ca' (closed archive) space since it 
>>>>>> now contains other types of objects besides j.l.Strings.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8212995/webrev.00/
>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8212995
>>>>>>
>>>>>> Before:
>>>>>>
>>>>>> mc  space:      8416 [  0.0% of total] out of     12288 bytes [ 
>>>>>> 68.5% used] at 0x0000000800000000
>>>>>> rw  space:   3946640 [ 21.4% of total] out of   3948544 bytes 
>>>>>> [100.0% used] at 0x0000000800003000
>>>>>> ro  space:   7319328 [ 39.6% of total] out of   7319552 bytes 
>>>>>> [100.0% used] at 0x00000008003c7000
>>>>>> md  space:      2416 [  0.0% of total] out of      4096 bytes [ 
>>>>>> 59.0% used] at 0x0000000800ac2000
>>>>>> od  space:   6475944 [ 35.0% of total] out of   6479872 bytes [ 
>>>>>> 99.9% used] at 0x0000000800ac3000
>>>>>> st0 space:    438272 [  2.4% of total] out of    438272 bytes 
>>>>>> [100.0% used] at 0x00000007ffc00000 <<<<<<<<<<
>>>>>> oa0 space:    282624 [  1.5% of total] out of    282624 bytes 
>>>>>> [100.0% used] at 0x00000007ff800000 <<<<<<<<<<
>>>>>> total    :  18473640 [100.0% of total] out of  18485248 bytes [ 
>>>>>> 99.9% used]
>>>>>>
>>>>>> After:
>>>>>>
>>>>>> mc  space:      8416 [  0.0% of total] out of     12288 bytes [ 
>>>>>> 68.5% used] at 0x0000000800000000
>>>>>> rw  space:   3946640 [ 21.4% of total] out of   3948544 bytes 
>>>>>> [100.0% used] at 0x0000000800003000
>>>>>> ro  space:   7319304 [ 39.6% of total] out of   7319552 bytes 
>>>>>> [100.0% used] at 0x00000008003c7000
>>>>>> md  space:      2416 [  0.0% of total] out of      4096 bytes [ 
>>>>>> 59.0% used] at 0x0000000800ac2000
>>>>>> od  space:   6475920 [ 35.0% of total] out of   6479872 bytes [ 
>>>>>> 99.9% used] at 0x0000000800ac3000
>>>>>> ca0 space:    442368 [  2.4% of total] out of    442368 bytes 
>>>>>> [100.0% used] at 0x00000007ffc00000 <<<<<<<<<<
>>>>>> oa0 space:    278528 [  1.5% of total] out of    278528 bytes 
>>>>>> [100.0% used] at 0x00000007ff800000 <<<<<<<<<<
>>>>>> total    :  18473592 [100.0% of total] out of  18485248 bytes [ 
>>>>>> 99.9% used]
>>>>>>
>>>>>> Tested with appcds tests on linux-x64 locally. Running 
>>>>>> tier1-teir4 tests.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>