New webrev: http://cr.openjdk.java.net/~erikj/8150736/webrev.02/
On 2016-10-04 11:03, Magnus Ihse Bursie wrote:
On 2016-10-03 10:22, Erik Joelsson wrote:
Hello,
On 2016-10-01 14:13, David Holmes wrote:
Hi Erik,
This is a big change that is hard to digest. I assume the actual
images themselves are unaffected by this change?
I understand that and I have been working on this for some time in
the background. Yes, I have done quite extensive comparisons during
this work so there should not be any relevant differences in the
images. There are some, for example jexec and jspawnhelper are now
stripped while the old strip logic missed them. I consider that
fixing a bug.
I was about to leave this patch for another release but since the
schedule was moved, and Magnus is available to review things again, I
thought it was worth trying to get it in. I believe it will add
significant value.
I believe so too. :-) It's great to see this cleared up!
Overall, I think your patch looks good. A few things:
* In hotspot/make/GensrcJvmti.gmk, you have added out-commented code. :-)
Created bug JDK-8167078 to track this issue and referred to it in the
comments, both in GensrcJvmti.gmk and Copy-java.base.gmk. I don't want
to throw away the build logic for these header files since I believe the
correct fix is to get the files from hotspot.
* You have missed updating copyright year overall.
Fixed.
* I'm wondering a bit about this comment about hotspot, that is now
removed:
# NOTE: The old build did not strip binaries on macosx.
As far as I can see, we are now stripping libjvm.dylib. Is this
something that has changed, or was the old comment incorrect?
The old comment was correct for the hotspot build, but then the images
build did a second round of strips. This is how the JDK libraries used
to get stripped. Since I now let SetupNativeCompilation do the
stripping, the secondary strip logic is removed and macosx libjvm must
be stripped like all the others.
* In jdk/make/Import.gmk, we did a lot of special handling for
libjsig, e.g. handle STATIC_BUILD separately, or selecting different
versions depending on if client or server was built (which even seemed
to be OS dependent). While the old logic looked really suspicios, I'm
not sure how to match that to the new code in CompileLibjsig.gmk. Are
the end result really the same? If not, can you specify how things
have changed and reassure me that it is correct? :-)
Short answer: The end result is the same.
STATIC_BUILD disabled building and importing of libjsig and still does.
What the old logic basically did was creating symlinks from each variant
subdir like this:
lib/amd64/libjsig.so
lib/amd64/server/libjsig.so -> ../libjsig.so
For the library that was trivial, but it also did the same for the
symbol files, and zipped the link if zipping symbols. On macosx, with
the dSYM dirs, this got even more complicated, which is the reason for
platform checks (as well as libjsig not being built on windows).
The only difference is this. When --with-native-debug-symbols=zipped is
set, we end up with both zipped and non zipped debug symbols in the
modules_libs/<module> dir. Because of this, both the zipped and non
zipped symlink are created, where before, only the zipped one would be.
An initial nit - why use "product-" in the target names?
"images-jdk" etc seems perfectly sufficient, and "product" may be a
misnomer.
A while back when we started introducing other images (test-image,
docs-image, symbols-image) we also renamed the old images target to
product-images. We left the alias images though since it's fairly
well established, and introduced the all-images target to build all
of them. To me it was then natural to create sub targets of
product-images rather than images for jdk and jre. I do think the new
targets are too long however and because of that will likely not see
much use. Perhaps we can add aliases for them too as you suggest.
I agree that product-images-jdk is not a very good name. But I also
agree with Erik's reasoning. :-) However, I have a suggestion that I
think will suit you both. :)
I suggest that we call the new, fine-grained, targets "jdk-image",
"jre-image" and "symbols-image". This is in line with the singular
form of "docs-image" and "test-image". The actual reason that "images"
(and hence, "product-images") was using the plural form was that it
consisted of these several individual images. So, if we have:
# Define product images
product-images: jdk-image jre-image symbols-image
# Add a common alias
images: product-images
In fact, we almost already does something like this:
# This target builds the product images, e.g. the JRE and JDK image
# (and possibly other, more specific versions)
product-images: product-images-jdk product-images-jre
product-images-symbols \
zip-security exploded-image
This is so simple and obvious, fixed.
... which got me wondering if "zip-security" is actually correctly
placed. I assume it should be a dependency to product-images-jdk (or
rather "jdk-image" :)), instead.
zip-security is a bit of an oddity. Right now is not a good time to sort
it out however. I have other work in progress where it will be. Added a
comment about it.
/Erik
/Magnus
/Erik
Thanks,
David
On 30/09/2016 8:17 PM, Erik Joelsson wrote:
Here is a rather large patch which should make life better for most
people building and developing OpenJDK. The main goal of this patch is
to reduce unnecessary space used by the build. A side goal also become
to figure out a better general strategy for handling native debug info
during the build since that was a large part of the wasted space.
These
are the high level changes:
1. Removed the dist step in the hotspot build (and the corresponding
import step in the jdk build) and linking libjvm directly into
support/modules_libs. This removes 2 extra copies of all the
hotspot
native libraries.
2. Made many files in the exploded image ($(OUTPUT_DIR)/jdk) symlinks
into support/modules_*. Note that binaries and libraries cannot be
symlinks since the image won't run then. Debug symbols and other
files seem to work fine though.
3. Introduced separate top level targets for building the jdk, jre
(and
serverjre) images. A developer can run "make product-images-jdk"
and
skip the others. The test targets now depend on just the jdk image
to avoid building extra images that aren't needed for tests.
4. Changed SetupNativeCompilation to strip binaries by default unless
told not to, and removed the separate strip build step. This
lets us
get rid of modules_*-stripped.
5. Build debug symbols into the OUTPUT_DIR in SetupNativeCompilation
(instead of OBJECT_DIR and then copy to OUTPUT_DIR). For normal jdk
and hotspot libraries, this means the debug symbols end up directly
in support/modules_libs/$MODULE/... and there is only one copy
of them.
6. If zipping debug symbols, this will also leave the unzipped
debugsymbols in OUTPUT_DIR, where we wouldn't do so before. This
also means that the exploded image will get symlinks to the
unzipped
debug symbols regardless of if we are asked to zip them or not.
This
in turn means that debugging the exploded image will just work.
This
change is space neutral since we were already keeping a copy of the
unzipped debugsymbols in the OBJECT_DIR before, they have just been
moved to a more useful place.
7. Stop copying debug symbols from test native binaries since we
aren't
stripping them anyway. Better to just leave debug information in
the
test binaries.
The reduction in size of course varies a lot depending on OS and
configuration, but as an example, on my 64bit Linux workstation
(debug-symbols=zipped) the size of the build directory has gone from
5.8GB when building "make images" before to 3.8GB when building "make
product-images-jdk". The biggest gain is from only building the jdk
image when only that is needed. A fairer comparison is "make images
test-image" which has gone from 7.3GB to 5.4GB, much due to not
duplicating debug information in test binaries. On Solaris the gain is
generally bigger.
Another nice effect of this is that the exploded image now has debug
information for all native libraries by default, and that handling of
dSYM directories on Macosx should now work properly.
The only significant changes detected by comparison builds is that
some
binaries that previously weren't stripped, now are, which they also
should be.
Bug: https://bugs.openjdk.java.net/browse/JDK-8150736
Webrev: http://cr.openjdk.java.net/~erikj/8150736/webrev.01/
/Erik
