Re: RFR 4947890 : Minimize JNI upcalls in system property initialization
Hi Roger, Looking good. I have a few small comments: I assume VM.saveAndRemoveProperties will be a separate cleanup. SystemProps::cmdProperties adds the system properties that can skip adding these internal properties to the Properties object. In fact, these internal properties are the interface between VM and libraries that can be replaced with a different mechanism other than system properties. Suggest to move the call to VersionProps.init(props) in SystemProps.initProperties The `VENDOR*` build time variables can always have a default like the other constants in VersionProps. Then no need to handle if not set. SystemProps.staticInitOnly_* are a bit odd. They are temporarily set with the values and then reset to null. Perhaps leave StaticProperty as is for now and re-visit it in the future. Then I think SystemProps::putDefault can be removed. Raw::xxx_NDX are initialized to 1 + previous_NDX. It's a general good approach to increment the index but I find it error-prone and hard to catch mistake since the (adjacent) variable names look so alike. Perhaps some form of verification or assertion to ensure the indices are correctly initialized. Mandy On 11/16/18 10:49 AM, Roger Riggs wrote: Updates to include the suggestions made by Mandy and Brent: - Move the build-time properties from native code to the VersionProps.java.template including java.vendor.* and java.specification.* properties, plus the java.class.version (major.minor) This included two changes to the build that generates source of VersionProps.java. - Updated the StaticProperty initialization to be explicitly invoked by initProperties. - Makes separate calls to native to retrieve the platform properties and VM/command line properties - (The hotspot function for JVM_GetProperties are unchanged) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ Issue: https://bugs.openjdk.java.net/browse/JDK-4947890 Thanks, Roger
Re: [PATCH] Windows 32-bit DLL name decoration
Hi Ali, It's exactly what I referred to. Yes, it does change the calling convention. Yet it's not a (big) problem because both parties will use the same calling convention. Using a DLL from another build will not be possible. But it's not a good idea to do so anyway. Mapfiles and export options have been removed by https://bugs.openjdk.java.net/browse/JDK-8200178 to simplify managing exported functions. So that to add or remove an exported function, you don't have modify makefiles and/or mapfiles. You just edit the source files. Regards, Alexey On 16/11/2018 22:42, Ali İnce wrote: Hi Alexey, Thanks for your reply. I will definitely give it a try though I’m not sure if that’ll be a breaking change. This is because JNICALL modifier is defined to be __stdcall on windows which is both specified on jdwpTransport_OnLoad exported functions both for dt_socket.dll and dt_shmem.dll. The __stdcall is ignored on x64 platforms and also name decoration is not applied (https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017). I believe that’s why we don’t experience any problems on x64 builds. Removing JNICALL (thus __stdcall) modifier (apparently only applies to x86 builds) will result in the calling convention to be changed, and thus will change how parameters are ordered and also the stack cleanup rules. I think this may result in problems in programs that are designed to load shared libraries and its exported functions at runtime (not bound by link time which probably won’t cause any issues - assuming that they use the shared function signature) - as in https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99. Any thoughts? Regards, Ali On 15 Nov 2018, at 22:14, Alexey Ivanov wrote: Hi Ali, Can the issue be resolved by using different call modifiers on the affected functions? Some build / link issues were resolved by adding / removing JNICALL modifier, see https://bugs.openjdk.java.net/browse/JDK-8201226 http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html https://bugs.openjdk.java.net/browse/JDK-8202476 http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html Regards, Alexey On 15/11/2018 21:43, Ali İnce wrote: Hi, An issue (https://github.com/AdoptOpenJDK/openjdk-build/issues/709) raised against AdoptOpenJDK jdk11 32-bit windows builds led us to the name decoration problem on built 32-bit dlls - specifically dt_socket.dll and dt_shmem.dll. Whereas 64-bit MSVC builds don’t apply any name decorations on exported functions, 32-bit builds apply them - which requires either def files or /export flags to be specified on the linker arguments. Although the expected linker arguments were present on previous jdk makefiles, they were removed in jdk11 makefiles. Below is the proposed patch, which can also be browsed at https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1. Would you please have a look and let me know of any points I may have missed (I don’t have any background information about why the export flags were removed in jdk11)? Regards, Ali diff --git a/make/lib/Lib-jdk.jdi.gmk b/make/lib/Lib-jdk.jdi.gmk index 197b95c2e2..ac6ebf5591 100644 --- a/make/lib/Lib-jdk.jdi.gmk +++ b/make/lib/Lib-jdk.jdi.gmk @@ -37,6 +37,7 @@ ifeq ($(OPENJDK_TARGET_OS), windows) jdk.jdwp.agent:include \ jdk.jdwp.agent:libjdwp/export, \ LDFLAGS := $(LDFLAGS_JDKLIB), \ + LDFLAGS_windows := -export:jdwpTransport_OnLoad, \ LIBS := $(JDKLIB_LIBS), \ )) diff --git a/make/lib/Lib-jdk.jdwp.agent.gmk b/make/lib/Lib-jdk.jdwp.agent.gmk index 0bc93e0d35..0382e55325 100644 --- a/make/lib/Lib-jdk.jdwp.agent.gmk +++ b/make/lib/Lib-jdk.jdwp.agent.gmk @@ -37,6 +37,7 @@ $(eval $(call SetupJdkLibrary, BUILD_LIBDT_SOCKET, \ libjdwp/export, \ LDFLAGS := $(LDFLAGS_JDKLIB) \ $(call SET_SHARED_LIBRARY_ORIGIN), \ +LDFLAGS_windows := -export:jdwpTransport_OnLoad, \ LIBS_linux := -lpthread, \ LIBS_solaris := -lnsl -lsocket, \ LIBS_windows := $(JDKLIB_LIBS) ws2_32.lib, \
Re: [PATCH] Windows 32-bit DLL name decoration
Hi Alexey, Thanks for your reply. I will definitely give it a try though I’m not sure if that’ll be a breaking change. This is because JNICALL modifier is defined to be __stdcall on windows which is both specified on jdwpTransport_OnLoad exported functions both for dt_socket.dll and dt_shmem.dll. The __stdcall is ignored on x64 platforms and also name decoration is not applied (https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017). I believe that’s why we don’t experience any problems on x64 builds. Removing JNICALL (thus __stdcall) modifier (apparently only applies to x86 builds) will result in the calling convention to be changed, and thus will change how parameters are ordered and also the stack cleanup rules. I think this may result in problems in programs that are designed to load shared libraries and its exported functions at runtime (not bound by link time which probably won’t cause any issues - assuming that they use the shared function signature) - as in https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99. Any thoughts? Regards, Ali > On 15 Nov 2018, at 22:14, Alexey Ivanov wrote: > > Hi Ali, > > Can the issue be resolved by using different call modifiers on the affected > functions? > > Some build / link issues were resolved by adding / removing JNICALL modifier, > see > https://bugs.openjdk.java.net/browse/JDK-8201226 > http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html > > https://bugs.openjdk.java.net/browse/JDK-8202476 > http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html > > Regards, > Alexey > > On 15/11/2018 21:43, Ali İnce wrote: >> Hi, >> >> An issue (https://github.com/AdoptOpenJDK/openjdk-build/issues/709) raised >> against AdoptOpenJDK jdk11 32-bit windows builds led us to the name >> decoration problem on built 32-bit dlls - specifically dt_socket.dll and >> dt_shmem.dll. Whereas 64-bit MSVC builds don’t apply any name decorations on >> exported functions, 32-bit builds apply them - which requires either def >> files or /export flags to be specified on the linker arguments. >> >> Although the expected linker arguments were present on previous jdk >> makefiles, they were removed in jdk11 makefiles. >> >> Below is the proposed patch, which can also be browsed at >> https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1. >> >> Would you please have a look and let me know of any points I may have missed >> (I don’t have any background information about why the export flags were >> removed in jdk11)? >> >> Regards, >> >> Ali >> >> diff --git a/make/lib/Lib-jdk.jdi.gmk b/make/lib/Lib-jdk.jdi.gmk >> index 197b95c2e2..ac6ebf5591 100644 >> --- a/make/lib/Lib-jdk.jdi.gmk >> +++ b/make/lib/Lib-jdk.jdi.gmk >> @@ -37,6 +37,7 @@ ifeq ($(OPENJDK_TARGET_OS), windows) >>jdk.jdwp.agent:include \ >>jdk.jdwp.agent:libjdwp/export, \ >>LDFLAGS := $(LDFLAGS_JDKLIB), \ >> + LDFLAGS_windows := -export:jdwpTransport_OnLoad, \ >>LIBS := $(JDKLIB_LIBS), \ >>)) >> >> diff --git a/make/lib/Lib-jdk.jdwp.agent.gmk >> b/make/lib/Lib-jdk.jdwp.agent.gmk >> index 0bc93e0d35..0382e55325 100644 >> --- a/make/lib/Lib-jdk.jdwp.agent.gmk >> +++ b/make/lib/Lib-jdk.jdwp.agent.gmk >> @@ -37,6 +37,7 @@ $(eval $(call SetupJdkLibrary, BUILD_LIBDT_SOCKET, \ >> libjdwp/export, \ >> LDFLAGS := $(LDFLAGS_JDKLIB) \ >> $(call SET_SHARED_LIBRARY_ORIGIN), \ >> +LDFLAGS_windows := -export:jdwpTransport_OnLoad, \ >> LIBS_linux := -lpthread, \ >> LIBS_solaris := -lnsl -lsocket, \ >> LIBS_windows := $(JDKLIB_LIBS) ws2_32.lib, \ >
Re: RFR 4947890 : Minimize JNI upcalls in system property initialization
Thanks, looks good to me now. /Erik On 2018-11-16 12:02, Roger Riggs wrote: Hi Erik, Yes, that is removed. Webrev updated in place. Thanks, Roger http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ On 11/16/2018 02:37 PM, Erik Joelsson wrote: Does this mean we can remove $(VERSION_CFLAGS) from System.c in make/lib/CoreLibraries.gmk? Otherwise this looks good from a build point of view. /Erik On 2018-11-16 10:49, Roger Riggs wrote: Updates to include the suggestions made by Mandy and Brent: - Move the build-time properties from native code to the VersionProps.java.template including java.vendor.* and java.specification.* properties, plus the java.class.version (major.minor) This included two changes to the build that generates source of VersionProps.java. - Updated the StaticProperty initialization to be explicitly invoked by initProperties. - Makes separate calls to native to retrieve the platform properties and VM/command line properties - (The hotspot function for JVM_GetProperties are unchanged) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ Issue: https://bugs.openjdk.java.net/browse/JDK-4947890 Thanks, Roger
Re: [PATCH] JDK-8167368 Leftover: get_source.sh in build documentation
Looks good to me. /Erik On 2018-11-16 12:20, Sergey wrote: Erik, The whole section about trees should be removed. That extension is just for managing a forest of repositories, which we no longer do. /Erik Whoops, thanks for pointing that out! I've fixed it. diff --git a/doc/building.md b/doc/building.md --- a/doc/building.md +++ b/doc/building.md @@ -48,7 +48,7 @@ Make sure you are getting the correct version. As of JDK 10, the source is no longer split into separate repositories so you only need to clone one single repository. At the [OpenJDK Mercurial server](http://hg.openjdk.java.net/) you -can see a list of all available forests. If you want to build an older version, +can see a list of all available repositorys. If you want to build an older version, e.g. JDK 8, it is recommended that you get the `jdk8u` forest, which contains incremental updates, instead of the `jdk8` forest, which was frozen at JDK 8 GA. @@ -1301,17 +1301,15 @@ affected parts get rebuilt. While this works great in most cases, and significantly speed up the development process, from time to time complex interdependencies will result in an incorrect build result. This is the most -common cause for unexpected build problems, together with inconsistencies -between the different Mercurial repositories in the forest. +common cause for unexpected build problems. Here are a suggested list of things to try if you are having unexpected build problems. Each step requires more time than the one before, so try them in order. Most issues will be solved at step 1 or 2. - 1. Make sure your forest is up-to-date + 1. Make sure your repository is up-to-date - Run `bash get_source.sh` to make sure you have the latest version of all - repositories. + Run `hg pull -u` to make sure you have the latest changes. 2. Clean build results @@ -1336,13 +1334,13 @@ make ``` - 4. Re-clone the Mercurial forest + 4. Re-clone the Mercurial repository - Sometimes the Mercurial repositories themselves gets in a state that causes - the product to be un-buildable. In such a case, the simplest solution is - often the "sledgehammer approach": delete the entire forest, and re-clone - it. If you have local changes, save them first to a different location - using `hg export`. + Sometimes the Mercurial repository gets in a state that causes the product + to be un-buildable. In such a case, the simplest solution is often the + "sledgehammer approach": delete the entire repository, and re-clone it. + If you have local changes, save them first to a different location using + `hg export`. ### Specific Build Issues @@ -1393,7 +1391,7 @@ ## Hints and Suggestions for Advanced Users -### Setting Up a Forest for Pushing Changes (defpath) +### Setting Up a Repository for Pushing Changes (defpath) To help you prepare a proper push path for a Mercurial repository, there exists a useful tool known as [defpath]( @@ -1420,11 +1418,6 @@ hg defpath -d -u ``` -If you also have the `trees` extension installed in Mercurial, you will -automatically get a `tdefpath` command, which is even more useful. By running -`hg tdefpath -du ` in the top repository of your forest, all repos -will get setup automatically. This is the recommended usage. - ### Bash Completion The `configure` and `make` commands tries to play nice with bash command-line @@ -1459,7 +1452,7 @@ ### Using Multiple Configurations -You can have multiple configurations for a single source forest. When you +You can have multiple configurations for a single source repository. When you create a new configuration, run `configure --with-conf-name=` to create a configuration with the name ``. Alternatively, you can create a directory under `build` and run `configure` from there, e.g. `mkdir build/ && cd @@ -1474,7 +1467,7 @@ ### Handling Reconfigurations -If you update the forest and part of the configure script has changed, the +If you update the repository and part of the configure script has changed, the build system will force you to re-run `configure`. Most of the time, you will be fine by running `configure` again with the same
Re: [PATCH] JDK-8167368 Leftover: get_source.sh in build documentation
Erik, > The whole section about trees should be removed. That extension is just > for managing a forest of repositories, which we no longer do. > > /Erik > > Whoops, thanks for pointing that out! I've fixed it. diff --git a/doc/building.md b/doc/building.md --- a/doc/building.md +++ b/doc/building.md @@ -48,7 +48,7 @@ Make sure you are getting the correct version. As of JDK 10, the source is no longer split into separate repositories so you only need to clone one single repository. At the [OpenJDK Mercurial server](http://hg.openjdk.java.net/) you -can see a list of all available forests. If you want to build an older version, +can see a list of all available repositorys. If you want to build an older version, e.g. JDK 8, it is recommended that you get the `jdk8u` forest, which contains incremental updates, instead of the `jdk8` forest, which was frozen at JDK 8 GA. @@ -1301,17 +1301,15 @@ affected parts get rebuilt. While this works great in most cases, and significantly speed up the development process, from time to time complex interdependencies will result in an incorrect build result. This is the most -common cause for unexpected build problems, together with inconsistencies -between the different Mercurial repositories in the forest. +common cause for unexpected build problems. Here are a suggested list of things to try if you are having unexpected build problems. Each step requires more time than the one before, so try them in order. Most issues will be solved at step 1 or 2. - 1. Make sure your forest is up-to-date + 1. Make sure your repository is up-to-date -Run `bash get_source.sh` to make sure you have the latest version of all -repositories. +Run `hg pull -u` to make sure you have the latest changes. 2. Clean build results @@ -1336,13 +1334,13 @@ make ``` - 4. Re-clone the Mercurial forest + 4. Re-clone the Mercurial repository -Sometimes the Mercurial repositories themselves gets in a state that causes -the product to be un-buildable. In such a case, the simplest solution is -often the "sledgehammer approach": delete the entire forest, and re-clone -it. If you have local changes, save them first to a different location -using `hg export`. +Sometimes the Mercurial repository gets in a state that causes the product +to be un-buildable. In such a case, the simplest solution is often the +"sledgehammer approach": delete the entire repository, and re-clone it. +If you have local changes, save them first to a different location using +`hg export`. ### Specific Build Issues @@ -1393,7 +1391,7 @@ ## Hints and Suggestions for Advanced Users -### Setting Up a Forest for Pushing Changes (defpath) +### Setting Up a Repository for Pushing Changes (defpath) To help you prepare a proper push path for a Mercurial repository, there exists a useful tool known as [defpath]( @@ -1420,11 +1418,6 @@ hg defpath -d -u ``` -If you also have the `trees` extension installed in Mercurial, you will -automatically get a `tdefpath` command, which is even more useful. By running -`hg tdefpath -du ` in the top repository of your forest, all repos -will get setup automatically. This is the recommended usage. - ### Bash Completion The `configure` and `make` commands tries to play nice with bash command-line @@ -1459,7 +1452,7 @@ ### Using Multiple Configurations -You can have multiple configurations for a single source forest. When you +You can have multiple configurations for a single source repository. When you create a new configuration, run `configure --with-conf-name=` to create a configuration with the name ``. Alternatively, you can create a directory under `build` and run `configure` from there, e.g. `mkdir build/ && cd @@ -1474,7 +1467,7 @@ ### Handling Reconfigurations -If you update the forest and part of the configure script has changed, the +If you update the repository and part of the configure script has changed, the build system will force you to re-run `configure`. Most of the time, you will be fine by running `configure` again with the same
Re: RFR 4947890 : Minimize JNI upcalls in system property initialization
Hi Erik, Yes, that is removed. Webrev updated in place. Thanks, Roger http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ On 11/16/2018 02:37 PM, Erik Joelsson wrote: Does this mean we can remove $(VERSION_CFLAGS) from System.c in make/lib/CoreLibraries.gmk? Otherwise this looks good from a build point of view. /Erik On 2018-11-16 10:49, Roger Riggs wrote: Updates to include the suggestions made by Mandy and Brent: - Move the build-time properties from native code to the VersionProps.java.template including java.vendor.* and java.specification.* properties, plus the java.class.version (major.minor) This included two changes to the build that generates source of VersionProps.java. - Updated the StaticProperty initialization to be explicitly invoked by initProperties. - Makes separate calls to native to retrieve the platform properties and VM/command line properties - (The hotspot function for JVM_GetProperties are unchanged) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ Issue: https://bugs.openjdk.java.net/browse/JDK-4947890 Thanks, Roger
Re: RFR: 8061282: Migrate jmh-jdk-microbenchmarks into the JDK
Looks good. A small comment: make test TEST=micro always builds the native libraries for the tests. It'd be nice if running microbenchmarks does not depend on the target of building native test libraries. Perhaps you can file a RFE as a follow up. Mandy On 11/16/18 4:43 AM, Claes Redestad wrote: Hi Mandy, I've updated copyright headers of all microbenchmarks to use the correct one, and fixed a few instances of long lines that stood out. We can do more cleanups as a follow-up. Currently no microbenchmark includes any resources, and as you say the current convention is to keep resources co-located. I've removed the classes and resources folders, and updated makefiles with the trivial changes needed for this. The JEP text also referred to `make run-test`, which is now just `make test`. Updated. To help set thing up JDK-8061281 includes examples and setup instructions in docs/testing.md|html - I've added a reference to this documentation in the JEP text. I think using the JEP as the authoritative place to document usage wouldn't age well, as instructions typically evolve and change over time. Combined webrev for 8061281 and 8061282 here: http://cr.openjdk.java.net/~redestad/8061281_8061282.01/ Thanks! /Claes On 2018-11-15 23:50, Mandy Chung wrote: Hi Claes, It's good to see this JEP targeted and integrate the microbenchmarks to colocate with JDK. Overall the work looks good. The copyright headers need update to GPL. There are some super long lines (mostly looking up method handles), for example: + MethodHandle bodyNormal = MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, "doWorkNormal", MethodType.methodType(void.class, MethodHandlesCatchException.class)); + MethodHandle bodyExceptional = MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, "doWorkExceptional", MethodType.methodType(void.class, MethodHandlesCatchException.class)); + MethodHandle fallback = MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, "fallback", MethodType.methodType(void.class, MyException.class, MethodHandlesCatchException.class)); JEP 230 proposes to separate the resources from the source files in micro/classes and micro/resources directories. What kinds of resources are expected to be placed under micro/resources directory? If they are java resources, then I would expect them follow the consistent layout as JDK source tree where the java classes and resources are placed together. I was trying to experiment building and running the benchmarks. What does configure --with-jmh expect to contain? I can't quite figure it out from the error message. The JEP describes make build-microbenchmark and run-test targets to build and execute the microbenchmarks. It'd also be helpful to update the JEP to include an example how to run a specific set of benchmarks e.g. org.openjdk.bench.java.lang.ObjectHashCode and how to run the benchmarks with JDK n and JDK n-1 and compare the result (is there a build target to do this)? We can reference this JEP to get started running the microbenchmark and refer to JMH and other documentation for other details like developing a JMH benchmark. Mandy On 10/18/18 2:03 PM, Claes Redestad wrote: Hi, as the final part of JEP 230: Microbenchmarks Suite, I propose migrating all microbenchmarks from the codetools jmh-jdk-microbenchmarks project into the JDK: http://cr.openjdk.java.net/~redestad/8061282/jdk.00/ This is built on top of the patch for JDK-8061281, and makes the entirety of this suite readily available to build, run and experiment with from the main jdk repos. While the future of the codetools jmh-jdk-microbenchmarks project is out of scope for JEP 230, it has been suggested it could be kept alive as a stabilization target and that stable microbenchmarks should be kept out of the jdk. That discussion is partly out of scope here, but I believe it makes sense to keep a copy in the JDK suite precisely because the benchmark will be compiled with the platform javac, meaning a different set of bugs, regressions and improvements will be discoverable. Two micros, org.openjdk.bench.java.lang.invoke.Indify and org.opendjk.bench.java.lang.reflect.GetAnnotation need special build treatment and will need to be dealt with in a follow-up. Thanks! /Claes
Re: RFR 4947890 : Minimize JNI upcalls in system property initialization
Does this mean we can remove $(VERSION_CFLAGS) from System.c in make/lib/CoreLibraries.gmk? Otherwise this looks good from a build point of view. /Erik On 2018-11-16 10:49, Roger Riggs wrote: Updates to include the suggestions made by Mandy and Brent: - Move the build-time properties from native code to the VersionProps.java.template including java.vendor.* and java.specification.* properties, plus the java.class.version (major.minor) This included two changes to the build that generates source of VersionProps.java. - Updated the StaticProperty initialization to be explicitly invoked by initProperties. - Makes separate calls to native to retrieve the platform properties and VM/command line properties - (The hotspot function for JVM_GetProperties are unchanged) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ Issue: https://bugs.openjdk.java.net/browse/JDK-4947890 Thanks, Roger
Re: RFR(XXS): 8214007: Fix sun.awt.nativedebug on X11 platforms
Looks ok to me. /Erik On 2018-11-16 10:08, Volker Simonis wrote: Hi, can I please have a review for the following trivial fix: http://cr.openjdk.java.net/~simonis/webrevs/2018/8214007/ https://bugs.openjdk.java.net/browse/JDK-8214007 AWT supports some kind of native logging which can be enabled with "-Dsun.awt.nativedebug=true -Dawtdebug.ctrace=true". Unfortunately this doesn't work on X platforms any more because both, libawt and libawt_xawt end up with a copy of debug_trace.o. Among others, debug_trace.o contains the static variable GlobalTracingEnabled which denotes the tracing state. This obviously can't work if the final executable contains several instances of this variable. The fix is trivial. Remove "common/awt/debug" from the set of sources for libawt_xawt. libawt_xawt is linked against libawt (which already contains debug_trace.o) anyway, so this is no problem. Thank you and best regards, Volker
RFR 4947890 : Minimize JNI upcalls in system property initialization
Updates to include the suggestions made by Mandy and Brent: - Move the build-time properties from native code to the VersionProps.java.template including java.vendor.* and java.specification.* properties, plus the java.class.version (major.minor) This included two changes to the build that generates source of VersionProps.java. - Updated the StaticProperty initialization to be explicitly invoked by initProperties. - Makes separate calls to native to retrieve the platform properties and VM/command line properties - (The hotspot function for JVM_GetProperties are unchanged) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/ Issue: https://bugs.openjdk.java.net/browse/JDK-4947890 Thanks, Roger
RFR(XXS): 8214007: Fix sun.awt.nativedebug on X11 platforms
Hi, can I please have a review for the following trivial fix: http://cr.openjdk.java.net/~simonis/webrevs/2018/8214007/ https://bugs.openjdk.java.net/browse/JDK-8214007 AWT supports some kind of native logging which can be enabled with "-Dsun.awt.nativedebug=true -Dawtdebug.ctrace=true". Unfortunately this doesn't work on X platforms any more because both, libawt and libawt_xawt end up with a copy of debug_trace.o. Among others, debug_trace.o contains the static variable GlobalTracingEnabled which denotes the tracing state. This obviously can't work if the final executable contains several instances of this variable. The fix is trivial. Remove "common/awt/debug" from the set of sources for libawt_xawt. libawt_xawt is linked against libawt (which already contains debug_trace.o) anyway, so this is no problem. Thank you and best regards, Volker
Re: RFR: JDK-8214003: Limit default test jobs based on memory size
Erik: On machines with a small amount of memory compared to number of cpus, we see test failures, and sometimes very long test run times (due to swapping). Because of this we need to limit the default TEST_JOBS based on memory size, just like we do for build JOBS in configure. This patch adds a limit at "memory in GB" / 2, which seems to alleviate the problem for our particular hosts. Webrev: http://cr.openjdk.java.net/~erikj/8214003/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8214003 Looks good. Thanks for doing this - we can still get some useful testing out of legacy hardware. Tim
Re: RFR: JDK-8214003: Limit default test jobs based on memory size
On 11/16/2018 05:49 PM, Erik Joelsson wrote: > On machines with a small amount of memory compared to number of cpus, we see > test failures, and > sometimes very long test run times (due to swapping). Because of this we need > to limit the default > TEST_JOBS based on memory size, just like we do for build JOBS in configure. > > This patch adds a limit at "memory in GB" / 2, which seems to alleviate the > problem for our > particular hosts. > > Webrev: http://cr.openjdk.java.net/~erikj/8214003/webrev.01/ Looks fine to me. We also have large machines with less memory than usual, and this should help. I'd probably be more conservative (i.e. prefer slower tests instead of resource exhaustion) and target 4G per test job, but 2G per test job is okay as well. -Aleksey
RFR: JDK-8214003: Limit default test jobs based on memory size
On machines with a small amount of memory compared to number of cpus, we see test failures, and sometimes very long test run times (due to swapping). Because of this we need to limit the default TEST_JOBS based on memory size, just like we do for build JOBS in configure. This patch adds a limit at "memory in GB" / 2, which seems to alleviate the problem for our particular hosts. Webrev: http://cr.openjdk.java.net/~erikj/8214003/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8214003 /Erik
Re: [PATCH] JDK-8167368 Leftover: get_source.sh in build documentation
One comment below, otherwise good. On 2018-11-16 06:01, Sergey wrote: Hi Erik, David, Thanks for review comments, I've applied fixes accordingly. Here [1] is the only place where I've left "forest" wording. I've attached and inlined an updated patch. Looking forward for further improvements or sponsorship. Thanks, su - [1] http://hg.openjdk.java.net/jdk/sandbox/file/a2413ed39eff/doc/building.md#l52 diff --git a/doc/building.md b/doc/building.md --- a/doc/building.md +++ b/doc/building.md @@ -48,7 +48,7 @@ Make sure you are getting the correct version. As of JDK 10, the source is no longer split into separate repositories so you only need to clone one single repository. At the [OpenJDK Mercurial server](http://hg.openjdk.java.net/) you -can see a list of all available forests. If you want to build an older version, +can see a list of all available repositorys. If you want to build an older version, e.g. JDK 8, it is recommended that you get the `jdk8u` forest, which contains incremental updates, instead of the `jdk8` forest, which was frozen at JDK 8 GA. @@ -1301,17 +1301,15 @@ affected parts get rebuilt. While this works great in most cases, and significantly speed up the development process, from time to time complex interdependencies will result in an incorrect build result. This is the most -common cause for unexpected build problems, together with inconsistencies -between the different Mercurial repositories in the forest. +common cause for unexpected build problems. Here are a suggested list of things to try if you are having unexpected build problems. Each step requires more time than the one before, so try them in order. Most issues will be solved at step 1 or 2. - 1. Make sure your forest is up-to-date + 1. Make sure your repository is up-to-date - Run `bash get_source.sh` to make sure you have the latest version of all - repositories. + Run `hg pull -u` to make sure you have the latest changes. 2. Clean build results @@ -1336,13 +1334,13 @@ make ``` - 4. Re-clone the Mercurial forest + 4. Re-clone the Mercurial repository - Sometimes the Mercurial repositories themselves gets in a state that causes - the product to be un-buildable. In such a case, the simplest solution is - often the "sledgehammer approach": delete the entire forest, and re-clone - it. If you have local changes, save them first to a different location - using `hg export`. + Sometimes the Mercurial repository gets in a state that causes the product + to be un-buildable. In such a case, the simplest solution is often the + "sledgehammer approach": delete the entire repository, and re-clone it. + If you have local changes, save them first to a different location using + `hg export`. ### Specific Build Issues @@ -1393,7 +1391,7 @@ ## Hints and Suggestions for Advanced Users -### Setting Up a Forest for Pushing Changes (defpath) +### Setting Up a Repository for Pushing Changes (defpath) To help you prepare a proper push path for a Mercurial repository, there exists a useful tool known as [defpath]( @@ -1422,8 +1420,8 @@ If you also have the `trees` extension installed in Mercurial, you will automatically get a `tdefpath` command, which is even more useful. By running -`hg tdefpath -du ` in the top repository of your forest, all repos -will get setup automatically. This is the recommended usage. +`hg tdefpath -du `, repository will get setup automatically. This +is the recommended usage. The whole section about trees should be removed. That extension is just for managing a forest of repositories, which we no longer do. /Erik ### Bash Completion @@ -1459,7 +1457,7 @@ ### Using Multiple Configurations -You can have multiple configurations for a single source forest. When you +You can have multiple configurations for a single source repository. When you create a new configuration, run `configure --with-conf-name=` to create a configuration with the name ``. Alternatively, you can create a directory under `build` and run `configure` from there, e.g. `mkdir build/ && cd @@ -1474,7 +1472,7 @@ ### Handling Reconfigurations -If you update the forest and part of the configure script has changed, the +If you update the repository and part of the configure script has changed, the build system will force you to re-run `configure`. Most of the time, you will be fine by running `configure` again with the same
Re: [PATCH] JDK-8167368 Leftover: get_source.sh in build documentation
Hi Erik, David, Thanks for review comments, I've applied fixes accordingly. Here [1] is the only place where I've left "forest" wording. I've attached and inlined an updated patch. Looking forward for further improvements or sponsorship. Thanks, su - [1] http://hg.openjdk.java.net/jdk/sandbox/file/a2413ed39eff/doc/building.md#l52 diff --git a/doc/building.md b/doc/building.md --- a/doc/building.md +++ b/doc/building.md @@ -48,7 +48,7 @@ Make sure you are getting the correct version. As of JDK 10, the source is no longer split into separate repositories so you only need to clone one single repository. At the [OpenJDK Mercurial server](http://hg.openjdk.java.net/) you -can see a list of all available forests. If you want to build an older version, +can see a list of all available repositorys. If you want to build an older version, e.g. JDK 8, it is recommended that you get the `jdk8u` forest, which contains incremental updates, instead of the `jdk8` forest, which was frozen at JDK 8 GA. @@ -1301,17 +1301,15 @@ affected parts get rebuilt. While this works great in most cases, and significantly speed up the development process, from time to time complex interdependencies will result in an incorrect build result. This is the most -common cause for unexpected build problems, together with inconsistencies -between the different Mercurial repositories in the forest. +common cause for unexpected build problems. Here are a suggested list of things to try if you are having unexpected build problems. Each step requires more time than the one before, so try them in order. Most issues will be solved at step 1 or 2. - 1. Make sure your forest is up-to-date + 1. Make sure your repository is up-to-date -Run `bash get_source.sh` to make sure you have the latest version of all -repositories. +Run `hg pull -u` to make sure you have the latest changes. 2. Clean build results @@ -1336,13 +1334,13 @@ make ``` - 4. Re-clone the Mercurial forest + 4. Re-clone the Mercurial repository -Sometimes the Mercurial repositories themselves gets in a state that causes -the product to be un-buildable. In such a case, the simplest solution is -often the "sledgehammer approach": delete the entire forest, and re-clone -it. If you have local changes, save them first to a different location -using `hg export`. +Sometimes the Mercurial repository gets in a state that causes the product +to be un-buildable. In such a case, the simplest solution is often the +"sledgehammer approach": delete the entire repository, and re-clone it. +If you have local changes, save them first to a different location using +`hg export`. ### Specific Build Issues @@ -1393,7 +1391,7 @@ ## Hints and Suggestions for Advanced Users -### Setting Up a Forest for Pushing Changes (defpath) +### Setting Up a Repository for Pushing Changes (defpath) To help you prepare a proper push path for a Mercurial repository, there exists a useful tool known as [defpath]( @@ -1422,8 +1420,8 @@ If you also have the `trees` extension installed in Mercurial, you will automatically get a `tdefpath` command, which is even more useful. By running -`hg tdefpath -du ` in the top repository of your forest, all repos -will get setup automatically. This is the recommended usage. +`hg tdefpath -du `, repository will get setup automatically. This +is the recommended usage. ### Bash Completion @@ -1459,7 +1457,7 @@ ### Using Multiple Configurations -You can have multiple configurations for a single source forest. When you +You can have multiple configurations for a single source repository. When you create a new configuration, run `configure --with-conf-name=` to create a configuration with the name ``. Alternatively, you can create a directory under `build` and run `configure` from there, e.g. `mkdir build/ && cd @@ -1474,7 +1472,7 @@ ### Handling Reconfigurations -If you update the forest and part of the configure script has changed, the +If you update the repository and part of the configure script has changed, the build system will force you to re-run `configure`. Most of the time, you will be fine by running `configure` again with the same patch_JDK-8167368 Description: Binary data
Re: [8u] RFR: 8212110: Build of saproc.dll broken on Windows 32 bit after JDK-8210647
On Fri, 2018-11-16 at 12:14 +, Alex Kashchenko wrote: > > > In this line: > > > > > > SA_CFLAGS = $(SA_CFLAGS) -ZI > > > > > > "ZI" should be changed to "Zi", otherwise it fails on fastdebug and > > > release (not compatible with O2). > > > > Nice catch! How about this? > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8212110/jdk8/webrev.03/ > > This worked for me. Great, thanks! Cheers, Severin
Re: RFR: 8061282: Migrate jmh-jdk-microbenchmarks into the JDK
Hi Mandy, I've updated copyright headers of all microbenchmarks to use the correct one, and fixed a few instances of long lines that stood out. We can do more cleanups as a follow-up. Currently no microbenchmark includes any resources, and as you say the current convention is to keep resources co-located. I've removed the classes and resources folders, and updated makefiles with the trivial changes needed for this. The JEP text also referred to `make run-test`, which is now just `make test`. Updated. To help set thing up JDK-8061281 includes examples and setup instructions in docs/testing.md|html - I've added a reference to this documentation in the JEP text. I think using the JEP as the authoritative place to document usage wouldn't age well, as instructions typically evolve and change over time. Combined webrev for 8061281 and 8061282 here: http://cr.openjdk.java.net/~redestad/8061281_8061282.01/ Thanks! /Claes On 2018-11-15 23:50, Mandy Chung wrote: Hi Claes, It's good to see this JEP targeted and integrate the microbenchmarks to colocate with JDK. Overall the work looks good. The copyright headers need update to GPL. There are some super long lines (mostly looking up method handles), for example: +MethodHandle bodyNormal = MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, "doWorkNormal", MethodType.methodType(void.class, MethodHandlesCatchException.class)); +MethodHandle bodyExceptional = MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, "doWorkExceptional", MethodType.methodType(void.class, MethodHandlesCatchException.class)); +MethodHandle fallback = MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, "fallback", MethodType.methodType(void.class, MyException.class, MethodHandlesCatchException.class)); JEP 230 proposes to separate the resources from the source files in micro/classes and micro/resources directories. What kinds of resources are expected to be placed under micro/resources directory? If they are java resources, then I would expect them follow the consistent layout as JDK source tree where the java classes and resources are placed together. I was trying to experiment building and running the benchmarks. What does configure --with-jmh expect to contain? I can't quite figure it out from the error message. The JEP describes make build-microbenchmark and run-test targets to build and execute the microbenchmarks. It'd also be helpful to update the JEP to include an example how to run a specific set of benchmarks e.g. org.openjdk.bench.java.lang.ObjectHashCode and how to run the benchmarks with JDK n and JDK n-1 and compare the result (is there a build target to do this)? We can reference this JEP to get started running the microbenchmark and refer to JMH and other documentation for other details like developing a JMH benchmark. Mandy On 10/18/18 2:03 PM, Claes Redestad wrote: Hi, as the final part of JEP 230: Microbenchmarks Suite, I propose migrating all microbenchmarks from the codetools jmh-jdk-microbenchmarks project into the JDK: http://cr.openjdk.java.net/~redestad/8061282/jdk.00/ This is built on top of the patch for JDK-8061281, and makes the entirety of this suite readily available to build, run and experiment with from the main jdk repos. While the future of the codetools jmh-jdk-microbenchmarks project is out of scope for JEP 230, it has been suggested it could be kept alive as a stabilization target and that stable microbenchmarks should be kept out of the jdk. That discussion is partly out of scope here, but I believe it makes sense to keep a copy in the JDK suite precisely because the benchmark will be compiled with the platform javac, meaning a different set of bugs, regressions and improvements will be discoverable. Two micros, org.openjdk.bench.java.lang.invoke.Indify and org.opendjk.bench.java.lang.reflect.GetAnnotation need special build treatment and will need to be dealt with in a follow-up. Thanks! /Claes
Re: [8u] RFR: 8212110: Build of saproc.dll broken on Windows 32 bit after JDK-8210647
On 11/16/2018 08:53 AM, Severin Gehwolf wrote: Hi Alex, Hi Alex, On Fri, 2018-11-16 at 00:32 +, Alex Kashchenko wrote: Hi Severin On 11/15/2018 07:39 PM, Erik Joelsson wrote: Looks good to me. /Erik On 2018-11-15 11:31, Severin Gehwolf wrote: Hi, When the SA optimization fix got backported to JDK 11 a build issue on Windows 32 bit was discovered by SAP. This would be the JDK 8 equivalent for the old build system. Note that the JDK 8 build still uses GZ which is deprecated since at least Visual Studio 2012[1]. So I've replaced it with RTC1. This backport patch depends on [2] which got reviewed already. Bug: https://bugs.openjdk.java.net/browse/JDK-8212110 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8212110/jdk8/webrev.02/ In this line: SA_CFLAGS = $(SA_CFLAGS) -ZI "ZI" should be changed to "Zi", otherwise it fails on fastdebug and release (not compatible with O2). Nice catch! How about this? http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8212110/jdk8/webrev.03/ This worked for me. -- -Alex
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
On Thu, Nov 15, 2018 at 6:01 PM Philip Race wrote: > > PS I am not sure why xrandr headers would not be available for AIX. > They are a standard part of the xdistribution. > I'm not an X11 guru, but as far as I understand, xrandr is an extension and as such it doesn't have to be supported by every implementation. To the best of my knowledge (I've just started another poll among some experts) AIX doesn't support Xrandr and does not have the corresponding headers. > If true, think what you are going to have to do is add a > --with-xrandr-include option > and provide it that way. > What if there are no standard Xrandr headers on a platform? Do you really want to force users to get them from some dubious sources just for building the OpenJDK? Sorry, but I don't think that's a good solution. Than I'd rather prefer the ugly ifdefs (or I check the headers back in again in an AIX-specific directory :) Thank you and best regards, Volker > -phil. > > On 11/15/18, 8:55 AM, Philip Race wrote: > > Hmm. I don't like the ifdefs. > > > > Xrandr is a requirement for the build. If its not there at runtime > > that's OK. > > > > -phil. > > > > On 11/15/18, 8:06 AM, Volker Simonis wrote: > >> Hi, > >> > >> can I please have a review for the following small change: > >> > >> http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ > >> https://bugs.openjdk.java.net/browse/JDK-8213944 > >> > >> Change JDK-8210863 removed the Xrandr.h/randr.h headers from the > >> OpenJDK sources but forgot to add a configure check for the Xrandr > >> extension which is now a build dependency. > >> > >> The change also broke the AIX build. AIX never supported Xrandr, but > >> that was only detected at runtime, when the JDK was unable to > >> dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the > >> source tree any more, we have to conditionally compile some parts of > >> src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such > >> that it doesn't require the definitions and declarations from > >> Xrandr.h/randr.h any more. > >> > >> Thank you and best regards, > >> Volker
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
On Thu, Nov 15, 2018 at 5:55 PM Philip Race wrote: > > Hmm. I don't like the ifdefs. > Mee too, but what else can we do, if there are no Xrandr headers available on a platform? Please see my answer to your other mail... > Xrandr is a requirement for the build. If its not there at runtime > that's OK. > > -phil. > > On 11/15/18, 8:06 AM, Volker Simonis wrote: > > Hi, > > > > can I please have a review for the following small change: > > > > http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ > > https://bugs.openjdk.java.net/browse/JDK-8213944 > > > > Change JDK-8210863 removed the Xrandr.h/randr.h headers from the > > OpenJDK sources but forgot to add a configure check for the Xrandr > > extension which is now a build dependency. > > > > The change also broke the AIX build. AIX never supported Xrandr, but > > that was only detected at runtime, when the JDK was unable to > > dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the > > source tree any more, we have to conditionally compile some parts of > > src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such > > that it doesn't require the definitions and declarations from > > Xrandr.h/randr.h any more. > > > > Thank you and best regards, > > Volker
Re: RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
On Thu, Nov 15, 2018 at 5:31 PM Aleksey Shipilev wrote:
>
> On 11/15/2018 05:06 PM, Volker Simonis wrote:
> > can I please have a review for the following small change:
> >
> > http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/
>
> *) I tested it on platform without libxrandr-dev installed, and configure
> reported the failure
> appopriately.
>
Thanks for testing!
> *) Indent looks off at L2207 here:
>
> 2205 static char *get_output_screen_name(JNIEnv *env, int screen) {
> 2206 #ifdef _AIX
> 2207 return NULL;
> 2208 #else
> 2209 if (!awt_XRRGetScreenResources || !awt_XRRGetOutputInfo) {
>
You're right. Fixed it.
Thank you and best regards,
Volker
>
> Thanks,
> -Aleksey
>
Re: [8u] RFR: 8212110: Build of saproc.dll broken on Windows 32 bit after JDK-8210647
Hi Alex, Hi Alex, On Fri, 2018-11-16 at 00:32 +, Alex Kashchenko wrote: > Hi Severin > > On 11/15/2018 07:39 PM, Erik Joelsson wrote: > > Looks good to me. > > > > /Erik > > > > On 2018-11-15 11:31, Severin Gehwolf wrote: > > > Hi, > > > > > > When the SA optimization fix got backported to JDK 11 a build issue on > > > Windows 32 bit was discovered by SAP. This would be the JDK 8 > > > equivalent for the old build system. Note that the JDK 8 build still > > > uses GZ which is deprecated since at least Visual Studio 2012[1]. So > > > I've replaced it with RTC1. This backport patch depends on [2] which > > > got reviewed already. > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8212110 > > > webrev: > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8212110/jdk8/webrev.02/ > > In this line: > > SA_CFLAGS = $(SA_CFLAGS) -ZI > > "ZI" should be changed to "Zi", otherwise it fails on fastdebug and > release (not compatible with O2). Nice catch! How about this? http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8212110/jdk8/webrev.03/ Thanks, Severin
