On 10/4/17 2:01 AM, Magnus Ihse Bursie wrote:
On 2017-10-04 10:47, Magnus Ihse Bursie wrote:
On 2017-10-04 02:36, Ioi Lam wrote:
If you use SharedArchiveFile, you should set -XX:-VerifySharedSpaces
at the same time.
Long story short -- for security, we don't want bad archives to be
mapped into the JVM.
If you don't specify SharedArchiveFile, the archive is loaded form
the JDK installation directory, and we trust that this location is
not tempered (or else your .so files, etc, could also be tempered,
and the CDS archive is the least of your problem ...). So no need to
verify. (VerifySharedSpaces is set to 0 during JVM start-up).
However, if you specify SharedArchiveFile, it could point to an less
secured location that could be tempered (e.g., /tmp). As a result,
we picked the "safe default" of -XX:+VerifySharedSpaces, which means
the archive is checksum'ed prior to loading.
In the case of the JDK build process, we are directly executing
binaries from the build output directory anyway (temporary copies of
the "java" executable, for example). So we have historically
(implicitly) fully trusted the safety of the build output directory.
So it will should be safe to use -XX:-VerifySharedSpaces so we can
squeeze out a few seconds.
This option has been available since JDK 8u40. So it should be safe
to use it now. There's no need to wait until we switch to using JDK
9 as the boot jdk.
Ok. So if we have -XX:-VerifySharedSpaces we could use this solution,
since it includes all benefits (local control of CDS archive, no
overhead due to verification). And if we don't, we can always use
-Xshare:auto, if the user has installed a system-wide classes.jsa
herself.
Updated patch that takes all this into consideration: (third time's a
charm)
.... or maybe not. It turned out that this build failed, and with good
reasons. TL;DR: I don't want to enable CDS for building. It's too risky.
Long story: In this patch, I turned on -Xshare:on to force the use of
CDS if we have a local jsa archive. However, this fails with:
Error occurred during initialization of VM
Unable to use shared archive.
ass paths mismatch (hint: enable -XX:+TraceClassPaths to diagnose the
failure)
(Note the no-pun-intended (?) incorrect error message. Somebody should
probably file a bug about that.)
The reason for this is that for certain parts of the build, we're
using a hybrid model where we use the old JDK (the bootjdk) to execute
code, but use -Xbootclasspath/p to point to our interim classes based
on the current source code. That means core classes that are present
in the CDS archive will be replaced.
The only reason this worked at all in my prior patch was that, by pure
luck, the "ass" paths mismatched, so -Xshare:auto just dismissed it
instead of using it. Otherwise, we would have used the old classes
from the CDS archive instead.
This all just seems to risky and prone to breakage, for little gain.
We could try to apply it just in those cases were we don't override
the bootclasspath, but then we need to add overhead to the makefiles
to separate those cases. It's just not worth it.
I will drop this patch altogether and close the bug as WNF. It was a
good idea but it didn't fly. :-(
How often is -Xbootclasspath/p used?
Why not use "-XX:-VerifySharedSpaces -XX:SharedArchiveFile=local.jsa
-Xshare:auto"? That way you will have the start-up benefit if possible.
- Ioi
/Magnus
http://cr.openjdk.java.net/~ihse/JDK-8188312-use-CDS-for-bootjdk/webrev.03
I'm currently running some benchmarking in this to confirm that it is
useful for jdk8u40. In any case, we're likely to switch to jdk9 as
boot jdk not too soon I hope, so it will at least be relevant by then
according to Erik's measurements.
/Magnus
product(bool, VerifySharedSpaces,
false, \
"Verify shared spaces (false for default archive, true for
" \
"archive specified by
-XX:SharedArchiveFile)") \
\
Thanks
- Ioi
On 10/3/17 1:37 PM, Magnus Ihse Bursie wrote:
It might be the case that we should not really continue with this
as long as we have JDK 8 as default boot jdk.
If so, we should at least add a -Xshare:auto, which will allow the
user to manually add a system-wide CDS archive to their boot jdk,
and benefit from it.
And then we'll revisit this when we switch to JDK 9, to see if
"-XX:-VerifySharedSpaces -XX:SharedArchiveFile=x" is worth the effort.
Sounds reasonable?
/Magnus
On 2017-10-03 16:45, Erik Joelsson wrote:
The change looks good, but unfortunately I don't see a lot of
gain. First of all it seems the -XX:SharedArchiveFile option
removes a lot of the performance gain. Here are some rough numbers
from my machine. All of them are "time make images" with a clean
build directory, on a 16 core/32 threads linux box. In some cases
I did two runs, which further shows the amount of variance in the
build time.
Baseline: boot jdk 8, no cds
real 3m14.688s
user 38m34.132s
sys 5m4.032s
real 3m11.086s
user 38m16.820s
sys 4m35.344s
Magnus second patch: boot jdk 8, cds SharedArchiveFile
real 3m6.268s
user 38m9.168s
sys 4m13.392s
real 3m15.985s
user 37m55.328s
sys 3m50.140s
Magnus first patch: boot jdk 8, cds in classes.jsa
real 2m55.972s
user 37m59.636s
sys 4m1.020s
Baseline with boot jdk 9, no cds
real 3m14.262s
user 40m9.216s
sys 5m5.480s
Magnus first patch: boot jdk 9, cds in classes.jsa
real 3m12.721s
user 39m29.332s
sys 4m25.472s
Magnus second patch: boot jdk 9, -XX:SharedArchiveFile,
-XX:-VerifySharedSpaces
real 3m4.297s
user 39m24.556s
sys 4m31.980s
So from this (certainly limited) data set, the only configuration
that shows meaningful improvement is with boot jdk 8 and the first
patch, where we stealth add classes.jsa to the boot jdk. I find
this a very bad thing to do in principle so would vote against
such a solution anyway. Also note that we will switch default boot
from 8 to 9 very soon, so it's really the results on 9 that are
relevant here. In 9, there is another option,
-XX:-VerifySharedSpaces which removes most of the overhead added
by -XX:SharedArchiveFile. Perhaps using that this becomes worth
while.
I would like to see more data showing the optimization is actually
relevant. On my machine it seems to make a measurable difference,
but not a very significant one.
One note on the proposed change. While working on this, when I
changed my boot jdk, configure did not regenerate the classes.jsa
file. I believe for safety we should regenerate on every configure
run. It's not that expensive anyway.
/Erik
On 2017-10-03 14:55, David Holmes wrote:
Looks good to me.
Thanks,
David
On 3/10/2017 10:47 PM, Magnus Ihse Bursie wrote:
On 2017-10-03 14:21, David Holmes wrote:
Erik J. raises a good point in the bug report that
-XX:SharedArchiveFile=xxx should be used if we create the
archive. The build system has no business creating an archive
inside the boot JDK.
Agree, that is a better solution. I was not aware of the
-XX:SharedArchiveFile option.
Here's an updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8188312-use-CDS-for-bootjdk/webrev.02
I create the jsa file in configure-support, that way it can
survive a "make clean".
/Magnus
David
On 3/10/2017 9:02 PM, David Holmes wrote:
Hi Claes,
On 3/10/2017 8:48 PM, Claes Redestad wrote:
Hi,
-Xshare:auto silently ignores failures to map the shared
archive and should be safe to use. I think you're thinking of
-Xshare:on which will fail/abort the VM if mapping fails.
Ah okay.
In that case seems reasonable. But please test thoroughly
across all platforms in JPRT.
Thanks,
David
/Claes
On 2017-10-03 12:28, David Holmes wrote:
Hi Magnus,
As I just put in the bug report, it isn't quite this simple.
You have to be able to tolerate/recover from failure to map
the shared archive.
Cheers,
David
On 3/10/2017 8:24 PM, Magnus Ihse Bursie wrote:
We should use CDS to minimize Java startup time during
build. We run multiple Java commands, and every second counts.
On my machine, I get a ~3% build time speedup with this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8188312
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8188312-use-CDS-for-bootjdk/webrev.01
/Magnus
