RFR: 8212995: Consider placing the Integer.IntegerCache and cached Integer objects in the closed archive heap region
Ioi Lam
ioi.lam at oracle.com
Sat Nov 3 00:15:52 UTC 2018
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