<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Thanks for the review, Jon!<br>
<br>
And thanks for filing the refactoring bug!<br>
<br>
Bengt<br>
<br>
On 3/12/13 4:41 PM, Jon Masamitsu wrote:<br>
</div>
<blockquote cite="mid:513F4C9F.8020408@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/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 moz-do-not-send="true" 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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/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>
</blockquote>
<br>
</body>
</html>