<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
On 03/12/13 00:47, Bengt Rutisson wrote:
<blockquote cite="mid:513EDD83.4040403@oracle.com" type="cite">
<br>
Hi Jon,
<br>
<br>
On 3/11/13 4:45 PM, Jon Masamitsu wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 03/11/13 07:34, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Hi Jon,
<br>
<br>
On 3/8/13 8:05 PM, Jon Masamitsu wrote:
<br>
<blockquote type="cite">Bengt,
<br>
<br>
With your change the order is
<br>
<br>
set_heap_size()
<br>
set_ergonomics_flags()
<br>
<br>
set_ergonomics_flags() can turn on UseCompressedOops.
<br>
set_heap_size() uses UseCompressedOops
<br>
<br>
Right?
<br>
<br>
It might not be a problem today but it makes me nervous.
<br>
<br>
Is there a real circularity there? I don't know but if
there is
<br>
a way to exclude a circularity, then I won't have to think
<br>
about it.
<br>
</blockquote>
<br>
Thanks for spotting this, Jon! There is definitely a circular
dependency between set_heap_size() and set_ergonomics_flags().
<br>
<br>
I could not figure out a nice and clean way of breaking the
circular dependency. But the circular dependency actually
originates from the fact that set_heap_size() does the check
that the original bug reports as an issue. It reduces the max
heap size if UseCompressedOops is enabled. The problem is that
_after_ that it adjust the max heap size to be at least as
large as the initial heap size. This is why we crash.
<br>
<br>
So, one way to fix this is to use the knowledge that the
initial heap size is the only thing in set_heap_size() that
can increase the max heap size beyond what UseCompressedOops
will work with. Here is a webrev for what that could look
like:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8001049/webrev.01/">http://cr.openjdk.java.net/~brutisso/8001049/webrev.01/</a>
<br>
</blockquote>
<br>
Did you intend to call set_use_compressed_oops() before
<br>
set_heap_size() and set_ergonomics_flags()? In the webrev
<br>
the call to set_use_compressed_oops() is in
<br>
set_ergonmonics_flags().
<br>
</blockquote>
<br>
I left the call to set_use_compressed_oops() inside
set_ergonomics_flags() on purpose. This was both for minimizing
the risk of the change (everything is now done in the exact same
order as before) and for making sure that
set_use_compressed_oops() has been called before we start reading
UseCompressedOops inside set_ergonomics_flags().
<br>
<br>
I am not completely happy with how this looks. It might be better
to move all of the compressed handling out of
set_ergonomics_flags(), but if it is all right with you I would
like to avoid that at the moment.
<br>
</blockquote>
<br>
I'm OK with the current fix. Moving the code to deal with<br>
COOPS seems like a good idea. I've filed an RFE for this<br>
issue.<br>
<br>
<a class="issue-created-key"
href="https://jbs.oracle.com/bugs/browse/JDK-8009910">JDK-8009910
- Refactor code in arguments.cpp to break circularity in COOPS
settings</a><br>
<br>
Your fix looks ready to go.<br>
<br>
Jon<br>
<blockquote cite="mid:513EDD83.4040403@oracle.com" type="cite">
<blockquote type="cite">
<br>
<blockquote type="cite">
<br>
I think there is another circular dependency already present
in the code. If you look at max_heap_for_compressed_oops() its
implementation uses ClassMetaspaceSize. But this value may be
updated a bit later in set_ergonomics_flags():
<br>
<br>
FLAG_SET_ERGO(uintx, ClassMetaspaceSize, 100*M);
<br>
<br>
Is this a problem? It is not really related to my current
change, but if it is a problem we should probably file a bug
for it.
<br>
</blockquote>
<br>
I'll file a bug for this. Thanks.
<br>
</blockquote>
<br>
Great, thanks!
<br>
<br>
Bengt
<br>
<br>
<br>
<blockquote type="cite">
<br>
Jon
<br>
<br>
<blockquote type="cite">
<br>
Thanks again for looking at this!
<br>
Bengt
<br>
<br>
<blockquote type="cite">
<br>
<br>
Jon
<br>
<br>
<br>
On 3/8/2013 5:20 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Hi all,
<br>
<br>
Could I have a couple of reviews for this change please?
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8001049/webrev.00/">http://cr.openjdk.java.net/~brutisso/8001049/webrev.00/</a>
<br>
<br>
I'm sending this to both the GC and the runtime teams
since I think compressed oops is mixed responsibility for
both teams.
<br>
<br>
Background (mostly from the bug report):
<br>
<br>
Hotspot crashes if it is run with a large initial size:
<br>
<br>
>./java -Xms70g -version
<br>
#
<br>
# A fatal error has been detected by the Java Runtime
Environment:
<br>
#
<br>
# SIGSEGV (0xb) at pc=0x0000000000000000, pid=14132,
tid=140305232803584
<br>
<br>
The reason is that we enable UseCompressedOops but we use
the default value for ObjectAlignmentInBytes. With a large
heap size we would have needed to adjust the object
alignment to be able to use compressed oops.
<br>
<br>
However, after reviewing the code it looks like the fix is
not to try to adjust the object alignment but rather to
not enable compressed oops for large heaps. If someone
wants to use compressed oops on a very large heap they
need to explicitly set both UseCompressedOops and
ObjectAlignmentInBytes on the command line. As far as I
can tell this is how it is intended to work.
<br>
<br>
Here is the reason for the crash and the rational behind
the fix:
<br>
<br>
In Arguments::set_ergonomics_flags() we check that the max
heap size is small enough before we enable compressed
oops:
<br>
<br>
if (MaxHeapSize <= max_heap_for_compressed_oops()) {
<br>
#if !defined(COMPILER1) || defined(TIERED)
<br>
if (FLAG_IS_DEFAULT(UseCompressedOops)) {
<br>
FLAG_SET_ERGO(bool, UseCompressedOops, true);
<br>
}
<br>
#endif
<br>
<br>
And after that we print a warning message if the heap is
too large:
<br>
<br>
if (UseCompressedOops &&
!FLAG_IS_DEFAULT(UseCompressedOops)) {
<br>
warning("Max heap size too large for Compressed
Oops");
<br>
FLAG_SET_DEFAULT(UseCompressedOops, false);
<br>
FLAG_SET_DEFAULT(UseCompressedKlassPointers, false);
<br>
}
<br>
<br>
Now the problem is that when we don't set the max heap
size on the command line it will be adjusted based on the
initial size (-Xms) and this happens in
Arguments::set_heap_size(), which is called *after*
Arguments::set_ergonomics_flags() has been called. So,
when we do the check against the max size in
Arguments::set_ergonomics_flags(), we check against the
default value for the max size. This value fits well with
a compressed heap, so we enable compressed oops and crash
later on when we can't address the upper part of the heap.
<br>
<br>
The fix is to move the call to set_heap_size() to *before*
the call to set_ergonomics_flags(). This way the check is
done against the correct value. This has two effects:
<br>
<br>
1) We don't enable UseCompressedOops on heap sizes that
are too large
<br>
2) If someone sets -XX:+UseCompressedOops on the command
line but specifies a too large heap a warning message will
be logged and UseCompressedOops will be turned off.
<br>
<br>
I am always hesitant to rearrange the order of calls in
Arguments::parse(). But in this case it is necessary to
get the correct behavior and I think it is safe. As far as
I can tell there is no other code between the two methods
that try to read the MaxHeapSize value.
<br>
<br>
Thanks,
<br>
Bengt
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>