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:37:58 UTC 2018
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