Re: RFR(S): 8234791: Fix Client VM build for x86_64 and AArch64
Hi, this looks good to me. Best regards, Tobias On 30.11.19 02:02, Ioi Lam wrote: > Hi Pengfei, > > I have cc-ed hotspot-compiler-...@openjdk.java.net. > > Please do not push the patch until someone from hotspot-compiler-dev has > looked at it. > > Many people are away due to Thanksgiving in the US. > > Thanks > - Ioi > > On 11/28/19 7:56 PM, Pengfei Li (Arm Technology China) wrote: >> Hi, >> >> Please help review this small fix for 64-bit client build. >> >> Webrev: http://cr.openjdk.java.net/~pli/rfr/8234791/webrev.00/ >> JBS: https://bugs.openjdk.java.net/browse/JDK-8234791 >> >> Current 64-bit client VM build fails because errors occurred in dumping >> the CDS archive. In JDK 12, we enabled "Default CDS Archives"[1] which >> runs "java -Xshare:dump" after linking the JDK image. But for Client VM >> build on 64-bit platforms, the ergonomic flag UseCompressedOops is not >> set.[2] This leads to VM exits in checking the flags for dumping the >> shared archive.[3] >> >> This change removes the "#if defined" macro to make shared archive dump >> successful in 64-bit client build. By tracking the history of the macro, >> I found it is initially added as "#ifndef COMPILER1"[4] 10 years ago >> when C1 did not have a good support of compressed oops and modified to >> current shape[5] in the implementation of tiered compilation. It should >> be safe to be removed today. >> >> This patch also fixes another client build issue on AArch64. >> >> [1] http://openjdk.java.net/jeps/341 >> [2] >> http://hg.openjdk.java.net/jdk/jdk/file/981a55672786/src/hotspot/share/runtime/arguments.cpp#l1694 >> [3] >> http://hg.openjdk.java.net/jdk/jdk/file/981a55672786/src/hotspot/share/runtime/arguments.cpp#l3551 >> [4] http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/323bd24c6520#l11.7 >> [5] http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/d5d065957597#l86.56 >> >> -- >> Thanks, >> Pengfei >> >
Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation
On 12/2/2019 2:07 PM, Phil Race wrote: This makes it very difficult to do more than a cursory re-review. There is one thing I just spotted. I believe these two scripts http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/test_jpackage.sh.html http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/test/jdk/tools/jpackage/run_tests.sh.html OK - run_tests.sh has been modified to not download jtreg from an external server. /Andy Should not be part of what is pushed. They should be removed from the webrev. -phil. On 11/27/19 6:07 AM, Andy Herrick wrote: yes - The attempted incremental patch is not much use. The source files were moved several times, and despite using "hg addremove -s 60", many of the files show as a remove and a new file. /Andy On 11/26/2019 9:01 PM, Philip Race wrote: > I believe otherwise it is an accurate incremental webrev of the jpackage changes since EA-5. It is also not very incremental. * *src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/main/Main.java is definitely not "new" ... -phil. On 11/26/19, 2:17 PM, Andy Herrick wrote: yes, this incremental webrev contains the JNLPConverter code, which it should not. I believe otherwise it is an accurate incremental webrev of the jpackage changes since EA-5. /Andy On 11/26/2019 4:53 PM, Phil Race wrote: Andy, I am puzzled by what I see here > The webrev at [3] shows the changes since EA-06 (Build 13-jpackage+1-49 ) up to the current point. > [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/ This includes the JNLPConverter which isn't what I expected to see and (correctly) isn't in the cumulative webrev Since [3] seemed like the most obvious thing to do a review of the recent changes I'd like to be sure it has the correct content and I am not sure it does ... -phil. On 11/26/19 1:36 PM, Kevin Rushforth wrote: This all looks good. +1 -- Kevin On 11/26/2019 12:56 PM, Erik Joelsson wrote: (adding build-dev) Build changes look good. /Erik On 2019-11-20 09:37, Andy Herrick wrote: I posted new composite webrev [7], and git patch [8] after pushing fix for JDK-8234402 [6]. [7] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-11/ [8] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-11.git.patch /Andy On 11/19/2019 3:13 PM, Kevin Rushforth wrote: I took the "git diff" patch [5] that you uploaded yesterday, applied it, and verified that it is the same as what is in the JDK-8200758-branch branch of the sandbox. It builds and runs fine for me. I ran jcheck and it found the following three files with whitespace errors that will need to be fixed before you push: src/jdk.incubator.jpackage/linux/classes/jdk/incubator/jpackage/internal/PackageProperty.java:49: Trailing whitespace src/jdk.incubator.jpackage/share/classes/jdk/incubator/jpackage/ToolProviderFactory.java:35: Trailing whitespace test/jdk/tools/jpackage/share/AddLauncherBase.java:137: Trailing whitespace The second of these will go away with the fix for JDK-8234402 [6], so you don't need to do anything there. Once the fix for JDK-8234402 is pushed to sandbox, I presume you will update the webrev, right? Pending the fix for JDK-8234402 and the needed white-space fixes, this is a +1 from me (although I am not a jdk Project Reviewer, so you will need at least one review from someone who is). -- Kevin [5] http://cr.openjdk.java.net/~herrick/8212780/JDK-EA-10.git.patch [6] https://bugs.openjdk.java.net/browse/JDK-8234402 On 11/13/2019 4:23 PM, Andy Herrick wrote: Please review changes for [1] which is the implementation bug for JEP-343. The webrev at [2] is the total cumulative webrev of changes for the jpackage tool, currently in the JDK-8200758-branch branch of the open sandbox repository. The webrev at [3] shows the changes since EA-06 (Build 13-jpackage+1-49 ) up to the current point. The latest EA (Build 14-jpackage+1-49 ) is posted at [4]. Please send feedback to core-libs-...@openjdk.java.net [1] https://bugs.openjdk.java.net/browse/JDK-8212780 [2] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-10/ [3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/ [4] http://jdk.java.net/jpackage/
Re: RFR: 8234835 Use UTF-8 charset in make support Java code
Hi Dan, I think it's a combination of oral tradition and long-standing precedent. Earlier this year, I raised this general issue, partly because of inconsistent use of -encoding in the build system. The response was that there was some concern that not all tools in the tool chain could handle UTF-8 files. $ find open/make -name \*.gmk | xargs grep -o -e '-encoding [^ ]*' open/make/Docs.gmk:-encoding ISO-8859-1 open/make/Docs.gmk:-encoding ISO-8859-1 open/make/common/SetupJavaCompilers.gmk:-encoding ascii open/make/common/SetupJavaCompilers.gmk:-encoding ascii I think we should be consistent, but (at the time) it did not seem worth pushing for UTF-8 everywhere. -- Jon On 11/27/2019 07:23 PM, Dan Smith wrote: For the other files, it seems strange to force the use of a charset which is different from the charset of record for all our source files (i.e. US-ASCII). Can you clarify where this "charset of record" rule comes from? Is this written down somewhere, or more of an oral tradition? The non-ASCII characters I'm working with are, in fact, in the original Markdown sources. If it's really important to avoid those in all sources, I could (reluctantly) use a different strategy. If the consensus is that the build tools should standardize on US-ASCII, I guess there's a separate question about whether we're willing to rely on the implicit platform default (now uniformly US-ASCII via command-line args), or whether it's better to be explicit about it (s/UTF_8/US_ASCII/ in my changeset).
RE: RFR(S): 8234791: Fix Client VM build for x86_64 and AArch64
Thanks Tobias. Could anyone help push this? It's now reviewed by adinn, aph and thartmann. > Hi, > > this looks good to me. > > Best regards, > Tobias > > On 30.11.19 02:02, Ioi Lam wrote: > > Hi Pengfei, > > > > I have cc-ed hotspot-compiler-...@openjdk.java.net. > > > > Please do not push the patch until someone from hotspot-compiler-dev > has looked at it. > > > > Many people are away due to Thanksgiving in the US. > > > > Thanks > > - Ioi > > > > On 11/28/19 7:56 PM, Pengfei Li (Arm Technology China) wrote: > >> Hi, > >> > >> Please help review this small fix for 64-bit client build. > >> > >> Webrev: http://cr.openjdk.java.net/~pli/rfr/8234791/webrev.00/ > >> JBS: https://bugs.openjdk.java.net/browse/JDK-8234791 > >> > >> Current 64-bit client VM build fails because errors occurred in > >> dumping the CDS archive. In JDK 12, we enabled "Default CDS > >> Archives"[1] which runs "java -Xshare:dump" after linking the JDK > >> image. But for Client VM build on 64-bit platforms, the ergonomic > >> flag UseCompressedOops is not set.[2] This leads to VM exits in > >> checking the flags for dumping the shared archive.[3] > >> > >> This change removes the "#if defined" macro to make shared archive > >> dump successful in 64-bit client build. By tracking the history of > >> the macro, I found it is initially added as "#ifndef COMPILER1"[4] 10 > >> years ago when C1 did not have a good support of compressed oops and > >> modified to current shape[5] in the implementation of tiered > >> compilation. It should be safe to be removed today. > >> > >> This patch also fixes another client build issue on AArch64. > >> > >> [1] http://openjdk.java.net/jeps/341 > >> [2] > >> http://hg.openjdk.java.net/jdk/jdk/file/981a55672786/src/hotspot/shar > >> e/runtime/arguments.cpp#l1694 > >> [3] > >> http://hg.openjdk.java.net/jdk/jdk/file/981a55672786/src/hotspot/shar > >> e/runtime/arguments.cpp#l3551 [4] > >> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/323bd24c6520#l11.7 > >> [5] > >> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/d5d065957597#l86.56 -- Thanks, Pengfei
Re: RFR(S): 8234791: Fix Client VM build for x86_64 and AArch64
On 04.12.19 07:25, Pengfei Li (Arm Technology China) wrote: > Thanks Tobias. Could anyone help push this? It's now reviewed by adinn, aph > and thartmann. Sure, pushed. Best regards, Tobias
