Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v11]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Revert changes to RunTests.gmk - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6e7189b4..504b564a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=09-10 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 19:17:40 GMT, Jorn Vernee wrote: >> I'd suggest at least adding `--enable-preview` as an argument when running >> benchmarks through the build system in that case. I think this should do the >> trick: >> >> >> diff --git a/make/RunTests.gmk b/make/RunTests.gmk >> index 81540266ec0..9ed45fb02a8 100644 >> --- a/make/RunTests.gmk >> +++ b/make/RunTests.gmk >> @@ -583,7 +583,7 @@ define SetupRunMicroTestBody >>$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) >> >># Current tests needs to open java.io >> - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED >> + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED >> --enable-preview >> >># Save output as JSON or CSV file >>ifneq ($$(MICRO_RESULTS_FORMAT), ) >> >> >> People manually running the benchmarks.jar will have to pass >> `--enable-preview` still though. > > After discussing this offline, it seems that javac no longer poisons the > minor class file version of every class file, but only of those that use > preview features. So, my concern is not warranted. Turns out this is no longer necessary. As part of the support for preview API, javac now only pollutes the classfile if a source file is using preview features, as described in this PR: https://github.com/openjdk/jdk/pull/703 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 18:35:12 GMT, Jorn Vernee wrote: >> Sure, this is problematic - but at the same time I don't think there's a >> better way to deal with this? I'd prefer to defer this to a separate issue >> (and I think the build team is in a much better position to suggest a better >> fix). IIRC we had this problem in the past as well. > > I'd suggest at least adding `--enable-preview` as an argument when running > benchmarks through the build system in that case. I think this should do the > trick: > > > diff --git a/make/RunTests.gmk b/make/RunTests.gmk > index 81540266ec0..9ed45fb02a8 100644 > --- a/make/RunTests.gmk > +++ b/make/RunTests.gmk > @@ -583,7 +583,7 @@ define SetupRunMicroTestBody >$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) > ># Current tests needs to open java.io > - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED > + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED > --enable-preview > ># Save output as JSON or CSV file >ifneq ($$(MICRO_RESULTS_FORMAT), ) > > > People manually running the benchmarks.jar will have to pass > `--enable-preview` still though. After discussing this offline, it seems that javac no longer poisons the minor class file version of every class file, but only of those that use preview features. So, my concern is not warranted. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v11]
On Thu, 24 Mar 2022 19:19:34 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Revert changes to RunTests.gmk Looks Good! - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v10]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add --enable-preview to micro benchmark java options - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/d95c6d0f..6e7189b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=08-09 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v9]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address more review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6881b6dc..d95c6d0f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=07-08 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v8]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3e8cfd74..6881b6dc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=06-07 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 17:48:23 GMT, Maurizio Cimadamore wrote: >> make/test/BuildMicrobenchmark.gmk line 97: >> >>> 95: SRC := $(MICROBENCHMARK_SRC), \ >>> 96: BIN := $(MICROBENCHMARK_CLASSES), \ >>> 97: JAVAC_FLAGS := --add-exports >>> java.base/sun.security.util=ALL-UNNAMED --enable-preview, \ >> >> It still seems like this would lead to potential issues. i.e. requiring all >> benchmarks to be run with `--enable-preview`? We ended up adding >> `--enable-preview` to our benchmarks, but do other benchmarks still work >> without it? AFAIK the entire benchmarks.jar will have the altered class file >> version. > > Sure, this is problematic - but at the same time I don't think there's a > better way to deal with this? I'd prefer to defer this to a separate issue > (and I think the build team is in a much better position to suggest a better > fix). IIRC we had this problem in the past as well. I'd suggest at least adding `--enable-preview` as an argument when running benchmarks through the build system in that case. I think this should do the trick: diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 81540266ec0..9ed45fb02a8 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -583,7 +583,7 @@ define SetupRunMicroTestBody $$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) # Current tests needs to open java.io - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED --enable-preview # Save output as JSON or CSV file ifneq ($$(MICRO_RESULTS_FORMAT), ) People manually running the benchmarks.jar will have to pass `--enable-preview` still though. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 13:00:12 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Drop redundant javadoc statements re. handling of nulls
>> (handling of nulls is specified once and for all in the package javadoc)
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1071:
>
>> 1069: sessionImpl.checkValidStateSlow();
>> 1070: if (offset < 0) throw new IllegalArgumentException("Requested
>> bytes offset must be >= 0.");
>> 1071: if (size < 0) throw new IllegalArgumentException("Requested
>> bytes size must be >= 0.");
>
> The javadoc also says that IAE will be thrown if `offset + size < 0` I think
> to guard against overflow, but I don't see that checked here. Is it missing?
`mapInternal` in FileChannelImpl takes care of that for both flavors of `map`
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v7]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/95f65eea..3e8cfd74 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=05-06 Stats: 16 lines in 3 files changed: 2 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 13:10:20 GMT, Jorn Vernee wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Drop redundant javadoc statements re. handling of nulls >> (handling of nulls is specified once and for all in the package javadoc) > > make/test/BuildMicrobenchmark.gmk line 97: > >> 95: SRC := $(MICROBENCHMARK_SRC), \ >> 96: BIN := $(MICROBENCHMARK_CLASSES), \ >> 97: JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED >> --enable-preview, \ > > It still seems like this would lead to potential issues. i.e. requiring all > benchmarks to be run with `--enable-preview`? We ended up adding > `--enable-preview` to our benchmarks, but do other benchmarks still work > without it? AFAIK the entire benchmarks.jar will have the altered class file > version. Sure, this is problematic - but at the same time I don't think there's a better way to deal with this? I'd prefer to defer this to a separate issue (and I think the build team is in a much better position to suggest a better fix). IIRC we had this problem in the past as well. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v6]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/c9bc9a70..95f65eea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=04-05 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: Failing jdk11u AIX build on Adoptium servers
Hi Matthias, On Thu, Mar 24, 2022 at 4:55 PM Baesken, Matthias wrote: > Hi , > > > > >My expected course of action is as follows: > >- Bump the minimum requirements in our Supported Builds document > >[https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms] > >- Open an issue with the Adoptium group to change their build for jdk11 > and above to use the version 16 compiler. > > Are the xlc16 – related changes already all in jdk11u-dev now ? > > > > >probably we should change the minimum requirement of jdk12 too then. > > > > JDK12 does not have the harfbuzz 2.8.0 backport so probably no need to > change the Supported Builds document. > > oh, you are right. I just thought that it would be strange if jdk11 had a newer compiler requirement than jdk12. But probably nobody cares for jdk12 on AIX. Thanks, Thomas > > > Best regards, Matthias > > > >
Re: Failing jdk11u AIX build on Adoptium servers
cc build-dev. Hi Tyler, makes sense to me. Wrt the documentation, probably we should change the minimum requirement of jdk12 too then. Cheers, Thomas On Thu, Mar 24, 2022 at 4:05 PM Tyler Steele wrote: > Hi all, > > In regards to the new Harfbuzz version bump (and the work done to prepare > for it). And of special importance now that > the Adoptium build of jdk11u-dev is > failing [ > https://ci.adoptopenjdk.net/job/AIX-jdk11-dev-build/17/execution/node/51/log/], > I wanted to suggest a course of > action to the AIX-folks here to get suggestions before I make any changes. > > Early this week, I spent some time looking into this issue. My assessment > is that AIX builds for jdk versions 11 and > greater will need the at least xlc version 16.1 to support the Harfbuzz > change. This is because full support for C++11 > with xlc did not arrive on AIX until that version (little-endian support > was complete in 13.1). > > For reference: > "IBM XL C++ for AIX > - Core language support status: C++11 partial in 13.1.3 and 16.1.0 (xlC > frontend), complete in 16.1.0 (xlclang > frontend)" [source: https://en.cppreference.com/w/cpp/compiler_support ] > > My expected course of action is as follows: > - Bump the minimum requirements in our Supported Builds document > [https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms] > - Open an issue with the Adoptium group to change their build for jdk11 > and above to use the version 16 compiler. > > As I am still relatively new to AIX and OpenJDK development, so I value > any suggestions for improvements, omissions you > may have noticed, or generally other comments the team here may have. > > Tyler >
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Wed, 23 Mar 2022 14:06:56 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop redundant javadoc statements re. handling of nulls
> (handling of nulls is specified once and for all in the package javadoc)
Some more nits.
One potential issue with adding --enable-preview when building benchmarks (last
comment of the bunch).
Other than that, I think this looks good.
make/test/BuildMicrobenchmark.gmk line 97:
> 95: SRC := $(MICROBENCHMARK_SRC), \
> 96: BIN := $(MICROBENCHMARK_CLASSES), \
> 97: JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED
> --enable-preview, \
It still seems like this would lead to potential issues. i.e. requiring all
benchmarks to be run with `--enable-preview`? We ended up adding
`--enable-preview` to our benchmarks, but do other benchmarks still work
without it? AFAIK the entire benchmarks.jar will have the altered class file
version.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 61:
> 59: * {@linkplain MemorySegment#allocateNative(long, long,
> MemorySession) native memory segments}, backed by off-heap memory;
> 60: * {@linkplain FileChannel#map(FileChannel.MapMode, long, long,
> MemorySession) mapped memory segments}, obtained by mapping
> 61: * a file into main memory ({@code mmap}); tha contents of a mapped
> memory segments can be {@linkplain #force() persisted} and
Suggestion:
* a file into main memory ({@code mmap}); the contents of a mapped memory
segments can be {@linkplain #force() persisted} and
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 298:
> 296:
> 297: /**
> 298: * Returns a slice of this memory segment, at given offset. The
> returned segment's base address is the base address
Saw a similar change in other places, so I'll suggest this here as well.
Suggestion:
* Returns a slice of this memory segment, at the given offset. The
returned segment's base address is the base address
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 311:
> 309:
> 310: /**
> 311: * Returns a slice of this memory segment, at given offset. The
> returned segment's base address is the base address
Suggestion:
* Returns a slice of this memory segment, at the given offset. The
returned segment's base address is the base address
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 143:
> 141:
> 142: /**
> 143: * {@return the owner thread associated with this memory session (if
> any)}
Maybe the "if any" here could be more specific. e.g. saying that `null` is
returned if the session doesn't have an owner thread.
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 165:
> 163:
> 164: /**
> 165: * Closes this memory session. As a side effect, if this operation
> completes without exceptions, this session
I'd suggest change this to "As a result of this", since the effects listed are
the main reason for closing a session. (it strikes me as strange. If the things
listed are side-effects, then what is the main effect of closing a segment?)
Suggestion:
* Closes this memory session. As a result of this, if this operation
completes without exceptions, this session
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 51:
> 49: *
> 50: * Clients can obtain a {@linkplain #loaderLookup() loader lookup},
> 51: * which can be used to search symbols in libraries loaded by the current
> classloader (e.g. using {@link System#load(String)},
"search symbols" sounds a bit unnatural to me... I like the wording in the
libraryLookup doc more
Suggestion:
* which can be used to find symbols in libraries loaded by the current
classloader (e.g. using {@link System#load(String)},
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 59:
> 57: *
> 58: * Finally, clients can load a library and obtain a {@linkplain
> #libraryLookup(Path, MemorySession) library lookup} which can be used
> 59: * to search symbols in that library. A library lookup is associated with
> a {@linkplain MemorySession memory session},
Suggestion:
* to find symbols in that library. A library lookup is associated with a
{@linkplain MemorySession memory session},
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7895:
> 7893: * VarHandle handle =
> MethodHandles.memorySegmentViewVarHandle(ValueLayout.JAVA_INT.withOrder(ByteOrder.BIG_ENDIAN));
> //(MemorySegment, long) -> int
> 7894: * handle = MethodHandles.insertCoordinates(handle, 1, 4);
>
Integrated: 8283575: Check for GNU time fails for version >1.7
On Wed, 23 Mar 2022 15:35:44 GMT, Erik Joelsson wrote: > The version output of GNU time changed from "GNU time" to "GNU Time" in > version 1.8. We need to update our check for identifying GNU time to handle > this. This pull request has now been integrated. Changeset: 1c4f5fcb Author:Erik Joelsson URL: https://git.openjdk.java.net/jdk/commit/1c4f5fcb88892e6c76074eac87b63d81d53647b2 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8283575: Check for GNU time fails for version >1.7 Reviewed-by: shade, ihse - PR: https://git.openjdk.java.net/jdk/pull/7925
Integrated: 8283323: libharfbuzz optimization level results in extreme build times
On Wed, 23 Mar 2022 12:25:08 GMT, Magnus Ihse Bursie wrote: > [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade > HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my reference > linux machine. This was one of the four culprits that caused a 25-30% build > time regression over the last two years. > > The problem here was that the new HarfBuzz code caught really bad behaviour > from gcc when compiling with optimizations. The official HarfBuzz build does > not use any -O flags at all for gcc, so presumably the HarfBuzz team is: > > a) not thinking compiler optimization is important for the performance of > this library, and > b) unaware that their code causes such a headache for gcc. > > (Other compilers fare much better: visual studio makes no difference at all, > and for clang just a small regression was observed.) > > The current optimization level was introduced by > [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were > really about moving libharfbuzz compilation back into libfontmanager. I could > find no comments/discussion relating to the change of optimization level, so > I assume it was incidental, and just seemed good at the time. > > This patch changes the optimization level to `SIZE` (which is the closest > thing we have to no optimization level) on gcc. This pull request has now been integrated. Changeset: 2c43ecb4 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/2c43ecb43fa3c94b69478039f1cd70ed4a577768 Stats: 9 lines in 1 file changed: 8 ins; 1 del; 0 mod 8283323: libharfbuzz optimization level results in extreme build times Reviewed-by: erikj, prr - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8283575: Check for GNU time fails for version >1.7 [v2]
On Wed, 23 Mar 2022 22:06:49 GMT, Erik Joelsson wrote: >> The version output of GNU time changed from "GNU time" to "GNU Time" in >> version 1.8. We need to update our check for identifying GNU time to handle >> this. > > Erik Joelsson has updated the pull request incrementally with one additional > commit since the last revision: > > Update make/autoconf/basic_tools.m4 > > Co-authored-by: Magnus Ihse Bursie Marked as reviewed by ihse (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7925
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times [v3]
On Thu, 24 Mar 2022 07:38:34 GMT, Sergey Bylokhov wrote: >>> @mrserb Phil said that we don't even use the subset part of harfbuzz. >>> >>> And unless you're backing up your claim that this patch is changing runtime >>> characteristic with data, that's just a guess, just like the initial >>> optimization level of this library (and as most of the native libraries in >>> the JDK... :-() was just a guess. Otoh, that we get a build time regression >>> for these two files is a proven fact. >>> >>> What is it you want to cache? >> >> It is a reasonable question. Build times might seem important to a small >> number of developers but runtime performance is King. That's why I asked to >> reduce it to just the files we don't care about. >> Would we give up even 0.1% of hotspot GC performance on Linux to avoid 30 >> seconds of compile time ? >> I doubt it. >> And haven't we improved compile time a lot with the parallelism ? So are we >> now over-optimising in the wrong place ? >> I mean good to keep an eye on it but I mean if a previous build arch took 15 >> mins and now with parallelism we take 3 mins and 45 secs .. is it so bad to >> be back up to 4 mins for more runtime benefit ? (Numbers entirely made up) >> There's always going to be a long pole (something that is last) and the >> questions are whether its understood and for a good reason and so forth. I >> mean we can doubtless try to improve but at some point it is diminishing >> returns. >> Perhaps someone can ask gcc devs why they are so slow on this code ? Maybe >> there's a good reason that helps runtime or maybe they just have design >> issues. They may want to know. > >> And unless you're backing up your claim that this patch is changing runtime >> characteristic with data, that's just a guess, just like the initial >> optimization level of this library (and as most of the native libraries in >> the JDK... :-() was just a guess. Otoh, that we get a build time regression >> for these two files is a proven fact. > > All java2d libraries are compiled using HIGHEST, that option was bumped to > this level over time because it was proved that they affect the performance, > an example the last change: https://bugs.openjdk.java.net/browse/JDK-8264846 > So I think we should check first that it does not affect performance, even if > it is not used now it can be used in future version of harfbuzz. @mrserb As Phil has stated twice, this code is not used by the JDK, so will not affect performance. It is impossible to predict what future changes in harfbuzz will do to our performance. That will of course needed to be tested when we update harfbuzz. If it turns out that setting this optimization level is detrimental to our performance at that point of time, we'll of course do what fixes needs to be done, at that point. @prrace The sad truth is that java.desktop-libs is at the end of the long pole, due to how different modules are interrelated. :-( That means that the compilation time for these native libraries tend to dominate the entire build time, regardless of how parallelized the rest of the build can be made. (cf. Amdahl's law) I'll look into once more if we can do some shenanigans to try to get around some of these dependencies, but I'm not sure it's possible. I encourage you to report this to both upstream harfbuzz and gcc. - PR: https://git.openjdk.java.net/jdk/pull/7919
Integrated: 8276799: Implementation of JEP 422: Linux/RISC-V Port
On Mon, 8 Nov 2021 11:17:47 GMT, Fei Yang wrote:
> This PR implements JEP 422: Linux/RISC-V Port [1].
> The PR starts as a squashed merge of the
> https://openjdk.java.net/projects/riscv-port branch.
>
> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are also
> carried out regularly. So it should be good enough to run most Java programs.
>
> [1] https://openjdk.java.net/jeps/422
This pull request has now been integrated.
Changeset: 5905b02c
Author:Fei Yang
URL:
https://git.openjdk.java.net/jdk/commit/5905b02c0e2643ae8d097562f181953f6c88fc89
Stats: 59141 lines in 188 files changed: 58964 ins; 54 del; 123 mod
8276799: Implementation of JEP 422: Linux/RISC-V Port
Co-authored-by: Yadong Wang
Co-authored-by: Yanhong Zhu
Co-authored-by: Feilong Jiang
Co-authored-by: Kun Wang
Co-authored-by: Zhuxuan Ni
Co-authored-by: Taiping Guo
Co-authored-by: Kang He
Co-authored-by: Aleksey Shipilev
Co-authored-by: Xiaolin Zheng
Co-authored-by: Kuai Wei
Co-authored-by: Magnus Ihse Bursie
Reviewed-by: ihse, dholmes, rriggs, kvn, shade
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times [v3]
On Thu, 24 Mar 2022 03:53:52 GMT, Phil Race wrote: > And unless you're backing up your claim that this patch is changing runtime > characteristic with data, that's just a guess, just like the initial > optimization level of this library (and as most of the native libraries in > the JDK... :-() was just a guess. Otoh, that we get a build time regression > for these two files is a proven fact. All java2d libraries are compiled using HIGHEST, that option was bumped to this level over time because it was proved that they affect the performance, an example the last change: https://bugs.openjdk.java.net/browse/JDK-8264846 So I think we should check first that it does not affect performance, even if it is not used now it can be used in future version of harfbuzz. - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v4]
On Wed, 23 Mar 2022 02:17:22 GMT, Vladimir Kozlov wrote: >> Fei Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix copyright header > > Update looks good. > Testing results are also good. @vnkozlov @shipilev : Thanks for reviewing this :-) - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v5]
> This PR implements JEP 422: Linux/RISC-V Port [1].
> The PR starts as a squashed merge of the
> https://openjdk.java.net/projects/riscv-port branch.
>
> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are also
> carried out regularly. So it should be good enough to run most Java programs.
>
> [1] https://openjdk.java.net/jeps/422
Fei Yang has updated the pull request with a new target base due to a merge or
a rebase. The incremental webrev excludes the unrelated changes brought in by
the merge/rebase. The pull request contains five additional commits since the
last revision:
- Merge remote-tracking branch 'upstream/master' into JDK-8276799
- Fix copyright header
- Address review comments
- Merge remote-tracking branch 'upstream/master' into JDK-8276799
- 8276799: Implementation of JEP 422: Linux/RISC-V Port
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6294/files
- new: https://git.openjdk.java.net/jdk/pull/6294/files/d8bef7fa..90db70eb
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=6294=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=6294=03-04
Stats: 3082 lines in 147 files changed: 1635 ins; 374 del; 1073 mod
Patch: https://git.openjdk.java.net/jdk/pull/6294.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6294/head:pull/6294
PR: https://git.openjdk.java.net/jdk/pull/6294
