RFR: 8212995: Consider placing the Integer.IntegerCache and cached Integer objects in the closed archive heap region
Jiangli Zhou
jiangli.zhou at oracle.com
Sat Nov 3 00:16:46 UTC 2018
Thanks!
Jiangli
On 11/2/18 5:15 PM, Ioi Lam wrote:
> Hi Jiangli,
>
> This looks good to me. Ship it!
>
> Thanks
>
> - Ioi
>
>
> On 11/2/18 10:57 AM, Jiangli Zhou wrote:
>> Hi Ioi,
>>
>> Here is the updated webrev with the warning message below for the
>> Integer cache.
>>
>> http://cr.openjdk.java.net/~jiangli/8212995/webrev.03/
>>
>> 995 *
>> 996 * WARNING: The cache is archived with CDS and reloaded from
>> the shared
>> 997 * archive at runtime. The archived cache (Integer[]) and
>> Integer objects
>> 998 * reside in the closed archive heap regions. Care should be
>> taken when
>> 999 * changing the implementation and the cache array should
>> not be assigned
>> 1000 * with new Integer object(s) after initialization.
>>
>> Including core-lib-dev mailing list since the change now touches the
>> core library file.
>>
>> Thanks,
>> Jiangli
>>
>>
>> On 11/1/18 10:58 PM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>>
>>>> On Nov 1, 2018, at 9:37 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>
>>>> Hi Jiangli,
>>>>
>>>> 576 void
>>>> HeapShared::check_closed_archive_heap_region_object(InstanceKlass* k,
>>>> 577 Thread* THREAD) {
>>>> 578 // Check fields in the object
>>>> 579 for (JavaFieldStream fs(k); !fs.done(); fs.next()) {
>>>> 580 if (!fs.access_flags().is_static()) {
>>>> 581 BasicType ft = fs.field_descriptor().field_type();
>>>> 582 if (!fs.access_flags().is_final() && (ft == T_ARRAY ||
>>>> T_OBJECT)) {
>>>> 583 ResourceMark rm(THREAD);
>>>> 584 log_warning(cds, heap)(
>>>> 585 "Please check reference field in %s instance in
>>>> closed archive heap region: %s %s",
>>>> 586 k->external_name(), (fs.name())->as_C_string(),
>>>> 587 (fs.signature())->as_C_string());
>>>> 588 }
>>>> 589 }
>>>> 590 }
>>>> 591 }
>>>>
>>>> Checking that all static fields of the affected class (IntegerCache
>>>> in this case) are final is not enough. The elements of a final
>>>> array can be modified.
>>> Just to clarify, the above checks all instance fields (non-static
>>> fields) in non-array objects. Static fields are not checked as
>>> mirrors are not in the closed archive heap region. In the
>>> IntegerCache subgraph case, it makes sure there is no non-final
>>> reference instance field in cached Integer objects (for future proof).
>>>> JLS requires that
>>>> https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.7
>>>> If the value p being boxed is an integer literal of type int
>>>> between -128 and 127 inclusive (§3.10.1), or the boolean literal
>>>> true or false (§3.10.3), or a character literal between '\u0000'
>>>> and '\u007f' inclusive (§3.10.4), then let a and b be the results
>>>> of any two boxing conversions of p. It is always the case that a == b.
>>>> However, there's no requirement that all these special literal
>>>> values must be created at system bootstrap time. So it's
>>>> conceivable that IntegerCache may be rewritten to create the object
>>>> dynamically:
>>>>
>>>> public static Integer valueOf(int i) {
>>>> if (i >= IntegerCache.low && i <= IntegerCache.high) {
>>>> + if (IntegerCache.cache[i + (-IntegerCache.low)] == null) {
>>>> + IntegerCache.cache[i + (-IntegerCache.low)] = new
>>>> Integer(i);
>>>> + }
>>>> return IntegerCache.cache[i + (-IntegerCache.low)];
>>>> }
>>>> return new Integer(i);
>>>> }
>>>>
>>>> Now, is this likely to happen? Probably not. However, in HotSpot,
>>>> we should not assume that the library will always stay the same,
>>>> and that the library writer knows what HotSpot requires.
>>> So your suggestion was to add warnings specifically for Integer
>>> cache array. Looks like I’ve given it a much deeper interpretation.
>>>
>>> I’ll add a warning about the cache. Will send new webrev tomorrow.
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>
>>>>> On 11/1/18 3:47 PM, Jiangli Zhou wrote:
>>>>> 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
>>
>
More information about the core-libs-dev
mailing list