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....@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