RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics
This is needed for performance improvements in support of virtual threads. The update includes the following: 1. Refactored the `VirtualThread` native methods: `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => `notifyJvmtiMount` `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => `notifyJvmtiUnmount` 2. Still useful implementation of old native methods is moved from `jvm.cpp` to `jvmtiThreadState.cpp`: `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. 4. Removed the`VirtualThread` static boolean state variable `notifyJvmtiEvents` and its support in `javaClasses`. 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` `notifyJvmtiEvents` variable. Implementing the same methods as C1 intrinsics can be needed in the future but is a low priority for now. Testing: - Ran mach5 tiers 1-6. No regressions were found. - Commit messages: - fix traling spaces in a couple of files - minor update for VTMS_notify_jvmti_events variable - 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics Changes: https://git.openjdk.org/jdk/pull/13054/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13054=00 Issue: https://bugs.openjdk.org/browse/JDK-8304303 Stats: 438 lines in 20 files changed: 265 ins; 125 del; 48 mod Patch: https://git.openjdk.org/jdk/pull/13054.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13054/head:pull/13054 PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v3]
On Wed, 15 Mar 2023 22:49:57 GMT, Leonid Mesnik wrote:
>> The StreamPumper is fixed to process the last line even it is not finishes
>> with '\n' or '\r'. The test included. Testing with tier1-3 also to verify
>> that tests are not broken.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> comments added
A few minor suggestions.
The additional commentary is very helpful. Figuring out exactly how this code
works is painful.
Thanks.
test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 50:
> 48:
> 49: // The line which exceeds internal StreamPumper buffer (256 bytes)
> 50: String VERY_LONG_LINE = "VERYLONGLINE".repeat(30);
It might be a bit more obvious that this does exceed 256 by simply doing:
String veryLongLine = "X".repeat(257);
test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 54:
> 52: System.out.print(args[0]);
> 53: } else {
> 54: test("ARG1");
Some boundary testing here would be:
test("\n");
test("\nARG1");
test("\nARG1\n");
test("ARG1/n");
test("ARG1");
test/lib/jdk/test/lib/process/StreamPumper.java line 99:
> 97: * Implements Thread.run(). Continuously read from {@code in} and
> write to
> 98: * {@code out} until {@code in} has reached end of stream.
> 99: * Additionally this method also split data read from buffer into the
> lines and process each line using linePumps.
A few grammatical nits. I suggest:
> Additionally this method also splits the data read from the buffer into
> lines, and processes each line using linePumps.
test/lib/jdk/test/lib/process/StreamPumper.java line 138:
> 136: }
> 137: // The remaining after last '\n' (lastcrlf position)
> buffer data is written into lineBos.
> 138: // The end of this line from next buffer is
> concatenated to this data in the next iteration.
In trying to help my own understanding here I suggest:
// If no crlf was found, or there was additional data after the last crlf was
found, then write the leftover data
// in lineBos. If there is more data to read it will be concatenated with the
current data on the next iteration.
// If there is no more data, or no more crlf found, all the remaining data will
be processed after the loop, as the
// final line.
test/lib/jdk/test/lib/process/StreamPumper.java line 149:
> 147: }
> 148: // Process data remaining after last '\n' in the last buffer.
> 149: // It is already written in the lineBos buffer but not
> processed because '\n' hasn't been met.
Suggestion:
// If there was no terminating crlf the remaining data has been written to
lineBos, but this final line of data
// now needs to be processed by the linePumper.
-
PR: https://git.openjdk.org/jdk/pull/13034
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v8]
On Wed, 15 Mar 2023 16:42:53 GMT, Adam Sotona wrote:
>> jdk.jlink internal plugins are heavily using ASM
>>
>> This patch converts ASM calls to Classfile API.
>>
>> Please review.
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed SystemModulesPlugin
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/IncludeLocalesPlugin.java
line 160:
> 158:
> resource.type().equals(ResourcePoolEntry.Type.CLASS_OR_RESOURCE)) {
> 159: byte[] bytes = resource.contentBytes();
> 160: if (newClassReader(path, bytes).interfaces().stream()
`IncludeLocalesPluginTest.java` test is failing. Adding
`-J-Djlink.debug=true` will get the stack trace:
Failed to parse class file:
/jdk.localedata/sun/text/resources/ext/LineBreakIteratorData_th
java.lang.IllegalStateException: Bad magic number
at
java.base/jdk.internal.classfile.impl.ClassReaderImpl.(ClassReaderImpl.java:94)
at
java.base/jdk.internal.classfile.impl.ClassImpl.(ClassImpl.java:68)
at java.base/jdk.internal.classfile.Classfile.parse(Classfile.java:165)
at
jdk.jlink/jdk.tools.jlink.internal.plugins.AbstractPlugin.newClassReader(AbstractPlugin.java:105)
at
jdk.jlink/jdk.tools.jlink.internal.plugins.IncludeLocalesPlugin.lambda$transform$5(IncludeLocalesPlugin.java:161)
at
jdk.jlink/jdk.tools.jlink.plugin.ResourcePool.lambda$transformAndCopy$0(ResourcePool.java:112)
at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
at
java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1924)
at
java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
at
jdk.jlink/jdk.tools.jlink.plugin.ResourcePool.transformAndCopy(ResourcePool.java:111)
at
jdk.jlink/jdk.tools.jlink.internal.plugins.IncludeLocalesPlugin.transform(IncludeLocalesPlugin.java:154)
at
jdk.jlink/jdk.tools.jlink.internal.ImagePluginStack.visitResources(ImagePluginStack.java:262)
at
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.generateJImage(ImageFileCreator.java:184)
at
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.writeImage(ImageFileCreator.java:163)
at
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.create(ImageFileCreator.java:100)
at
jdk.jlink/jdk.tools.jlink.internal.JlinkTask$ImageHelper.retrieve(JlinkTask.java:889)
at
jdk.jlink/jdk.tools.jlink.internal.ImagePluginStack.operate(ImagePluginStack.java:194)
at
jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:452)
at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:292)
at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:56)
at jdk.jlink/jdk.tools.jlink.internal.Main.main(Main.java:34)
Dumping class file
26549-include-locales//jdk.localedata/sun/text/resources/ext/LineBreakIteratorData_th
java.lang.IllegalStateException: Bad magic number
at
java.base/jdk.internal.classfile.impl.ClassReaderImpl.(ClassReaderImpl.java:94)
at
java.base/jdk.internal.classfile.impl.ClassImpl.(ClassImpl.java:68)
at java.base/jdk.internal.classfile.Classfile.parse(Classfile.java:165)
at
jdk.jlink/jdk.tools.jlink.internal.plugins.AbstractPlugin.newClassReader(AbstractPlugin.java:105)
at
jdk.jlink/jdk.tools.jlink.internal.plugins.IncludeLocalesPlugin.lambda$transform$5(IncludeLocalesPlugin.java:161)
at
jdk.jlink/jdk.tools.jlink.plugin.ResourcePool.lambda$transformAndCopy$0(ResourcePool.java:112)
at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
at
java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1924)
at
java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
at
jdk.jlink/jdk.tools.jlink.plugin.ResourcePool.transformAndCopy(ResourcePool.java:111)
at
jdk.jlink/jdk.tools.jlink.internal.plugins.IncludeLocalesPlugin.transform(IncludeLocalesPlugin.java:154)
at
jdk.jlink/jdk.tools.jlink.internal.ImagePluginStack.visitResources(ImagePluginStack.java:262)
at
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.generateJImage(ImageFileCreator.java:184)
at
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.writeImage(ImageFileCreator.java:163)
at
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.create(ImageFileCreator.java:100)
at
jdk.jlink/jdk.tools.jlink.internal.JlinkTask$ImageHelper.retrieve(JlinkTask.java:889)
at
jdk.jlink/jdk.tools.jlink.internal.ImagePluginStack.operate(ImagePluginStack.java:194)
at
jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:452)
at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:292)
at
Integrated: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle
On Tue, 7 Mar 2023 18:02:41 GMT, Jorn Vernee wrote: > The issue is that the size of the code buffer is not large enough to hold the > whole stub. > > Proposed solution is to scale the size of the stub with the number of > arguments. I've adjusted sizes for both downcall and upcall stubs. I've also > dropped the number of relocations, since we're not really using any for > downcalls, and for upcalls we only have 1 AFAICS. (the size of the > relocations can not be zero however, as that leads to the relocation section > [not being initialized][1], and triggering [an assert][2] later when the code > blob is copied). > > The way I've determined the new base size and per-argument size for stubs, is > by first linking a stub without any arguments to get the required base size, > and by then adding 20 `double` arguments to get a rough per-argument size. > Both values have wiggle room as well. The sizes can be printed using e.g. > `-XX:+LogCompilation`, and then looking for `nep_invoker_blob` and > `upcall_stub*` in the log file. This experiment was done on a fastdebug build > to account for additional debug code being generated. The included test is > designed to try and maximize the size of the generated stub. > > I've also updated `CodeBuffer::log_section_sizes` to print the in-use size, > rather than just the capacity and free space. > > [1]: > https://github.com/openjdk/jdk/blob/56512cfe1f0682c98ba3488af3d03ccef632c016/src/hotspot/share/asm/codeBuffer.cpp#L119-L121 > [2]: > https://github.com/openjdk/jdk/blob/56512cfe1f0682c98ba3488af3d03ccef632c016/src/hotspot/share/asm/codeBuffer.cpp#L675 This pull request has now been integrated. Changeset: 2b81faeb Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/2b81faeb3514060e6c8c950ef4e39e299c43199d Stats: 102 lines in 8 files changed: 81 ins; 0 del; 21 mod 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle Reviewed-by: kvn, vlivanov - PR: https://git.openjdk.org/jdk/pull/12908
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v3]
> The StreamPumper is fixed to process the last line even it is not finishes > with '\n' or '\r'. The test included. Testing with tier1-3 also to verify > that tests are not broken. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: comments added - Changes: - all: https://git.openjdk.org/jdk/pull/13034/files - new: https://git.openjdk.org/jdk/pull/13034/files/7e0305d1..aa12782b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13034=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13034=01-02 Stats: 7 lines in 1 file changed: 5 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13034.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13034/head:pull/13034 PR: https://git.openjdk.org/jdk/pull/13034
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v6]
On Wed, 15 Mar 2023 16:37:54 GMT, Adam Sotona wrote:
>> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
>> line 614:
>>
>>> 612: private void genConstructor(ClassBuilder clb) {
>>> 613: clb.withMethod("", MethodTypeDesc.of(CD_void),
>>> 614: ACC_PUBLIC, mb ->
>>> mb.withFlags(ACC_PUBLIC).withCode( cob -> {
>>
>> Why `withFlags(ACC_PUBLIC)` when the flags is already given in
>> `withMethod`'s 3rd argument? Same for occurrences below.
>
> It has been redundant, now more effective `withMethodBody` method is used.
I also had the same comment in PR
[12945](https://github.com/openjdk/jdk/pull/12945). Similar change should
apply to PR 12945.
-
PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v8]
On Wed, 15 Mar 2023 16:42:53 GMT, Adam Sotona wrote: >> jdk.jlink internal plugins are heavily using ASM >> >> This patch converts ASM calls to Classfile API. >> >> Please review. >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > fixed SystemModulesPlugin These convenient methods really simplify it and improve the readability. This patch seems to fix `BasicTest.java` test failure I mentioned to you offline. What was the issue with the previous revision? $ JTwork/scratch/mysmallimage/bin/java -Xlog:init=debug -m test/jdk.test.Test Error occurred during initialization of boot layer java.lang.InternalError: java.lang.ClassNotFoundException: jdk/internal/module/SystemModules$all at java.base/jdk.internal.module.SystemModuleFinders.systemModules(SystemModuleFinders.java:132) at java.base/jdk.internal.module.ModuleBootstrap.boot2(ModuleBootstrap.java:228) at java.base/jdk.internal.module.ModuleBootstrap.boot(ModuleBootstrap.java:174) at java.base/java.lang.System.initPhase2(System.java:) Caused by: java.lang.ClassNotFoundException: jdk/internal/module/SystemModules$all at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:391) at java.base/java.lang.Class.forName(Class.java:382) at java.base/jdk.internal.module.SystemModuleFinders.systemModules(SystemModuleFinders.java:129) ... 3 more - PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8303018: Unicode Emoji Properties [v3]
On Wed, 15 Mar 2023 20:43:22 GMT, Eirik Bjorsnos wrote: > Unrelated side note: Reviewing this PR inspired me to see if one could > generate more efficient switch expressions for the CharacterDataLatin1 > methods as replacement for the property lookup / masking done today. This > seemed to give a small improvements on benchmarks and also collapsed a few > methods to simply "return false". Could be something to explore at a later > point. Yeah, agree. `Latin1` can be (should be?) fast-path'ed as always. - PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8304161: Add TypeKind.from to derive from TypeDescriptor.OfField
On Tue, 14 Mar 2023 17:12:50 GMT, Chen Liang wrote: > Such an API allows creating TypeKind from both Class and ClassDesc than > having to call descriptorString() explicitly at every use site. > > See > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000240.html > for context. > > Upgraded usages of `fromDescriptor` to `from` in applicable use sites in > Classfile API and its tests. +1 - PR: https://git.openjdk.org/jdk/pull/13024
Re: RFR: 8304161: Add TypeKind.from to derive from TypeDescriptor.OfField
On Tue, 14 Mar 2023 17:12:50 GMT, Chen Liang wrote: > Such an API allows creating TypeKind from both Class and ClassDesc than > having to call descriptorString() explicitly at every use site. > > See > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000240.html > for context. > > Upgraded usages of `fromDescriptor` to `from` in applicable use sites in > Classfile API and its tests. LGTM. I think @briangoetz might want to weigh in on this as well. - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.org/jdk/pull/13024
Re: RFR: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
On Wed, 15 Mar 2023 02:18:37 GMT, Chen Liang wrote: > Add Constant Descriptors for DirectMethodHandleDesc of > MethodHandles::classData and classDataAt in ConstantDescs. This facilitates > easier access of class data via condy in Classfile API-generated bytecode. > Most other constant bootstrap methods provided in the JDK, notably from > `ConstantBootstraps`, already have constant descriptors in ConstantDescs. > > Context: > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000235.html P.S. we don't seem to have a test for this class at the moment (except indirectly), but it might be nice to add a test that iterates over all the fields in this class, and tries to resolve them, to make sure that that works. - PR: https://git.openjdk.org/jdk/pull/13033
Re: RFR: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
On Wed, 15 Mar 2023 02:18:37 GMT, Chen Liang wrote: > Add Constant Descriptors for DirectMethodHandleDesc of > MethodHandles::classData and classDataAt in ConstantDescs. This facilitates > easier access of class data via condy in Classfile API-generated bytecode. > Most other constant bootstrap methods provided in the JDK, notably from > `ConstantBootstraps`, already have constant descriptors in ConstantDescs. > > Context: > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000235.html Looks good. I've added myself as a reviewer on the CSR as well. - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.org/jdk/pull/13033
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v16]
On Tue, 14 Mar 2023 22:52:44 GMT, Martin Doerr wrote: >> Martin Doerr has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix storing 32 bit integers into Java frames. Enable TestArrayStructs. > > Btw. the new cases in which we use int and short accesses when byteWidth is > not a power of 2 are never unaligned AFAICS. I guess _UNALIGNED is > unnecessary in the JAVA_INT_UNALIGNED and JAVA_SHORT_UNALIGNED. They are > always aligned wrt. to their size. @TheRealMDoerr I've approved the PR. I suggest for a second reviewer to try and get someone who knows the PPC port. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle [v3]
On Fri, 10 Mar 2023 14:14:55 GMT, Jorn Vernee wrote: >> The issue is that the size of the code buffer is not large enough to hold >> the whole stub. >> >> Proposed solution is to scale the size of the stub with the number of >> arguments. I've adjusted sizes for both downcall and upcall stubs. I've also >> dropped the number of relocations, since we're not really using any for >> downcalls, and for upcalls we only have 1 AFAICS. (the size of the >> relocations can not be zero however, as that leads to the relocation section >> [not being initialized][1], and triggering [an assert][2] later when the >> code blob is copied). >> >> The way I've determined the new base size and per-argument size for stubs, >> is by first linking a stub without any arguments to get the required base >> size, and by then adding 20 `double` arguments to get a rough per-argument >> size. Both values have wiggle room as well. The sizes can be printed using >> e.g. `-XX:+LogCompilation`, and then looking for `nep_invoker_blob` and >> `upcall_stub*` in the log file. This experiment was done on a fastdebug >> build to account for additional debug code being generated. The included >> test is designed to try and maximize the size of the generated stub. >> >> I've also updated `CodeBuffer::log_section_sizes` to print the in-use size, >> rather than just the capacity and free space. >> >> [1]: >> https://github.com/openjdk/jdk/blob/56512cfe1f0682c98ba3488af3d03ccef632c016/src/hotspot/share/asm/codeBuffer.cpp#L119-L121 >> [2]: >> https://github.com/openjdk/jdk/blob/56512cfe1f0682c98ba3488af3d03ccef632c016/src/hotspot/share/asm/codeBuffer.cpp#L675 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > RISCV changes Thanks for the reviews. I'm running one more round of tests before integrating. - PR: https://git.openjdk.org/jdk/pull/12908
Re: RFR: 8303018: Unicode Emoji Properties [v3]
On Wed, 15 Mar 2023 18:21:11 GMT, Naoto Sato wrote: >> Proposing accessor methods to Emoji properties defined in [Unicode Technical >> Standard #51](https://unicode.org/reports/tr51/) in `java.lang.Character` >> class. This is per a request from the client group, as well as refining the >> currently existing ad-hoc emoji implementation in regex. A CSR has also been >> drafted, and I would appreciate reviews for it too. > > Naoto Sato has updated the pull request incrementally with two additional > commits since the last revision: > > - InternalError message/mask constants cleanup > - Indentation/print comment fix Unrelated side note: Reviewing this PR inspired me to see if one could generate more efficient switch expressions for the CharacterDataLatin1 methods as replacement for the property lookup / masking done today. This seemed to give a small improvements on benchmarks and also collapsed a few methods to simply "return false". Could be something to explore at a later point. - PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v6]
On Wed, 15 Mar 2023 19:23:52 GMT, Joe Darcy wrote: > I assume https://bugs.openjdk.org/browse/JDK-8303431 recounts the motivation > behind this change? Yes, it does. Thanks in advance. - PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
On Thu, 23 Feb 2023 09:04:23 GMT, Justin Lu wrote: > This PR converts Unicode sequences to UTF-8 native in .properties file. > (Excluding the Unicode space and tab sequence). The conversion was done using > native2ascii. > > In addition, the build logic is adjusted to support reading in the > .properties files as UTF-8 during the conversion from .properties file to > .java ListResourceBundle file. test/jdk/java/text/Format/NumberFormat/CurrencySymbols.properties line 156: > 154: zh=\u00A4 > 155: zh_CN=\uFFE5 > 156: zh_HK=HK$ Why are they not encoded into UTF-8 native? - PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: 8304225: Remove javax/script/Test7.java from ProblemList
On Wed, 15 Mar 2023 06:16:17 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes > `javax/script/Test7.java` from the ProblemList? > > As noted in https://bugs.openjdk.org/browse/JDK-8304225, this test no longer > fails and passes just like the other tests in `javax/script` directory with: > > > --System.out:(4/60)-- > > Test7 > > Warning: No js engine found; test vacuously passes. > > > With this proposed change, relevant tier testing has been run to verify that > the original reported issue which caused this test to be problem listed > https://bugs.openjdk.org/browse/JDK-8239361 is no longer seen. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.org/jdk/pull/13035
Re: RFR: 8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR
On Tue, 14 Mar 2023 22:16:45 GMT, Justin Lu wrote:
> This PR fixes the bug which occurred when `Calendar.roll(WEEK_OF_YEAR)`
> rolled into a minimal first week with an invalid `WEEK_OF_YEAR` and
> `DAY_OF_WEEK` combo.
>
> For example, Rolling _Monday, 30 December 2019_ by 1 week produced _Monday,
> 31 December 2018_, which is incorrect. This is because `WEEK_OF_YEAR` is
> rolled from 52 to 1, and the original `DAY_OF_WEEK` is 1. However, there is
> no Monday in week 1 of 2019. This is exposed when a future method calls
> `Calendar.complete()`, which eventually calculates a `fixedDate` with the
> invalid `WEEK_OF_YEAR` and `DAY_OF_WEEK` combo.
>
> To prevent this, a check is added for rolls into week 1, which determines if
> the first week is minimal. If it is indeed minimal, then it is checked if
> `DAY_OF_WEEK` exists in that week, if not, `WEEK_OF_YEAR` must be incremented
> by one.
>
> After the fix, Rolling _Monday, 30 December 2019_ by 1 week produces _Monday,
> 7 January 2019_
Hi Justin,
Thanks for the fix. Still reviewing the changes, but here are some comments I
have noticed:
src/java.base/share/classes/java/util/GregorianCalendar.java line 1314:
> 1312: // current DAY_OF_WEEK
> 1313: if (newWeekOfYear == 1 && isInvalidWeek1()) {
> 1314: newWeekOfYear+=1;
is `+1` always correct? Does it work when the amount is negative?
src/java.base/share/classes/java/util/GregorianCalendar.java line 3030:
> 3028: }
> 3029: }
> 3030:
Are these `GregorianCalendar` specific? What about other calendars?
test/jdk/java/util/Calendar/RollToMinWeek.java line 30:
> 28: * is rolled into a minimal week 1
> 29: * @run junit RollToMinWeek
> 30: */
Have you considered adding test cases into
`test/jdk/java/util/Calendar/CalendarTestScripts`, instead of creating a
single-purpose test case?
test/jdk/java/util/Calendar/RollToMinWeek.java line 79:
> 77: return Stream.of(
> 78: // Test a variety of rolls that previously produced
> incorrect results
> 79: Arguments.of(buildCalendar(27, 11, 2020, Locale.ENGLISH),
The first day of week depends on the region, not the language. In fact, I would
prefer explicitly specifying it via a locale like `en-US-u-fw-mon`, and testing
all the weekdays.
test/jdk/java/util/Calendar/RollToMinWeek.java line 95:
> 93:
> 94: private static Calendar buildCalendar(int day, int month, int year,
> Locale locale) {
> 95: Calendar calendar = Calendar.getInstance(locale);
If the fix is `GregorianCalendar` specific, should it check the type?
-
PR: https://git.openjdk.org/jdk/pull/13031
Re: RFR: 8291598: Matcher.appendReplacement should not create new StringBuilder instances [v2]
On Wed, 15 Mar 2023 18:55:35 GMT, Roger Riggs wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8291598: Matcher.appendReplacement should not create new StringBuilder
>> instances
>>
>> Fix copyright year
>
> src/java.base/share/classes/java/util/regex/Matcher.java line 1066:
>
>> 1064: } else {
>> 1065: break;
>> 1066: }
>
> Possibly remove another SB allocation; remember start and end and use
> `replacement.substring(start, end)`.
Thanks @RogerRiggs, addressed in latest commit.
-
PR: https://git.openjdk.org/jdk/pull/13048
Re: RFR: 8291598: Matcher.appendReplacement should not create new StringBuilder instances [v3]
> Remove instantiation of `StringBuilder` Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8291598: Matcher.appendReplacement should not create new StringBuilder instances Removed other allocations of StringBuilder when processing named groups in replacement string. - Changes: - all: https://git.openjdk.org/jdk/pull/13048/files - new: https://git.openjdk.org/jdk/pull/13048/files/c9432a69..4b5ac595 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13048=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13048=01-02 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13048.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13048/head:pull/13048 PR: https://git.openjdk.org/jdk/pull/13048
Re: RFR: 8304283: Modernize the switch statements in jdk.internal.foreign
On Wed, 15 Mar 2023 18:10:29 GMT, Per Minborg wrote:
> This PR proposes changing old-type switch statements to newer forms of switch.
src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
line 219:
> 217: case Binding.Cast unused-> true;
> 218: };
> 219: }
I'd go a bit further here and visually organize the cases as well. Also, using
a static import for `Binding.*`:
Suggestion:
static boolean isUnbox(Binding binding) {
return switch (binding) {
case VMStore unused -> true;
case BufferLoadunused -> true;
case Copy unused -> true;
case UnboxAddress unused -> true;
case Dup unused -> true;
case Cast unused -> true;
case VMLoad unused -> false;
case BufferStore unused -> false;
case Allocate unused -> false;
case BoxAddress unused -> false;
};
}
It's a bit unfortunate that we need variable names as well.
-
PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8304283: Modernize the switch statements in jdk.internal.foreign
On Wed, 15 Mar 2023 19:33:19 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
>> line 208:
>>
>>> 206: static boolean isUnbox(Binding binding) {
>>> 207: return switch (binding) {
>>> 208: case Binding.VMStore unused -> true;
>>
>> I wonder... here it might be better to capture the box/unbox nature of a
>> binding in a virtual method in the binding class? E.g. isBox(), isUnbox() ?
>
> I prefer this approach, to be honest, since the logic for all the different
> binding operators is in one place. It gives an overview of which operators
> are expected in an unboxing recipe. Making them virtual methods would put
> quite a bit of visual distance between the different `true`/`false` values.
>
> I've been through the Binding file quite a bit, and the amount of
> scrolling/searching needed to find a particular implementation is kind of
> annoying.
To pull on that string some more. I think we should move the implementations of
various Binding::verify methods here, or perhaps into a separate
BindingVerifier class.
Ditto for the Binding::interpret implementations. They could be moved to
BindingInterpreter. The Binding class would just be left as a simple bag of
records + documentation for each operator.
The main motivation for this would be that, if you're looking at e.g.
interpretation in `Binding`, there's a lot of noise that you have to filter
through. I think it makes more sense to group these things into classes (for
specialization/verification/interpretation), where all the code in a class is
logically related.
-
PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8304283: Modernize the switch statements in jdk.internal.foreign
On Wed, 15 Mar 2023 18:33:34 GMT, Maurizio Cimadamore
wrote:
>> This PR proposes changing old-type switch statements to newer forms of
>> switch.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
> line 208:
>
>> 206: static boolean isUnbox(Binding binding) {
>> 207: return switch (binding) {
>> 208: case Binding.VMStore unused -> true;
>
> I wonder... here it might be better to capture the box/unbox nature of a
> binding in a virtual method in the binding class? E.g. isBox(), isUnbox() ?
I prefer this approach, to be honest, since the logic for all the different
binding operators is in one place. It gives an overview of which operators are
expected in an unboxing recipe. Making them virtual methods would put quite a
bit of visual distance between the different `true`/`false` values.
I've been through the Binding file quite a bit, and the amount of
scrolling/searching needed to find a particular implementation is kind of
annoying.
-
PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v6]
On Wed, 15 Mar 2023 18:00:49 GMT, Doug Simon wrote: > @jddarcy would you be able to help review this PR? Based on git log, you are > knowledgeable in `sun.reflect.annotation`. If not, please suggest who else I > can reach out to for a review. I can take a look at this, but it will be at least a few days before I can swap it in. I've also worked on the annotation-reading API in javax.lang.model. I assume https://bugs.openjdk.org/browse/JDK-8303431 recounts the motivation behind this change? - PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v17]
On Wed, 15 Mar 2023 18:53:08 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update: Will
>> get addressed separately:
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separate RFE). Update: This issue is
>> resolved by 2nd commit.
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix Copyright format.
Marked as reviewed by jvernee (Reviewer).
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303018: Unicode Emoji Properties [v3]
On Wed, 15 Mar 2023 18:21:11 GMT, Naoto Sato wrote: >> Proposing accessor methods to Emoji properties defined in [Unicode Technical >> Standard #51](https://unicode.org/reports/tr51/) in `java.lang.Character` >> class. This is per a request from the client group, as well as refining the >> currently existing ad-hoc emoji implementation in regex. A CSR has also been >> drafted, and I would appreciate reviews for it too. > > Naoto Sato has updated the pull request incrementally with two additional > commits since the last revision: > > - InternalError message/mask constants cleanup > - Indentation/print comment fix Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8291598: Matcher.appendReplacement should not create new StringBuilder instances [v2]
On Wed, 15 Mar 2023 18:47:09 GMT, Raffaello Giulietti
wrote:
>> Remove instantiation of `StringBuilder`
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8291598: Matcher.appendReplacement should not create new StringBuilder
> instances
>
> Fix copyright year
src/java.base/share/classes/java/util/regex/Matcher.java line 1066:
> 1064: } else {
> 1065: break;
> 1066: }
Possibly remove another SB allocation; remember start and end and use
`replacement.substring(start, end)`.
-
PR: https://git.openjdk.org/jdk/pull/13048
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v17]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update: Will
> get addressed separately:
> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE). Update: This issue is resolved by
> 2nd commit.
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Fix Copyright format.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/9173af20..5320f895
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=16
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=15-16
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8291598: Matcher.appendReplacement should not create new StringBuilder instances [v2]
> Remove instantiation of `StringBuilder` Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8291598: Matcher.appendReplacement should not create new StringBuilder instances Fix copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/13048/files - new: https://git.openjdk.org/jdk/pull/13048/files/ea2eebe9..c9432a69 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13048=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13048=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13048.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13048/head:pull/13048 PR: https://git.openjdk.org/jdk/pull/13048
Re: RFR: JDK-8304063: tools/jpackage/share/AppLauncherEnvTest.java fails when checking LD_LIBRARY_PATH
On Wed, 15 Mar 2023 11:46:22 GMT, Matthias Baesken wrote:
> The test fails on Alpine Linux 3.17, when checking the environment variable
> LD_LIBRARY_PATH; looks like the actual env variable is much longer than the
> test expects. It turned out that at least on Linux (probably also on other OS
> like AIX) the actual env variable has the expected string at it's end, but
> might contain more path entries ( LD_LIBRARY_PATH can be adjusted by jvm -
> https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjli/java_md.c
> ).
Changes requested by asemenyuk (Reviewer).
test/jdk/tools/jpackage/share/AppLauncherEnvTest.java line 93:
> 91: } else {
> 92: TKit.assertEquals(expectedEnvVarValue, actualEnvVarValue,
> msg);
> 93: }
I'd keep the check as a single statement:
TKit.assertTextStream(expectedEnvVarValue)
.predicate(TKit.isLinux() ? String::endsWith : String::equals)
.label(String.format("value of %s env variable", envVarName))
.apply(Stream.of(actualEnvVarValue));
It also will produce a nicer log record than `TKit.assertTrue(false, msg);`
-
PR: https://git.openjdk.org/jdk/pull/13041
Re: RFR: 8304283: Modernize the switch statements in jdk.internal.foreign
On Wed, 15 Mar 2023 18:10:29 GMT, Per Minborg wrote:
> This PR proposes changing old-type switch statements to newer forms of switch.
Overall looks good. I've added a couple of optional comments for your
consideration.
src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
line 208:
> 206: static boolean isUnbox(Binding binding) {
> 207: return switch (binding) {
> 208: case Binding.VMStore unused -> true;
I wonder... here it might be better to capture the box/unbox nature of a
binding in a virtual method in the binding class? E.g. isBox(), isUnbox() ?
src/java.base/share/classes/jdk/internal/foreign/abi/riscv64/linux/TypeClass.java
line 112:
> 110: return
> flatten(sequenceLayout.elementLayout()).mul(elementCount);
> 111: }
> 112: case null, default -> throw new
> IllegalStateException("Cannot get here: " + layout);
Since the default throws, and the switch w/o a `case null` also throws, do we
need the `case null` here?
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8291598: Matcher.appendReplacement should not create new StringBuilder instances
On Wed, 15 Mar 2023 18:15:02 GMT, Raffaello Giulietti wrote: > Remove instantiation of `StringBuilder` Benchmark for the example in the documentation for `Matcher.appendReplacement()` before BenchmarkMode CntScore Error Units AppendReplacement.testAppendReplacement avgt 15 177.029 ± 6.294 ns/op after BenchmarkMode CntScore Error Units AppendReplacement.testAppendReplacement avgt 15 142.373 ± 1.684 ns/op Same example, but with pattern `"(cat)"` (1 capturing group), matcher on the input repeated 1000 times, and replacement string `"$1dog$1cat$1mouse"` (3 back references). before BenchmarkMode Cnt Score Error Units AppendReplacement.testAppendReplacement avgt 15 262576.335 ± 3718.905 ns/op after BenchmarkMode Cnt Score Error Units AppendReplacement.testAppendReplacement avgt 15 225700.642 ± 683.721 ns/op - PR: https://git.openjdk.org/jdk/pull/13048
RFR: 8291598: Matcher.appendReplacement should not create new StringBuilder instances
Remove instantiation of `StringBuilder` - Commit messages: - 8291598: Matcher.appendReplacement should not create new StringBuilder instances Changes: https://git.openjdk.org/jdk/pull/13048/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13048=00 Issue: https://bugs.openjdk.org/browse/JDK-8291598 Stats: 152 lines in 1 file changed: 62 ins; 52 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/13048.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13048/head:pull/13048 PR: https://git.openjdk.org/jdk/pull/13048
Re: RFR: 8303018: Unicode Emoji Properties [v2]
On Tue, 14 Mar 2023 17:09:54 GMT, Eirik Bjorsnos wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed method descriptions > > make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java > line 215: > >> 213: maskEmojiComponent = 0x0400L, >> 214: maskExtendedPictographic = 0x0800L; >> 215: > > Would be nice the have the '=' aligned in the same column as the ones above, > or at least have the emoji ones align with each other. Thanks. Indentation aligned, and although in production `-string` is used so comments are not emitted, added information for `B` table in `GenerateCharacter.propertiesComments` for consistency. - PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8303018: Unicode Emoji Properties [v2]
On Tue, 14 Mar 2023 20:47:55 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed method descriptions
>
> make/jdk/src/classes/build/tools/generatecharacter/EmojiData.java line 99:
>
>> 97: case "Emoji_Component" -> EMOJI_COMPONENT;
>> 98: case "Extended_Pictographic" -> EXTENDED_PICTOGRAPHIC;
>> 99: default -> throw new InternalError();
>
> It would be useful to include the "type" as the exception argument. It give
> some idea as to the corruption or missing case.
Added `type` to the error message
> make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java
> line 214:
>
>> 212: maskEmojiModifierBase = 0x0200L,
>> 213: maskEmojiComponent = 0x0400L,
>> 214: maskExtendedPictographic = 0x0800L;
>
> It would be good to leverage a common definition (perhaps a bit number) here
> and in EmojiData.java
> and build the constants with <<< shifts.
Good point. I managed to get rid of the constants in `EmojiData` altogether, by
using the constants in `GenerateCharacter`. Used the bit numbers to construct
constants.
> make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java
> line 810:
>
>> 808: if (x.equals("maskEmojiModifierBase")) return "0x" +
>> hex4(maskEmojiModifierBase >> 32);
>> 809: if (x.equals("maskEmojiComponent")) return "0x" +
>> hex4(maskEmojiComponent >> 32);
>> 810: if (x.equals("maskExtendedPictographic")) return "0x" +
>> hex4(maskExtendedPictographic >> 32);
>
> An upgrade would be to modify hex4(), hexNN() to use
> `HexFormat.of().toUpperCase().toHexDigits((short)xxx)`
> The HexFormat is reusable and would avoid creating extra strings.
> Perhaps also create a method that combines the repetitive shift and prefixing.
>
> This if...then... sequence could be an expression switch (x) {...}.
It would be good, but it would be for another day IMO.
-
PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8303018: Unicode Emoji Properties [v3]
> Proposing accessor methods to Emoji properties defined in [Unicode Technical > Standard #51](https://unicode.org/reports/tr51/) in `java.lang.Character` > class. This is per a request from the client group, as well as refining the > currently existing ad-hoc emoji implementation in regex. A CSR has also been > drafted, and I would appreciate reviews for it too. Naoto Sato has updated the pull request incrementally with two additional commits since the last revision: - InternalError message/mask constants cleanup - Indentation/print comment fix - Changes: - all: https://git.openjdk.org/jdk/pull/13006/files - new: https://git.openjdk.org/jdk/pull/13006/files/0b7b7296..6b4eef94 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13006=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13006=01-02 Stats: 66 lines in 2 files changed: 36 ins; 9 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/13006.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13006/head:pull/13006 PR: https://git.openjdk.org/jdk/pull/13006
RFR: 8304283: Modernize the switch statements in jdk.internal.foreign
This PR proposes changing old-type switch statements to newer forms of switch. - Commit messages: - Update copyright years - Modernize the switch statements in jdk.internal.foreign Changes: https://git.openjdk.org/jdk/pull/13047/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13047=00 Issue: https://bugs.openjdk.org/browse/JDK-8304283 Stats: 214 lines in 8 files changed: 23 ins; 96 del; 95 mod Patch: https://git.openjdk.org/jdk/pull/13047.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13047/head:pull/13047 PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v6]
On Tue, 14 Mar 2023 16:06:06 GMT, Doug Simon wrote:
>> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for
>> accessing annotations. The main differences from
>> `java.lang.reflect.AnnotatedElement` are:
>> * All methods in the `Annotated` interface explicitly specify requested
>> annotation type(s). That is, there is no equivalent of
>> `AnnotatedElement.getAnnotations()`.
>> * Annotation data is returned in a map-like object (of type
>> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
>> works better for libgraal as it avoids the need for annotation types to be
>> loaded and included in libgraal.
>>
>> To demonstrate the new API, here's an example in terms
>> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>>
>> ResolvedJavaMethod method = ...;
>> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
>> return switch (a.kind()) {
>> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
>> case FULL_UNROLL_UNTIL_RETURN ->
>> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
>> ...
>> }
>>
>>
>> The same code using the new API:
>>
>>
>> ResolvedJavaMethod method = ...;
>> ResolvedJavaType explodeLoopType = ...;
>> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
>> return switch (a.getEnum("kind").getName()) {
>> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
>> case "FULL_UNROLL_UNTIL_RETURN" ->
>> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
>> ...
>> }
>>
>>
>> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
>> parsing annotations and serializing/deserializing to/from a byte array. This
>> allows the annotation data to be passed from the HotSpot heap to the
>> libgraal heap.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> addressed review feedback
@jddarcy would you be able to help review this PR? Based on git log, you are
knowledgeable in `sun.reflect.annotation`. If not, please suggest who else I
can reach out to for a review.
-
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v16]
On Tue, 14 Mar 2023 22:52:44 GMT, Martin Doerr wrote: > Btw. the new cases in which we use int and short accesses when byteWidth is > not a power of 2 are never unaligned AFAICS. I guess _UNALIGNED is > unnecessary in the JAVA_INT_UNALIGNED and JAVA_SHORT_UNALIGNED. They are > always aligned wrt. to their size. They are not necessarily aligned, for instance of the struct we are given is not aligned itself (at least in the reading case). Though we currently reject that case in AbstractLinker, it is something we might want to allow, and we have been looking at that as of late, for instance to pass packed structs to native code. FWIW, using the unaligned variant effectively just turns off the alignment checks we do. So we have less safety, in theory. There should be no performance difference though just from using the _UNALIGNED layout. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v16]
On Tue, 14 Mar 2023 22:30:22 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update: Will
>> get addressed separately:
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separate RFE). Update: This issue is
>> resolved by 2nd commit.
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix storing 32 bit integers into Java frames. Enable TestArrayStructs.
src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/TypeClass.java line
2:
> 1: /*
> 2: * Copyright (c) 2022, 2023 Oracle and/or its affiliates. All rights
> reserved.
The copyright header here is missing a comma after the second year:
Suggestion:
* Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle [v3]
On Fri, 10 Mar 2023 14:14:55 GMT, Jorn Vernee wrote: >> The issue is that the size of the code buffer is not large enough to hold >> the whole stub. >> >> Proposed solution is to scale the size of the stub with the number of >> arguments. I've adjusted sizes for both downcall and upcall stubs. I've also >> dropped the number of relocations, since we're not really using any for >> downcalls, and for upcalls we only have 1 AFAICS. (the size of the >> relocations can not be zero however, as that leads to the relocation section >> [not being initialized][1], and triggering [an assert][2] later when the >> code blob is copied). >> >> The way I've determined the new base size and per-argument size for stubs, >> is by first linking a stub without any arguments to get the required base >> size, and by then adding 20 `double` arguments to get a rough per-argument >> size. Both values have wiggle room as well. The sizes can be printed using >> e.g. `-XX:+LogCompilation`, and then looking for `nep_invoker_blob` and >> `upcall_stub*` in the log file. This experiment was done on a fastdebug >> build to account for additional debug code being generated. The included >> test is designed to try and maximize the size of the generated stub. >> >> I've also updated `CodeBuffer::log_section_sizes` to print the in-use size, >> rather than just the capacity and free space. >> >> [1]: >> https://github.com/openjdk/jdk/blob/56512cfe1f0682c98ba3488af3d03ccef632c016/src/hotspot/share/asm/codeBuffer.cpp#L119-L121 >> [2]: >> https://github.com/openjdk/jdk/blob/56512cfe1f0682c98ba3488af3d03ccef632c016/src/hotspot/share/asm/codeBuffer.cpp#L675 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > RISCV changes Looks good. - Marked as reviewed by vlivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/12908
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v6]
On Tue, 14 Mar 2023 16:34:59 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - long lines wrapped
>> - StripJavaDebugAttributesPlugin transformation fixed
>> - VersionPropsPlugin transformation fixed
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
> line 614:
>
>> 612: private void genConstructor(ClassBuilder clb) {
>> 613: clb.withMethod("", MethodTypeDesc.of(CD_void),
>> 614: ACC_PUBLIC, mb ->
>> mb.withFlags(ACC_PUBLIC).withCode( cob -> {
>
> Why `withFlags(ACC_PUBLIC)` when the flags is already given in `withMethod`'s
> 3rd argument? Same for occurrences below.
It has been redundant, now more effective `withMethodBody` method is used.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
> line 615:
>
>> 613: clb.withMethod("", MethodTypeDesc.of(CD_void),
>> 614: ACC_PUBLIC, mb ->
>> mb.withFlags(ACC_PUBLIC).withCode( cob -> {
>> 615: cob.loadInstruction(TypeKind.ReferenceType, 0);
>
> What's the basis for using `loadInstruction(ReferenceType, 0)` over
> `aload(0)`, etc.? I think, at least for constructors, we won't have other
> TypeKinds, so we can safely use `aload(0)` `return_()`, and this is what the
> original ASM code used.
The whole syntax used in `SystemModulesPlugin` deserved a refresh.
`loadInstruction(ReferenceType, 0)` is a generic pattern used before more
convenient `aload(0)` has been added to `CodeBuilder`.
Now it is fixed, thanks.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
> line 916:
>
>> 914: sb.append("Ljava/lang/Object;");
>> 915: }
>> 916: sb.append(")Ljava/util/Set;");
>
> The construction of the Set.of method type should move away from
> StringBuilder. It can be as:
>
> var mtdArgs = new ClassDesc[size];
> Arrays.fill(mtdArgs, CD_Object);
> MethodTypeDesc mtd = MethodTypeDesc.of(CD_Set, mtdArgs);
Applied, thanks.
-
PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v8]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed SystemModulesPlugin - Changes: - all: https://git.openjdk.org/jdk/pull/12944/files - new: https://git.openjdk.org/jdk/pull/12944/files/91bf43f6..49c80bcd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12944=07 - incr: https://webrevs.openjdk.org/?repo=jdk=12944=06-07 Stats: 372 lines in 1 file changed: 38 ins; 21 del; 313 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
On Thu, 23 Feb 2023 09:04:23 GMT, Justin Lu wrote: > This PR converts Unicode sequences to UTF-8 native in .properties file. > (Excluding the Unicode space and tab sequence). The conversion was done using > native2ascii. > > In addition, the build logic is adjusted to support reading in the > .properties files as UTF-8 during the conversion from .properties file to > .java ListResourceBundle file. test/jdk/java/util/ResourceBundle/Bug6204853.properties line 1: > 1: # This file should probably be excluded because it's used in a test that relates to UTF-8 encoding (or not) of property files. - PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
On Thu, 23 Feb 2023 09:04:23 GMT, Justin Lu wrote:
> This PR converts Unicode sequences to UTF-8 native in .properties file.
> (Excluding the Unicode space and tab sequence). The conversion was done using
> native2ascii.
>
> In addition, the build logic is adjusted to support reading in the
> .properties files as UTF-8 during the conversion from .properties file to
> .java ListResourceBundle file.
make/langtools/tools/compileproperties/CompileProperties.java line 252:
> 250: try {
> 251: writer = new BufferedWriter(
> 252: new OutputStreamWriter(new
> FileOutputStream(outputPath), StandardCharsets.ISO_8859_1));
Using ISO_8859_1 seems strange.
Since these are generated files, you could write them as UTF-8 and then
override the default javac option for ascii when compiling _just_ these files.
Or else just stay with ascii; no one should be looking at these files!
-
PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
On Tue, 7 Mar 2023 23:15:14 GMT, Jonathan Gibbons wrote:
>> This PR converts Unicode sequences to UTF-8 native in .properties file.
>> (Excluding the Unicode space and tab sequence). The conversion was done
>> using native2ascii.
>>
>> In addition, the build logic is adjusted to support reading in the
>> .properties files as UTF-8 during the conversion from .properties file to
>> .java ListResourceBundle file.
>
> make/langtools/tools/compileproperties/CompileProperties.java line 252:
>
>> 250: try {
>> 251: writer = new BufferedWriter(
>> 252: new OutputStreamWriter(new
>> FileOutputStream(outputPath), StandardCharsets.ISO_8859_1));
>
> Using ISO_8859_1 seems strange.
> Since these are generated files, you could write them as UTF-8 and then
> override the default javac option for ascii when compiling _just_ these files.
>
> Or else just stay with ascii; no one should be looking at these files!
Will stick with your latter solution, as since the .properties files were
converted via native2ascii, it makes sense to write out via ascii.
-
PR: https://git.openjdk.org/jdk/pull/12726
RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
This PR converts Unicode sequences to UTF-8 native in .properties file. (Excluding the Unicode space and tab sequence). The conversion was done using native2ascii. In addition, the build logic is adjusted to support reading in the .properties files as UTF-8 during the conversion from .properties file to .java ListResourceBundle file. - Commit messages: - Write to ASCII - Read in .properties as UTF-8, but write to LRB .java as ISO-8859-1 - Compile class with ascii (Not ready to make system wide change) - Toggle UTF-8 for javac option in JavaCompilation.gmk - CompileProperties converts in UTF-8 - Convert .properties from ISO-8859-1 to UTF-8 Changes: https://git.openjdk.org/jdk/pull/12726/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12726=00 Issue: https://bugs.openjdk.org/browse/JDK-8301991 Stats: 29093 lines in 490 files changed: 6 ins; 0 del; 29087 mod Patch: https://git.openjdk.org/jdk/pull/12726.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12726/head:pull/12726 PR: https://git.openjdk.org/jdk/pull/12726
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 14:56:28 GMT, Eirik Bjorsnos wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update StringLatin1.canEncode to sync with same test in CharacterData.of
>
> Just for fun, I tried with a benchmark where the code point is Latin1 in
> every other call:
>
>
> @Benchmark
> public void isDigitVarying(Blackhole blackhole) {
> blackhole.consume(Character.isDigit(48));
> blackhole.consume(Character.isDigit(1632));
> }
>
>
> With this benchmark, there is no difference between the baseline, the PR and
> using StringLatin1.canEncode:
>
> Baseline:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigitVarying 1632 avgt 15 1.198 ± 0.056 ns/op
>
>
> PR:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigitVarying 1632 avgt 15 1.195 ± 0.058 ns/op
>
>
> StringLatin1.canEncode:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigitVarying 1632 avgt 15 1.193 ± 0.055 ns/op
> ```
>
> At this point, I'm starting to wonder a bit if the performance benefits
> suggested by this PR might be dubious and will only surface in very narrow
> benchmarks. On the other hand, it does not seem harmful either. What do
> people think?
> @eirbjo I suggest to add `-prof perfasm` and see what's going on; I suspect
> it's better that inputs are loaded from `@State` field...and don't use
> `BlackHole` but combine the results of the 2 operations (although I remember
> that compiler assisted black holes are not as noisy as I'm used to observe
> with JDK 11 ones)
I think I'm done with benchmarking for this PR. Any StringLatin1.canEncode
method call regression can be solved outside this PR.
The PR as it stands seems to have some benefit for Latin1-only apps/use cases,
which I think is not uncommon. Any benefit for mixed cases are more dubious,
but don't seem to hurt.
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 14:57:43 GMT, Quan Anh Mai wrote: >>> The StringLatin1.canEncode regression disappears. >> >> I mixed things up so StringLatin1.canEncode was benchmarked without the >> updated code. >> >> Here are updated benchmark results: >> >> >> Baseline: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigitRandom 1632 avgt 15 5.437 ± 0.235 ns/op >> >> >> PR: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigitRandom 1632 avgt 15 5.319 ± 0.341 ns/op >> >> >> StringLatin1.canEncode: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigitRandom 1632 avgt 15 5.447 ± 0.304 ns/op >> ``` >> >> So it seems using StringLatin1.canEncode still might have a regression also >> in the randomized input case. >> >> For this PR, I suggest we update StringLatin1.canEncode to be in sync with >> CharacterData.of, without one calling the other. If anyone wants to >> investigate the regression further, than can be done outside this PR. >> >> I have independently verified that StringLatin1.canEncode sees performance >> improvements using the StringIndexOf benchmark. > > We can do `Integer.compareUnsigned(ch, 0xFF) <= 0` We could, but benchmarks show no performance improvements over the current PR. I think the current code is perhaps slightly more readable. - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 14:56:28 GMT, Eirik Bjorsnos wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update StringLatin1.canEncode to sync with same test in CharacterData.of
>
> Just for fun, I tried with a benchmark where the code point is Latin1 in
> every other call:
>
>
> @Benchmark
> public void isDigitVarying(Blackhole blackhole) {
> blackhole.consume(Character.isDigit(48));
> blackhole.consume(Character.isDigit(1632));
> }
>
>
> With this benchmark, there is no difference between the baseline, the PR and
> using StringLatin1.canEncode:
>
> Baseline:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigitVarying 1632 avgt 15 1.198 ± 0.056 ns/op
>
>
> PR:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigitVarying 1632 avgt 15 1.195 ± 0.058 ns/op
>
>
> StringLatin1.canEncode:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigitVarying 1632 avgt 15 1.193 ± 0.055 ns/op
> ```
>
> At this point, I'm starting to wonder a bit if the performance benefits
> suggested by this PR might be dubious and will only surface in very narrow
> benchmarks. On the other hand, it does not seem harmful either. What do
> people think?
@eirbjo I suggest to add `-prof perfasm` and see what's going on; I suspect
it's better that inputs are loaded from `@State` field...and don't use
`BlackHole` but combine the results of the 2 operations (although I remember
that compiler assisted black holes are not as noisy as I'm used to observe with
JDK 11 ones)
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 14:23:35 GMT, Eirik Bjorsnos wrote: >> Many thanks to have tried, yep, I was curious indeed re the >> "StringLatin1.canEncode regression" case. >> I would still modify the benchmark to use inputs (I know that will make it >> memory bound sadly, due to reading inputs - but the size of such inputs can >> be a benchmark parameter, together with the bias eg "latin","mix", >> "non-latin") "semi-randomly" generated based on the mentioned >> strategies/biases. >> It will benefit future tests on this, although could be provided as a >> separate PR. > >> The StringLatin1.canEncode regression disappears. > > I mixed things up so StringLatin1.canEncode was benchmarked without the > updated code. > > Here are updated benchmark results: > > > Baseline: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigitRandom 1632 avgt 15 5.437 ± 0.235 ns/op > > > PR: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigitRandom 1632 avgt 15 5.319 ± 0.341 ns/op > > > StringLatin1.canEncode: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigitRandom 1632 avgt 15 5.447 ± 0.304 ns/op > ``` > > So it seems using StringLatin1.canEncode still might have a regression also > in the randomized input case. > > For this PR, I suggest we update StringLatin1.canEncode to be in sync with > CharacterData.of, without one calling the other. If anyone wants to > investigate the regression further, than can be done outside this PR. > > I have independently verified that StringLatin1.canEncode sees performance > improvements using the StringIndexOf benchmark. We can do `Integer.compareUnsigned(ch, 0xFF) <= 0` - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 14:31:03 GMT, Eirik Bjorsnos wrote:
>> By avoiding a bit shift operation for the latin1 fast-path test, we can
>> speed up the `java.lang.CharacterData.of` method by ~25% for latin1 code
>> points.
>>
>> The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace
>> this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain
>> (especially for Latin1 code points):
>>
>> This method is called frequently by various property-determining methods in
>> `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should
>> expect improvements for all these methods.
>>
>> Performance is tested using the `Characters.isDigit` benchmark using the
>> digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632,
>> in CharacterData00):
>>
>> Baseline:
>>
>>
>> Benchmark (codePoint) Mode Cnt Score Error Units
>> Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op
>> Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op
>>
>> PR:
>>
>>
>> Benchmark (codePoint) Mode Cnt Score Error Units
>> Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op
>> Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op
>
> Eirik Bjorsnos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update StringLatin1.canEncode to sync with same test in CharacterData.of
Just for fun, I tried with a benchmark where the code point is Latin1 in every
other call:
@Benchmark
public void isDigitVarying(Blackhole blackhole) {
blackhole.consume(Character.isDigit(48));
blackhole.consume(Character.isDigit(1632));
}
With this benchmark, there is no difference between the baseline, the PR and
using StringLatin1.canEncode:
Baseline:
Benchmark (codePoint) Mode Cnt Score Error Units
Characters.isDigitVarying 1632 avgt 15 1.198 ± 0.056 ns/op
PR:
Benchmark (codePoint) Mode Cnt Score Error Units
Characters.isDigitVarying 1632 avgt 15 1.195 ± 0.058 ns/op
StringLatin1.canEncode:
Benchmark (codePoint) Mode Cnt Score Error Units
Characters.isDigitVarying 1632 avgt 15 1.193 ± 0.055 ns/op
```
At this point, I'm starting to wonder a bit if the performance benefits
suggested by this PR might be dubious and will only surface in very narrow
benchmarks. On the other hand, it does not seem harmful either. What do people
think?
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 14:31:03 GMT, Eirik Bjorsnos wrote: >> By avoiding a bit shift operation for the latin1 fast-path test, we can >> speed up the `java.lang.CharacterData.of` method by ~25% for latin1 code >> points. >> >> The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace >> this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain >> (especially for Latin1 code points): >> >> This method is called frequently by various property-determining methods in >> `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should >> expect improvements for all these methods. >> >> Performance is tested using the `Characters.isDigit` benchmark using the >> digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632, >> in CharacterData00): >> >> Baseline: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op >> Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op >> >> PR: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op >> Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Update StringLatin1.canEncode to sync with same test in CharacterData.of Marked as reviewed by stsypanov (Author). - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 12:00:59 GMT, Claes Redestad wrote: >> Eirik Bjorsnos has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update StringLatin1.canEncode to sync with same test in CharacterData.of > > Nice one! @cl4es Do you agree that the StringLatin1.canEncode change should be included in this PR? If so, can you approve the latest update? - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
On Wed, 15 Mar 2023 13:50:44 GMT, Francesco Nigro wrote: >> I created a randomized version of `Characters.isDigit` which tests with code >> points picked at random such that any category (Latin1, negative, different >> planes, unassiged) are equally probable. >> >> Baseline: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigitRandom 1632 avgt 15 5.503 ± 0.371 ns/op >> >> >> Current PR: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigitRandom 1632 avgt 15 5.393 ± 0.336 ns/op >> >> >> Using StringLatin1.canEncode: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigitRandom 1632 avgt 15 5.377 ± 0.322 ns/op >> >> >> Seems the PR still has a small improvement for this scenario. The >> StringLatin1.canEncode regression disappears. >> >> In the real world ASCII/Latin1 seems to dominate most data, so this scenario >> is perhaps not very realistic. >> >> I'm running this on a Mac, so cannot try `-prof perfnorm`. > > Many thanks to have tried, yep, I was curious indeed re the > "StringLatin1.canEncode regression" case. > I would still modify the benchmark to use inputs (I know that will make it > memory bound sadly, due to reading inputs - but the size of such inputs can > be a benchmark parameter, together with the bias eg "latin","mix", > "non-latin") "semi-randomly" generated based on the mentioned > strategies/biases. > It will benefit future tests on this, although could be provided as a > separate PR. > The StringLatin1.canEncode regression disappears. I mixed things up so StringLatin1.canEncode was benchmarked without the updated code. Here are updated benchmark results: Baseline: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigitRandom 1632 avgt 15 5.437 ± 0.235 ns/op PR: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigitRandom 1632 avgt 15 5.319 ± 0.341 ns/op StringLatin1.canEncode: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigitRandom 1632 avgt 15 5.447 ± 0.304 ns/op ``` So it seems using StringLatin1.canEncode still might have a regression also in the randomized input case. For this PR, I suggest we update StringLatin1.canEncode to be in sync with CharacterData.of, without one calling the other. If anyone wants to investigate the regression further, than can be done outside this PR. I have independently verified that StringLatin1.canEncode sees performance improvements using the StringIndexOf benchmark. - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test [v2]
> By avoiding a bit shift operation for the latin1 fast-path test, we can speed > up the `java.lang.CharacterData.of` method by ~25% for latin1 code points. > > The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace > this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain > (especially for Latin1 code points): > > This method is called frequently by various property-determining methods in > `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should expect > improvements for all these methods. > > Performance is tested using the `Characters.isDigit` benchmark using the > digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632, > in CharacterData00): > > Baseline: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op > Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op > > PR: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op > Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Update StringLatin1.canEncode to sync with same test in CharacterData.of - Changes: - all: https://git.openjdk.org/jdk/pull/13040/files - new: https://git.openjdk.org/jdk/pull/13040/files/0174440e..0baf690e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13040=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13040=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13040.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13040/head:pull/13040 PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v2]
On Wed, 15 Mar 2023 14:23:06 GMT, Leonid Mesnik wrote: >> The StreamPumper is fixed to process the last line even it is not finishes >> with '\n' or '\r'. The test included. Testing with tier1-3 also to verify >> that tests are not broken. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fixed test test/lib/jdk/test/lib/process/StreamPumper.java line 145: > 143: } > 144: } > 145: final String line = lineBos.toString(); The code in 'lastcrlf == -1' as well as in 'lastcrlf < len - 1' writes remaining buf into lineBos. This lineBos is used to make line which is processed in line 127. However, if the stream is emptied then the chunk after last '\n' is written in in the lineBos but we never reach line 127 to call processLine for it. - PR: https://git.openjdk.org/jdk/pull/13034
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v2]
> The StreamPumper is fixed to process the last line even it is not finishes > with '\n' or '\r'. The test included. Testing with tier1-3 also to verify > that tests are not broken. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed test - Changes: - all: https://git.openjdk.org/jdk/pull/13034/files - new: https://git.openjdk.org/jdk/pull/13034/files/2f33a8c4..7e0305d1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13034=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13034=00-01 Stats: 12 lines in 1 file changed: 2 ins; 8 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13034.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13034/head:pull/13034 PR: https://git.openjdk.org/jdk/pull/13034
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 13:42:22 GMT, Eirik Bjorsnos wrote: >> Can you check what happen adding much more inputs to the dataset including >> non-latin chars as well and use `-prof perfnorm` to check what `perf` report >> re branches/branch-misses? >> >> You can use `SplittableRandom` to pre-populate an array of inputs which >> sequence is "random" but still allow deterministic benchmarking and feed the >> benchmark method by cycling the pre-computed inputs. >> In the real world I expect `isDigit` to happen on different input types and >> both having C2 with both branches places based on prev inputs distribution >> and a confused branch-predictor to allow comparing vs something that looks a >> bit nearest to the real world (TBD, I know). >> I expect in that case that a single cmp + mask to work better depending on >> latin input distribution/occurrence > > I created a randomized version of `Characters.isDigit` which tests with code > points picked at random such that any category (Latin1, negative, different > planes, unassiged) are equally probable. > > Baseline: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigitRandom 1632 avgt 15 5.503 ± 0.371 ns/op > > > Current PR: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigitRandom 1632 avgt 15 5.393 ± 0.336 ns/op > > > Using StringLatin1.canEncode: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigitRandom 1632 avgt 15 5.377 ± 0.322 ns/op > > > Seems the PR still has a small improvement for this scenario. The > StringLatin1.canEncode regression disappears. > > In the real world ASCII/Latin1 seems to dominate most data, so this scenario > is perhaps not very realistic. > > I'm running this on a Mac, so cannot try `-prof perfnorm`. Many thanks to have tried, yep, I was curious indeed re the "StringLatin1.canEncode regression" case. I would still modify the benchmark to use inputs (I know that will make it memory bound sadly, due to reading inputs - but the size of such inputs can be a benchmark parameter, together with the bias eg "latin","mix", "non-latin") "semi-randomly" generated based on the mentioned strategies/biases. It will benefit future tests on this, although could be provided as a separate PR. - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 12:37:24 GMT, Francesco Nigro wrote: >>> It seems reasonable to keep these two in sync, yes. (`CharacterData.of` >>> could even call into `StringLatin1.canEncode`, unless that's cause for some >>> performance anomaly) >> >> If I update `StringLatin1.canEncode` and call into that from >> `CharacterData.of`, I observe no regression for the Latin1 case, but a >> significant regression for the non-Latin1 case. I have no idea how to >> explain that: >> >> >> Benchmark (codePoint) Mode Cnt Score Error Units >> Characters.isDigit 48 avgt 15 0.675 ± 0.029 ns/op >> Characters.isDigit 1632 avgt 15 2.435 ± 0.032 ns/op > > Can you check what happen adding much more inputs to the dataset including > non-latin chars as well and use `-prof perfnorm` to check what `perf` report > re branches/branch-misses? > > You can use `SplittableRandom` to pre-populate an array of inputs which > sequence is "random" but still allow deterministic benchmarking and feed the > benchmark method by cycling the pre-computed inputs. > In the real world I expect `isDigit` to happen on different input types and > both having C2 with both branches places based on prev inputs distribution > and a confused branch-predictor to allow comparing vs something that looks a > bit nearest to the real world (TBD, I know). > I expect in that case that a single cmp + mask to work better depending on > latin input distribution/occurrence I created a randomized version of `Characters.isDigit` which tests with code points picked at random such that any category (Latin1, negative, different planes, unassiged) are equally probable. Baseline: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigitRandom 1632 avgt 15 5.503 ± 0.371 ns/op Current PR: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigitRandom 1632 avgt 15 5.393 ± 0.336 ns/op Using StringLatin1.canEncode: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigitRandom 1632 avgt 15 5.377 ± 0.322 ns/op Seems the PR still has a small improvement for this scenario. The StringLatin1.canEncode regression disappears. In the real world ASCII/Latin1 seems to dominate most data, so this scenario is perhaps not very realistic. I'm running this on a Mac, so cannot try `-prof perfnorm`. - PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 12:28:05 GMT, Eirik Bjorsnos wrote:
>>> `if (ch && 0xFF00 == 0) {`
>>
>> This seems to perform similar to baseline:
>>
>>
>> Benchmark (codePoint) Mode Cnt Score Error Units
>> Characters.isDigit 48 avgt 15 0.890 ± 0.025 ns/op
>> Characters.isDigit 1632 avgt 15 2.174 ± 0.011 ns/op
>>
>>
>> Would be interesting to check the performance on non-Intel architectures. If
>> you want to give it a spin on your M1, here's the benchmark command I used:
>>
>> `make test TEST='micro:java.lang.Characters.isDigit' MICRO="OPTIONS=-p
>> codePoint=48,1632"`
>
>> It seems reasonable to keep these two in sync, yes. (`CharacterData.of`
>> could even call into `StringLatin1.canEncode`, unless that's cause for some
>> performance anomaly)
>
> If I update `StringLatin1.canEncode` and call into that from
> `CharacterData.of`, I observe no regression for the Latin1 case, but a
> significant regression for the non-Latin1 case. I have no idea how to explain
> that:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigit 48 avgt 15 0.675 ± 0.029 ns/op
> Characters.isDigit 1632 avgt 15 2.435 ± 0.032 ns/op
Can you check what happen adding much more inputs to the dataset that includes
non-latin chars as well and use `-prof perfnorm` to check what `perf` report re
branches/branch-misses?
You can use SplittableRandom to pre-populate an array of inputs which sequence
is "random" but still allow deterministic benchmarking and feed the benchmark
method cycling the pre-computed inputs
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 12:15:39 GMT, Eirik Bjorsnos wrote:
>> It seems reasonable to keep these two in sync, yes. (`CharacterData.of`
>> could even call into `StringLatin1.canEncode`, unless that's cause for some
>> performance anomaly)
>
>> `if (ch && 0xFF00 == 0) {`
>
> This seems to perform similar to baseline:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigit 48 avgt 15 0.890 ± 0.025 ns/op
> Characters.isDigit 1632 avgt 15 2.174 ± 0.011 ns/op
>
>
> Would be interesting to check the performance on non-Intel architectures. If
> you want to give it a spin on your M1, here's the benchmark command I used:
>
> `make test TEST='micro:java.lang.Characters.isDigit' MICRO="OPTIONS=-p
> codePoint=48,1632"`
> It seems reasonable to keep these two in sync, yes. (`CharacterData.of` could
> even call into `StringLatin1.canEncode`, unless that's cause for some
> performance anomaly)
If I update `StringLatin1.canEncode` and call into that from
`CharacterData.of`, I observe no regression for the Latin1 case, but a
significant regression for the non-Latin1 case. I have no idea how to explain
that:
Benchmark (codePoint) Mode Cnt Score Error Units
Characters.isDigit 48 avgt 15 0.675 ± 0.029 ns/op
Characters.isDigit 1632 avgt 15 2.435 ± 0.032 ns/op
-
PR: https://git.openjdk.org/jdk/pull/13040
Withdrawn: 8203035: Implement equals() and hashCode() for Throwable
On Sat, 10 Dec 2022 18:11:30 GMT, Victor Toni wrote: > Being able to compare instances of Throwable allows simple detection of > exceptions raised by the same circumstances. Comparison allows for reduction > of excessive logging e.g. in hotspots without requiring custom code to > compare Throwables. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/11624
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 12:13:00 GMT, Claes Redestad wrote:
>> Btw, I think we can do the same for `StringLatin1.canEncode()`
>
> It seems reasonable to keep these two in sync, yes. (`CharacterData.of` could
> even call into `StringLatin1.canEncode`, unless that's cause for some
> performance anomaly)
> `if (ch && 0xFF00 == 0) {`
This seems to perform similar to baseline:
Benchmark (codePoint) Mode Cnt Score Error Units
Characters.isDigit 48 avgt 15 0.890 ± 0.025 ns/op
Characters.isDigit 1632 avgt 15 2.174 ± 0.011 ns/op
Would be interesting to check the performance on non-Intel architectures. If
you want to give it a spin on your M1, here's the benchmark command I used:
`make test TEST='micro:java.lang.Characters.isDigit' MICRO="OPTIONS=-p
codePoint=48,1632"`
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 12:07:12 GMT, Sergey Tsypanov wrote:
>> src/java.base/share/classes/java/lang/CharacterData.java line 72:
>>
>>> 70:
>>> 71: static final CharacterData of(int ch) {
>>> 72: if (ch >= 0 && ch <= 0xFF) { // fast-path
>>
>> Maybereducing to a single branch with a mask op helps further? Or maybe the
>> JIT already effectively does that:
>>
>> `if (ch && 0xFF00 == 0) {`
>
> Btw, I think we can do the same for `StringLatin1.canEncode()`
It seems reasonable to keep these two in sync, yes. (`CharacterData.of` could
even call into `StringLatin1.canEncode`, unless that's cause for some
performance anomaly)
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v4]
On Tue, 14 Mar 2023 18:00:51 GMT, Mandy Chung wrote: >> Right, I'll fix it, thanks. > > Looks like `LineNumberTable` is not stripped even with > `Option.processLineNumbers(false)`. Yes, I forgot to transform code elements, where `Classfile.Option.processLineNumbers(false)` option is applied. Thanks for catching it. We may also consider a new form of transformation allowing to filter code attributes without inflation of all code elements (in the Classfile API). - PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 11:58:14 GMT, Claes Redestad wrote:
>> By avoiding a bit shift operation for the latin1 fast-path test, we can
>> speed up the `java.lang.CharacterData.of` method by ~25% for latin1 code
>> points.
>>
>> The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace
>> this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain
>> (especially for Latin1 code points):
>>
>> This method is called frequently by various property-determining methods in
>> `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should
>> expect improvements for all these methods.
>>
>> Performance is tested using the `Characters.isDigit` benchmark using the
>> digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632,
>> in CharacterData00):
>>
>> Baseline:
>>
>>
>> Benchmark (codePoint) Mode Cnt Score Error Units
>> Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op
>> Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op
>>
>> PR:
>>
>>
>> Benchmark (codePoint) Mode Cnt Score Error Units
>> Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op
>> Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op
>
> src/java.base/share/classes/java/lang/CharacterData.java line 72:
>
>> 70:
>> 71: static final CharacterData of(int ch) {
>> 72: if (ch >= 0 && ch <= 0xFF) { // fast-path
>
> Maybereducing to a single branch with a mask op helps further? Or maybe the
> JIT already effectively does that:
>
> `if (ch && 0xFF00 == 0) {`
Btw, I think we can do the same for `StringLatin1.canEncode()`
-
PR: https://git.openjdk.org/jdk/pull/13040
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v7]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed StripJavaDebugAttribute to drop line numbers from code - Changes: - all: https://git.openjdk.org/jdk/pull/12944/files - new: https://git.openjdk.org/jdk/pull/12944/files/5fe8c7f4..91bf43f6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12944=06 - incr: https://webrevs.openjdk.org/?repo=jdk=12944=05-06 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 11:26:21 GMT, Eirik Bjorsnos wrote:
> By avoiding a bit shift operation for the latin1 fast-path test, we can speed
> up the `java.lang.CharacterData.of` method by ~25% for latin1 code points.
>
> The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace
> this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain
> (especially for Latin1 code points):
>
> This method is called frequently by various property-determining methods in
> `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should expect
> improvements for all these methods.
>
> Performance is tested using the `Characters.isDigit` benchmark using the
> digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632,
> in CharacterData00):
>
> Baseline:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op
> Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op
>
> PR:
>
>
> Benchmark (codePoint) Mode Cnt Score Error Units
> Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op
> Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op
Nice one!
src/java.base/share/classes/java/lang/CharacterData.java line 72:
> 70:
> 71: static final CharacterData of(int ch) {
> 72: if (ch >= 0 && ch <= 0xFF) { // fast-path
Maybereducing to a single branch with a mask op helps further? Or maybe the JIT
already effectively does that:
`if (ch && 0xFF00 == 0) {`
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.org/jdk/pull/13040
RFR: JDK-8304063: tools/jpackage/share/AppLauncherEnvTest.java fails when checking LD_LIBRARY_PATH
The test fails on Alpine Linux 3.17, when checking the environment variable LD_LIBRARY_PATH; looks like the actual env variable is much longer than the test expects. It turned out that at least on Linux (probably also on other OS like AIX) the actual env variable has the expected string at it's end, but might contain more path entries ( LD_LIBRARY_PATH can be adjusted by jvm - https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjli/java_md.c ). - Commit messages: - JDK-8304063 Changes: https://git.openjdk.org/jdk/pull/13041/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13041=00 Issue: https://bugs.openjdk.org/browse/JDK-8304063 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13041.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13041/head:pull/13041 PR: https://git.openjdk.org/jdk/pull/13041
Re: RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
On Wed, 15 Mar 2023 11:26:21 GMT, Eirik Bjorsnos wrote: > By avoiding a bit shift operation for the latin1 fast-path test, we can speed > up the `java.lang.CharacterData.of` method by ~25% for latin1 code points. > > The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace > this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain > (especially for Latin1 code points): > > This method is called frequently by various property-determining methods in > `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should expect > improvements for all these methods. > > Performance is tested using the `Characters.isDigit` benchmark using the > digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632, > in CharacterData00): > > Baseline: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op > Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op > > PR: > > > Benchmark (codePoint) Mode Cnt Score Error Units > Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op > Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op Marked as reviewed by stsypanov (Author). - PR: https://git.openjdk.org/jdk/pull/13040
RFR: 8304245: Speed up CharacterData.of by avoiding bit shifting in the latin1 fast-path test
By avoiding a bit shift operation for the latin1 fast-path test, we can speed up the `java.lang.CharacterData.of` method by ~25% for latin1 code points. The latin1 test is currently implemented as `ch >>> 8 == 0`. We can replace this with `ch >= 0 && ch <= 0xFF` for a noticable performance gain (especially for ASCII chars): This methods is called frequently by various property-determining methods in `java.lang.Character` like `isLowerCase`, `isDigit` etc, so one should expect improvements for all these methods. Performance is tested using the `Characters.isDigit` benchmark using the digits '0' (decimal 48, in CharacterDataLatin1) and '\u0660' (decimal 1632, in CharacterData00): Baseline: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigit 48 avgt 15 0.870 ± 0.011 ns/op Characters.isDigit 1632 avgt 15 2.168 ± 0.017 ns/op PR: Benchmark (codePoint) Mode Cnt Score Error Units Characters.isDigit 48 avgt 15 0.654 ± 0.007 ns/op Characters.isDigit 1632 avgt 15 2.032 ± 0.019 ns/op - Commit messages: - Update copyright year - Avoid bit shifting in the CharacterDataLatin1 fast-path Changes: https://git.openjdk.org/jdk/pull/13040/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13040=00 Issue: https://bugs.openjdk.org/browse/JDK-8304245 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13040.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13040/head:pull/13040 PR: https://git.openjdk.org/jdk/pull/13040
RFR: 8304164: jdk/classfile/CorpusTest.java still fails after JDK-8303910
Massive parallel execution of parametrised CorpusTest junit tests revealed race condition in `ClassHierarchyImpl.CachedClassHierarchyResolver::getClassInfo`. The race condition may skip calculation of the ClassHierarchyInfo. In this specific case it caused SegmentScope not being identified as an interface and verify error on assignability between declared method return type and actual returned type has been emitted by the test. Proposed patch fixes the race condition in `ClassHierarchyImpl.CachedClassHierarchyResolver::getClassInfo`. Please review. Thank you, Adam - Commit messages: - 8304164: jdk/classfile/CorpusTest.java still fails after JDK-8303910 Changes: https://git.openjdk.org/jdk/pull/13037/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13037=00 Issue: https://bugs.openjdk.org/browse/JDK-8304164 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13037.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13037/head:pull/13037 PR: https://git.openjdk.org/jdk/pull/13037
Integrated: 8294962: Convert java.base/jdk.internal.module package to use the Classfile API to modify and write module-info.class
On Fri, 25 Nov 2022 14:38:55 GMT, Adam Sotona wrote: > 8294962: java.base jdk.internal.module package uses ASM to modify and write > module-info.class. > This patch converts it to use Classfile API. > > Please review. > Thanks, > Adam This pull request has now been integrated. Changeset: 714b5f03 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/714b5f036fc70d8d1d4d3ec8777fe95cffc0fe5b Stats: 323 lines in 8 files changed: 78 ins; 127 del; 118 mod 8294962: Convert java.base/jdk.internal.module package to use the Classfile API to modify and write module-info.class Reviewed-by: alanb, mchung - PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8303697: ProcessTools doesn't print last line of process output
On Wed, 15 Mar 2023 05:41:33 GMT, Leonid Mesnik wrote:
> The StreamPumper is fixed to process the last line even it is not finishes
> with '\n' or '\r'. The test included. Testing with tier1-3 also to verify
> that tests are not broken.
Not clear on this one sorry. I would have thought the:
if (lastcrlf == -1) {
was supposed to handle lines without final \n. But I really can't follow this
code.
test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 56:
> 54: test("ARG1\nARG2\n");
> 55: test("\nARG1\nARG2\n");
> 56:
> test("\nARG1\nVERYVERYLONGLINEVERYVERYLONGLINEVERYVERYLONGLINEVERYVERYLONGLINEVERYVERYLONGLINE"
> + "" +
Probably easier/clearer to use String.repeat to create as long a line as you
want.
-
PR: https://git.openjdk.org/jdk/pull/13034
RFR: 8304225: Remove javax/script/Test7.java from ProblemList
Can I please get a review of this change which removes `javax/script/Test7.java` from the ProblemList? As noted in https://bugs.openjdk.org/browse/JDK-8304225, this test no longer fails and passes just like the other tests in `javax/script` directory with: --System.out:(4/60)-- Test7 Warning: No js engine found; test vacuously passes. With this proposed change, relevant tier testing has been run to verify that the original reported issue which caused this test to be problem listed https://bugs.openjdk.org/browse/JDK-8239361 is no longer seen. - Commit messages: - 8304225: Remove javax/script/Test7.java from ProblemList Changes: https://git.openjdk.org/jdk/pull/13035/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13035=00 Issue: https://bugs.openjdk.org/browse/JDK-8304225 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13035.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13035/head:pull/13035 PR: https://git.openjdk.org/jdk/pull/13035
Re: RFR: 8303530: Add system property for custom JAXP configuration file [v3]
> Add a system property, jdk.xml.config.file, to return the path to a custom > JAXP configuration file. The current configuration file, jaxp.properties, > that the JDK supports will become the default configuration file. > > CSR: https://bugs.openjdk.org/browse/JDK-8303531 > > Tests: XML SQE and JCK tests passed. Joe Wang has updated the pull request incrementally with one additional commit since the last revision: update config file description; add a general scope and order section; move value definition for external properties to class description - Changes: - all: https://git.openjdk.org/jdk/pull/12985/files - new: https://git.openjdk.org/jdk/pull/12985/files/014d99fd..43407bdc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12985=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12985=01-02 Stats: 392 lines in 23 files changed: 112 ins; 175 del; 105 mod Patch: https://git.openjdk.org/jdk/pull/12985.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12985/head:pull/12985 PR: https://git.openjdk.org/jdk/pull/12985
