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)
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
