Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
Hello, I could reproduce this and found the issue. There is a typo in the conditional. Posting review. /Erik On 2020-04-30 10:43, Florian Weimer wrote: * Magnus Ihse Bursie: On 2020-04-30 15:50, Florian Weimer wrote: * Magnus Ihse Bursie: I made sure that no build performances were measured on my system, and since I saw no such indication, I did not make any more systematic analysis. What is the difference if you run with or without the javac server? Thanks. Which configure flags do you want me to test? Four measurements: Prior to JDK-8244036: configure with --enable-javac-server and --disable-javac-server, and "make jdk-image" --enable-javac-server: 174s --disable-javac-server: 241s After JDK-8244036: configure with --enable-javac-server and --disable-javac-server, and "make jdk-image" --enable-javac-server: 247s --disable-javac-server: 249s If any of them show major differences with and without JDK-8244036, you can try running that option for more granular targets, e.g. "jdk". jdk times before: ~161s (--enable-javac-server), 231s (--disable-javac-server) jdk times after: 237s (both cases) It looks like the server is not running anymore. The build log difference seems to be this. Before: ( /bin/rm -f /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log && /usr/lib/jvm/java-14-openjdk-amd64/bin/java -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1 -Duser.language=en -Duser.country=US -XX:+UnlockDiagnosticVMOptions -XX:-VerifySharedSpaces -XX:SharedArchiveFile=/mnt/scratch1/fw/jdk/configure-support/classes.jsa -Xshare:auto --limit-modules java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --add-modules java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --module-path /mnt/scratch1/fw/jdk/buildtools/interim_langtools_modules --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim --add-exports java.base/jdk.internal.jmod=jdk.compiler.interim --add-exports java.base/jdk.internal.misc=jdk.compiler.interim -m jdk.compiler.interim/com.sun.tools.sjavac.Main --server:portfile=/mnt/scratch1/fw/jdk/make-support/javacservers/server.port,id=BUILD_TOOLS_JDK,sjavac=/usr/lib/jvm/java-14-openjdk-amd64/bin/java%20-Xms512M%20-Xmx2048M%20--limit-modules%20java.base%2Cjdk.zipfs%2Cjava.compiler.interim%2Cjdk.compiler.interim%2Cjdk.javadoc.interim%20--add-modules%20java.compiler.interim%2Cjdk.compiler.interim%2Cjdk.javadoc.interim%20--module-path%20/mnt/scratch1/fw/jdk/buildtools/interim_langtools_modules%20--add-exports%20java.base/sun.reflect.annotation=jdk.compiler.interim%20--add-exports%20java.base/jdk.internal.jmod=jdk.compiler.interim%20--add-exports%20java.base/jdk.internal.misc=jdk.compiler.interim%20-m%20jdk.compiler.interim/com.sun.tools.sjavac.Main -g -source 14 -target 14 -XDignore.symbol.file=true -XDstringConcat=inline -Xlint:all -Werror -Xlint:-options --add-exports java.desktop/sun.awt=ALL-UNNAMED --add-exports java.base/sun.text=ALL-UNNAMED -implicit:none -d /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes @/mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.tmp > >(/usr/bin/tee -a /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log) 2> >(/usr/bin/tee -a /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log >&2) || ( exitcode=$? && /bin/cp /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log /mnt/scratch1/fw/jdk/make-support/failure-logs/buildtools_jdk_tools_classes__the.BUILD_TOOLS_JDK_batch.log && /bin/cp /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.cmdline /mnt/scratch1/fw/jdk/make-support/failure-logs/buildtools_jdk_tools_classes__the.BUILD_TOOLS_JDK_batch.cmdline && exit $exitcode ) ) && /bin/mv /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.tmp /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch After: ( /bin/rm -f /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log && /usr/lib/jvm/java-14-openjdk-amd64/bin/java -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1 -Duser.language=en -Duser.country=US -XX:+UnlockDiagnosticVMOptions -XX:-VerifySharedSpaces -XX:SharedArchiveFile=/mnt/scratch1/fw/jdk/configure-support/classes.jsa -Xshare:auto --limit-modules java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --add-modules java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --module-path /mnt/scratch1/fw/jdk/buildtools/interim_langtools_modules --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim --add-exports java.base/jdk.internal.jmod=jdk.compiler.interim --add-exports java.base/jdk.internal.misc=jdk.compiler.interim -m jdk.compiler.interim/com.sun.tools.javac.Main -g -Xlint:all --doclint-format html5 -source 14 -target 14 -implicit:none -Xprefer:source -XDignore.symbol.file=true -encoding ascii
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
* Magnus Ihse Bursie: > On 2020-04-30 15:50, Florian Weimer wrote: >> * Magnus Ihse Bursie: >> >>> I made sure that no build performances were measured on my system, and >>> since I saw no such indication, I did not make any more systematic analysis. >>> >>> What is the difference if you run with or without the javac server? >> Thanks. Which configure flags do you want me to test? >> > Four measurements: > > Prior to JDK-8244036: configure with --enable-javac-server and > --disable-javac-server, and "make jdk-image" --enable-javac-server: 174s --disable-javac-server: 241s > After JDK-8244036: configure with --enable-javac-server and > --disable-javac-server, and "make jdk-image" --enable-javac-server: 247s --disable-javac-server: 249s > If any of them show major differences with and without JDK-8244036, you > can try running that option for more granular targets, e.g. "jdk". jdk times before: ~161s (--enable-javac-server), 231s (--disable-javac-server) jdk times after: 237s (both cases) It looks like the server is not running anymore. The build log difference seems to be this. Before: ( /bin/rm -f /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log && /usr/lib/jvm/java-14-openjdk-amd64/bin/java -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1 -Duser.language=en -Duser.country=US -XX:+UnlockDiagnosticVMOptions -XX:-VerifySharedSpaces -XX:SharedArchiveFile=/mnt/scratch1/fw/jdk/configure-support/classes.jsa -Xshare:auto --limit-modules java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --add-modules java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --module-path /mnt/scratch1/fw/jdk/buildtools/interim_langtools_modules --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim --add-exports java.base/jdk.internal.jmod=jdk.compiler.interim --add-exports java.base/jdk.internal.misc=jdk.compiler.interim -m jdk.compiler.interim/com.sun.tools.sjavac.Main --server:portfile=/mnt/scratch1/fw/jdk/make-support/javacservers/server.port,id=BUILD_TOOLS_JDK,sjavac=/usr/lib/jvm/java-14-openjdk-amd64/bin/java%20-Xms512M%20-Xmx2048M%20--limit-modules%20java.base%2Cjdk.zipfs%2Cjava.compiler.interim%2Cjdk.compiler.interim%2Cjdk.javadoc.interim%20--add-modules%20java.compiler.interim%2Cjdk.compiler.interim%2Cjdk.javadoc.interim%20--module-path%20/mnt/scratch1/fw/jdk/buildtools/interim_langtools_modules%20--add-exports%20java.base/sun.reflect.annotation=jdk.compiler.interim%20--add-exports%20java.base/jdk.internal.jmod=jdk.compiler.interim%20--add-exports%20java.base/jdk.internal.misc=jdk.compiler.interim%20-m%20jdk.compiler.interim/com.sun.tools.sjavac.Main -g -source 14 -target 14 -XDignore.symbol.file=true -XDstringConcat=inline -Xlint:all -Werror -Xlint:-options --add-exports java.desktop/sun.awt=ALL-UNNAMED --add-exports java.base/sun.text=ALL-UNNAMED -implicit:none -d /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes @/mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.tmp > >(/usr/bin/tee -a /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log) 2> >(/usr/bin/tee -a /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log >&2) || ( exitcode=$? && /bin/cp /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log /mnt/scratch1/fw/jdk/make-support/failure-logs/buildtools_jdk_tools_classes__the.BUILD_TOOLS_JDK_batch.log && /bin/cp /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.cmdline /mnt/scratch1/fw/jdk/make-support/failure-logs/buildtools_jdk_tools_classes__the.BUILD_TOOLS_JDK_batch.cmdline && exit $exitcode ) ) && /bin/mv /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.tmp /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch After: ( /bin/rm -f /mnt/scratch1/fw/jdk/buildtools/jdk_tools_classes/_the.BUILD_TOOLS_JDK_batch.log && /usr/lib/jvm/java-14-openjdk-amd64/bin/java -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1 -Duser.language=en -Duser.country=US -XX:+UnlockDiagnosticVMOptions -XX:-VerifySharedSpaces -XX:SharedArchiveFile=/mnt/scratch1/fw/jdk/configure-support/classes.jsa -Xshare:auto --limit-modules java.base,jdk.zipfs,java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --add-modules java.compiler.interim,jdk.compiler.interim,jdk.javadoc.interim --module-path /mnt/scratch1/fw/jdk/buildtools/interim_langtools_modules --add-exports java.base/sun.reflect.annotation=jdk.compiler.interim --add-exports java.base/jdk.internal.jmod=jdk.compiler.interim --add-exports java.base/jdk.internal.misc=jdk.compiler.interim -m jdk.compiler.interim/com.sun.tools.javac.Main -g -Xlint:all --doclint-format html5 -source 14 -target 14 -implicit:none -Xprefer:source -XDignore.symbol.file=true -encoding ascii -Werror --add-exports java.desktop/sun.awt=ALL-UNNAMED --add-exports java.base/sun.text=ALL-UNNAMED -Xlint:-options -d /mnt/
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
On 2020-04-30 15:50, Florian Weimer wrote: * Magnus Ihse Bursie: I made sure that no build performances were measured on my system, and since I saw no such indication, I did not make any more systematic analysis. What is the difference if you run with or without the javac server? Thanks. Which configure flags do you want me to test? Four measurements: Prior to JDK-8244036: configure with --enable-javac-server and --disable-javac-server, and "make jdk-image" After JDK-8244036: configure with --enable-javac-server and --disable-javac-server, and "make jdk-image" Make sure you purge the build directory between runs. If any of them show major differences with and without JDK-8244036, you can try running that option for more granular targets, e.g. "jdk". /Magnus
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
* Magnus Ihse Bursie: > I made sure that no build performances were measured on my system, and > since I saw no such indication, I did not make any more systematic analysis. > > What is the difference if you run with or without the javac server? Thanks. Which configure flags do you want me to test?
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
On 2020-04-30 15:07, Magnus Ihse Bursie wrote:
On 2020-04-30 14:55, Florian Weimer wrote:
* Magnus Ihse Bursie:
The code for setting up Java compilation has long been quite hard to
understand, and has a tricky API. Part of this is due to the support
for
the sjavac ("smart javac") system. We do not use sjavac anymore, and it
has not been tested for long. Part of the sjavac effort was extracted
into the "depend" javac plugin, and another part ended up as the java
server, both of which we still use.
This commit:
# HG changeset patch
# User ihse
# Date 1588142957 -7200
# Wed Apr 29 08:49:17 2020 +0200
# Node ID a8e4856be54f73225780a56b2b6ee9cc1e12ab8f
# Parent f53590a82709caf92213106954eab2bdc97cc498
8244036: Refresh SetupJavaCompilation, and remove support for sjavac
Reviewed-by: erikj
appears to have regressed build time (“time make jdk-image”) by
roughly one third, on an x86-64 machine with 8 cores/16 threads.
The configure arguments are “--with-zlib=system --with-lcms=system”,
so nothing sjavac-related as far as I can see.
Is this expected?
No, that is very much not expected. :-(
I made sure that no build performances were measured on my system,
"... no build performance *regressions* ..."
/Magnus
and since I saw no such indication, I did not make any more systematic
analysis.
What is the difference if you run with or without the javac server?
Both prior to and after this commit. Do you have any numbers to share?
/Magnus
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
On 2020-04-30 14:55, Florian Weimer wrote:
* Magnus Ihse Bursie:
The code for setting up Java compilation has long been quite hard to
understand, and has a tricky API. Part of this is due to the support for
the sjavac ("smart javac") system. We do not use sjavac anymore, and it
has not been tested for long. Part of the sjavac effort was extracted
into the "depend" javac plugin, and another part ended up as the java
server, both of which we still use.
This commit:
# HG changeset patch
# User ihse
# Date 1588142957 -7200
# Wed Apr 29 08:49:17 2020 +0200
# Node ID a8e4856be54f73225780a56b2b6ee9cc1e12ab8f
# Parent f53590a82709caf92213106954eab2bdc97cc498
8244036: Refresh SetupJavaCompilation, and remove support for sjavac
Reviewed-by: erikj
appears to have regressed build time (“time make jdk-image”) by
roughly one third, on an x86-64 machine with 8 cores/16 threads.
The configure arguments are “--with-zlib=system --with-lcms=system”,
so nothing sjavac-related as far as I can see.
Is this expected?
No, that is very much not expected. :-(
I made sure that no build performances were measured on my system, and
since I saw no such indication, I did not make any more systematic analysis.
What is the difference if you run with or without the javac server? Both
prior to and after this commit. Do you have any numbers to share?
/Magnus
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
* Magnus Ihse Bursie:
> The code for setting up Java compilation has long been quite hard to
> understand, and has a tricky API. Part of this is due to the support for
> the sjavac ("smart javac") system. We do not use sjavac anymore, and it
> has not been tested for long. Part of the sjavac effort was extracted
> into the "depend" javac plugin, and another part ended up as the java
> server, both of which we still use.
This commit:
# HG changeset patch
# User ihse
# Date 1588142957 -7200
# Wed Apr 29 08:49:17 2020 +0200
# Node ID a8e4856be54f73225780a56b2b6ee9cc1e12ab8f
# Parent f53590a82709caf92213106954eab2bdc97cc498
8244036: Refresh SetupJavaCompilation, and remove support for sjavac
Reviewed-by: erikj
appears to have regressed build time (“time make jdk-image”) by
roughly one third, on an x86-64 machine with 8 cores/16 threads.
The configure arguments are “--with-zlib=system --with-lcms=system”,
so nothing sjavac-related as far as I can see.
Is this expected?
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
On 2020-04-29 00:35, Erik Joelsson wrote:
Hello Magnus,
Nice job! Looks good.
Is the disabling of the warning "options" actually needed in that many
places or did you just add it everywhere where GENERATE_OLDBYTECODE
was used?
It is really needed -- I started out without it, and added it wherever
the warning crept up.
My next step is to go over each java compilation and scrutinize them.
Hopefully I can tune the options to get rid of this warning. That might
mean that the output class files can change though, so I need to make
sure that it's okay.
Minor notes:
JavaCompilation.gmk
39: spelling
206: do -> does
Thanks for catching that! I fixed it before I pushed.
/Magnus
/Erik
On 2020-04-28 13:37, Magnus Ihse Bursie wrote:
The code for setting up Java compilation has long been quite hard to
understand, and has a tricky API. Part of this is due to the support
for the sjavac ("smart javac") system. We do not use sjavac anymore,
and it has not been tested for long. Part of the sjavac effort was
extracted into the "depend" javac plugin, and another part ended up
as the java server, both of which we still use.
This patch removes the "traditional" sjavac support, keeping just the
depend plugin and javac server.
It also redesigns the SetupJavaCompilation API on how to define which
compiler to use (from the boot jdk or the interim compiler built as
part of the buildtools), and what JDK to target. This captures more
precisely what was expressed by the cryptic "Setups" in
SetupJavaCompilers.gmk.
The generated Java code that goes into the product should be
byte-by-byte identical with this patch. However, I've not applied the
same level of strictness for our build tools, where I have accepted
changes in bytecode to make the new API calls simpler. This does not
affect the output from the build tools.
I imagine a second pass will be needed after this, to clear up some
remaining stuff. For instance, it is not clear that all instances of
SetupJavaCompilation really ask for the correct thing.
Bug: https://bugs.openjdk.java.net/browse/JDK-8244036
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8244036-remove-sjavac/webrev.01
/Magnus
Re: RFR: JDK-8244036 Refresh SetupJavaCompilation, and remove support for sjavac
Hello Magnus,
Nice job! Looks good.
Is the disabling of the warning "options" actually needed in that many
places or did you just add it everywhere where GENERATE_OLDBYTECODE was
used?
Minor notes:
JavaCompilation.gmk
39: spelling
206: do -> does
/Erik
On 2020-04-28 13:37, Magnus Ihse Bursie wrote:
The code for setting up Java compilation has long been quite hard to
understand, and has a tricky API. Part of this is due to the support
for the sjavac ("smart javac") system. We do not use sjavac anymore,
and it has not been tested for long. Part of the sjavac effort was
extracted into the "depend" javac plugin, and another part ended up as
the java server, both of which we still use.
This patch removes the "traditional" sjavac support, keeping just the
depend plugin and javac server.
It also redesigns the SetupJavaCompilation API on how to define which
compiler to use (from the boot jdk or the interim compiler built as
part of the buildtools), and what JDK to target. This captures more
precisely what was expressed by the cryptic "Setups" in
SetupJavaCompilers.gmk.
The generated Java code that goes into the product should be
byte-by-byte identical with this patch. However, I've not applied the
same level of strictness for our build tools, where I have accepted
changes in bytecode to make the new API calls simpler. This does not
affect the output from the build tools.
I imagine a second pass will be needed after this, to clear up some
remaining stuff. For instance, it is not clear that all instances of
SetupJavaCompilation really ask for the correct thing.
Bug: https://bugs.openjdk.java.net/browse/JDK-8244036
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8244036-remove-sjavac/webrev.01
/Magnus
