RFR: 8212995: Consider placing the Integer.IntegerCache and cached Integer objects in the closed archive heap region
Ioi Lam
ioi.lam at oracle.com
Fri Nov 2 04:38:59 UTC 2018
BTW, the bug title should be changed from "Consider placing ..." to
"Place ..."
Thanks
- Ioi
On 11/1/18 9:37 PM, Ioi Lam 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.
>
> 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.
>
> 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 hotspot-runtime-dev
mailing list