Re: RFR: 8307795: AArch64: Optimize VectorMask.truecount() on Neon [v4]
On Mon, 29 May 2023 02:20:07 GMT, Chang Peng wrote: >> In Vector API Java level, vector mask is represented as a boolean array with >> 0x00/0x01 (8 bits of each element) as values, aka in-memory format. When it >> is loaded into vector register, e.g. Neon, the in-memory format will be >> converted to in-register format with 0/-1 value for each lane (lane width >> aligned to its type) by VectorLoadMask [1] operation, and convert back to >> in-memory format by VectorStoreMask[2]. In Neon, a typical VectorStoreMask >> operation will first narrow given vector registers by xtn insn [3] into byte >> element type, and then do a vector negate to convert to 0x00/0x01 value for >> each element. >> >> For most of the vector mask operations, the input mask is in-register >> format. And a vector mask also works in-register format all through the >> compilation. But for some operations like VectorMask.trueCount()[4] which >> counts the elements of true value, the expected input mask is in-memory >> format. So a VectorStoreMask is generated to convert the mask from >> in-register format to in-memory format before those operations. >> >> However, for trueCount() these xtn instructions in VectorStoreMask can be >> saved, since the narrowing operations will not influence the number of >> active lane (value of 0x01) of its input. >> >> This patch adds an optimized rule `VectorMaskTrueCount (VectorStoreMask >> mask)` to save the unnecessary narrowing operations. >> >> For example, >> >> >> var m = VectorMask.fromArray(IntVector.SPECIES_PREFERRED, ba, 0); >> m.not().trueCount(); >> >> >> will produce following assembly on a Neon machine before this patch: >> >> >> ... >> mvn v16.16b, v16.16b // VectorMask.not() >> xtn v16.4h, v16.4s >> xtn v16.8b, v16.8h >> neg v16.8b, v16.8b // VectorStoreMask >> addvb17, v16.8b >> umovw0, v17.b[0] // VectorMask.trueCount() >> ... >> >> >> After this patch: >> >> >> ... >> mvn v16.16b, v16.16b // VectorMask.not() >> addvs17, v16.4s >> smovx0, v17.b[0] >> neg x0, x0 // Optimized VectorMask.trueCount() >> ... >> >> >> In this case, we can save two xtn insns. >> >> Performance: >> >> Benchmark Before After Unit >> testInt 723.822 ± 1.029 1182.375 ± 12.363ops/ms >> testLong 632.154 ± 0.197 1382.74 ± 2.188ops/ms >> testShort 788.665 ± 1.852 1152.38 ± 3.77 ops/ms >> >> [1]: >> https://github.com/openjdk/jdk/blob/e1e758a7b43c29840296d337bd2f0213ab0ca3c9/src/hotspot/cpu/aarch64/aarch64_vect... > > Chang Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Update aarch64_vector.ad Still looks good to me. - Marked as reviewed by eliu (Committer). PR Review: https://git.openjdk.org/jdk/pull/13974#pullrequestreview-1450026196
Withdrawn: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded
On Thu, 9 Feb 2023 15:20:23 GMT, Volker Simonis wrote: > Prior to > [JDK-8239384](https://bugs.openjdk.org/browse/JDK-8239384)/[JDK-8238358](https://bugs.openjdk.org/browse/JDK-8238358) > LambdaMetaFactory has created VM-anonymous classes which could easily be > unloaded once they were not referenced any more. Starting with JDK 15 and the > new "hidden class" based implementation, this is not the case any more, > because the hidden classes will be strongly tied to their defining class > loader. If this is the default application class loader, these hidden classes > can never be unloaded which can easily lead to Metaspace exhaustion (see the > [test case in the JBS > issue](https://bugs.openjdk.org/secure/attachment/102601/LambdaClassLeak.java)). > This is a regression compared to previous JDK versions which some of our > applications have been affected from when migrating to JDK 17. > > The reason why the newly created hidden classes are strongly linked to their > defining class loader is not clear to me. JDK-8239384 mentions it as an > "implementation detail": > >> *4. the lambda proxy class has the strong relationship with the class loader >> (that will share the VM metaspace for its defining loader - implementation >> details)* > > From my current understanding the strong link between a hidden class created > by `LambdaMetaFactory` and its defining class loader is not strictly > required. In order to prevent potential OOMs and fix the regression compared > the JDK 14 and earlier I propose to create these hidden classes without the > `STRONG` option. > > I'll be happy to add the test case as JTreg test to this PR if you think that > would be useful. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/12493
Withdrawn: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose
On Sun, 12 Mar 2023 08:31:26 GMT, Eirik Bjorsnos wrote: > Please review this PR which speeds up TestTooManyEntries and clarifies its > purpose: > > - The name 'TestTooManyEntries' does not clearly convey the purpose of the > test. What is tested is the validation that the total CEN size fits in a Java > byte array. Suggested rename: CenSizeTooLarge > - The test creates DEFLATED entries which incurs zlib costs and File Data / > Data Descriptors for no additional benefit. We can use STORED instead. > - By creating a single LocalDateTime and setting it with > `ZipEntry.setTimeLocal`, we can avoid repeated time zone calculations. > - The name of entries is generated by calling UUID.randomUUID, we could use > simple counter instead. > - The produced file is unnecessarily large. We know how large a CEN entry is, > let's take advantage of that to create a file with the minimal size. > - By adding a maximally large extra field to the CEN entries, we get away > with fewer CEN records and save memory > - The summary and comments of the test can be improved to help explain the > purpose of the test and how we reach the limit being tested. > > These speedups reduced the runtime from 4 min 17 sec to 4 seconds on my > Macbook Pro. The produced ZIP size was reduced from 5.7 GB to 2 GB. Memory > consumption is down from 8GB to something like 12MB. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/12991
Re: RFR: 8308286 Fix clang warnings in linux code [v2]
> When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. Artem Semenov has updated the pull request incrementally with one additional commit since the last revision: update - Changes: - all: https://git.openjdk.org/jdk/pull/14033/files - new: https://git.openjdk.org/jdk/pull/14033/files/86077cdf..3f343c26 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14033=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14033=00-01 Stats: 134 lines in 15 files changed: 12 ins; 122 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14033/head:pull/14033 PR: https://git.openjdk.org/jdk/pull/14033
Re: RFR: 8309042: MemorySegment::reinterpret cleanup action is not called for all overloads
On Mon, 29 May 2023 10:39:19 GMT, Maurizio Cimadamore wrote: > There's an obvious bug in `AbstractMemorySegmentImpl::reinterpret(long, > Arena, Consumer)`: this method does not pass the consumer down to the > internal implementation method (it just passes `null`). As a result, the > cleanup is not registered. Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14199#pullrequestreview-1449744149
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
> As the FFM API evolved over time, some parts of the javadoc went out of sync. > Now that most of the API is stable, it is a good time to look again at the > javadoc as a whole, and bring some more consistency. > > While most of the changes in this PR are stylistic, I'd like to call out few > things which resulted in API changes: > > * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified > requirement that its alignment parameter must be a power of two. > > * `MemoryLayout::sliceHandle` did not require the absence of dereference > paths in the provided layout path. While that is technically possible to > allow, currently the method is specified as returning a "slice" corresponding > to some "nested layout", so following pointers seems incompatible with the > spec. I have decided to disallow for now - we can always compatibly enable it > later, if required. > > * `MemorySegment::copy` - most of the overloads did not specify that > `UnsupportedOperationException` is thrown if the destination segment is > read-only. > > * In several places, an extra `@throws IllegalArgumentException` or `@throws > IllegalArgumentException` has been added to capture cases where element size > * index computation can overflow. This is true for all the element-wise > segment copy methods, for the indexed accessors in memory segment (e.g. > `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for > `SegmentAllocator::allocateArray(MemoryLayout, long)`. > > In all these cases (except for overflows), new tests have been added to cover > the additional API changes (a CSR will also be filed to cover these). > > The class with most changes is `MemoryLayout`. I realized that the javadoc > there was a bit sloppy around the definition of "layout paths". Now there are > several subsection in the class javadoc, which explain how different kinds of > paths can be used. I have introduced the notion of "open path elements" to > denote those path elements that are not fully specified, and result in > additional var handle coordinates to be added. There is also now a definition > of what it means for a layout path to be "well-formed", so that all methods > accepting a layout path can simply refer to it (this definition is tricky to > give inline in the javadoc of the various path-accepting methods, as it > depends on many factors). > > Another biggie change was in `MemorySegment` as now all dereference methods > state precisely their spatial checks requirements, as well also specifying > when the API can throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix wrong link in layout well-formedness doc - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/6703eee9..265e0d5b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14098=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14098=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
RFR: 8309042: MemorySegment::reinterpret cleanup action is not called for all overloads
There's an obvious bug in `AbstractMemorySegmentImpl::reinterpret(long, Arena, Consumer)`: this method does not pass the consumer down to the internal implementation method (it just passes `null`). As a result, the cleanup is not registered. - Commit messages: - Remove redundant import - Initial push Changes: https://git.openjdk.org/jdk/pull/14199/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14199=00 Issue: https://bugs.openjdk.org/browse/JDK-8309042 Stats: 16 lines in 2 files changed: 15 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14199.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14199/head:pull/14199 PR: https://git.openjdk.org/jdk/pull/14199
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes [v2]
On Fri, 26 May 2023 16:54:42 GMT, Roger Riggs wrote: >> Decouple the jdk.internal.util OperatingSystem and Version classes to >> simplify class loading and avoid an indirect cyclic initialization. >> >> Move the method to get the current OS version from OperatingSystem to the >> Version class and its initialization. >> Revert to using String.toUpperCase() instead of custom code to uppercase. >> >> Update the uses of OperatingSystem.version to be Version.current. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Rename jdk.internal.util.Version to OSVersion Thanks for renaming the class. The renaming LGTM. - PR Review: https://git.openjdk.org/jdk/pull/14181#pullrequestreview-1449236628
Re: RFR: 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem [v2]
> Fix of jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo returning correct > zero power of two. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/14148/files - new: https://git.openjdk.org/jdk/pull/14148/files/8e2754e2..c51b50d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14148=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14148=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14148.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14148/head:pull/14148 PR: https://git.openjdk.org/jdk/pull/14148
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]
On Tue, 2 May 2023 13:57:37 GMT, Rémi Forax wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Adding comments >> - Improving performance >> - Merge branch 'master' into JDK-8291966 >> - 8291966: SwitchBootstrap.typeSwitch could be faster > > Also there is a lot of use cases where the type switch is called only with > instances of the same class, for those scenarii, the VM should be able to > fully remove the type switch and inline the body of the corresponding case > like this is done with a virtual call. > > I don't think there is a way currently to explain to the VM that the a hash > map really acts as a cache so if the key is a constant then the value is a > constant too (this optimization is also missing when JITing ClassValue.get()). @forax Hi! Sorry for this sudden message, but this one captured my attention > and subtype checks are usually fast. And I hope this PR to be the right place to raise this. I was looking this PR to better understand how it applies to secondary supers and https://bugs.openjdk.org/browse/JDK-8180450 (that's still WIP) doesn't look like subtype checks are yet fast nor scalable (with multiple interfaces and > 1 receiver types observed) - see https://ionutbalosin.com/2023/03/jvm-performance-comparison-for-jdk-17/#typecheckscalabilitybenchmark that can be modified to use type switch too. In addition, at least for secondary types, a missed `IsInstance` (ie which type isn't implemented) can cost O(n) over the secondary supers types (see https://ionutbalosin.com/2023/03/jvm-performance-comparison-for-jdk-17/#typecheckslowpathbenchmark) that's still not ideal, due to the high bootstrapping cost of `prnz scas`. Hence, the implementation of type switch is likely to account for the existing (performance) deficiencies of the secondary supers type check or is relying on the fix https://bugs.openjdk.org/browse/JDK-8180450 that will appear at some point? Hope to have interacted in the right way with this with the JDK dev community, and thanks again for your hard work on this wonderful piece of engineering! - PR Comment: https://git.openjdk.org/jdk/pull/9779#issuecomment-1566691245
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]
On Tue, 17 Jan 2023 15:55:40 GMT, Jan Lahoda wrote:
>> The pattern matching switches are using a bootstrap method
>> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch.
>> Basically, for a switch like:
>>
>> switch (obj) {
>> case String s when s.isEmpty() -> {}
>> case String s -> {}
>> case CharSequence cs -> {}
>> ...
>> }
>>
>>
>> this method will produce a MethodHandle that will be analyze the provided
>> selector value (`obj` in the example), and will return the case index to
>> which the switch should jump. This method also accepts a (re)start index for
>> the search, which is used to handle guards. For example, if the
>> `s.isEmpty()` guard in the above sample returns false, the matching is
>> restarted on the next case.
>>
>> The current implementation is fairly slow, it basically goes through the
>> labels in a loop. The proposal here is to replace that with a MethodHandle
>> structure like this:
>>
>> obj == null ? -1
>> : switch (restartIndex) {
>> case 0 -> obj instanceof String ? 0 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 1 -> obj instanceof String ? 1 : obj instanceof
>> CharSequence ? 2 : ... ;
>> case 2 -> obj instanceof CharSequence ? 2 : ... ;
>> ...
>> default -> ;
>> }
>>
>>
>> This appear to run faster than the current implementation, using testcase
>> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these
>> are the results
>>
>> PatternsOptimizationTest.testLegacyIndyLongSwitch thrpt 25 1515989.562
>> ± 32047.918 ops/s
>> PatternsOptimizationTest.testHandleIndyLongSwitch thrpt 25 2630707.585
>> ± 37202.210 ops/s
>>
>> PatternsOptimizationTest.testLegacyIndyShortSwitch thrpt 25 6789310.900
>> ± 61921.636 ops/s
>> PatternsOptimizationTest.testHandleIndyShortSwitch thrpt 25 10771729.464
>> ± 69607.467 ops/s
>>
>>
>> The "LegacyIndy" is the current implementation, "HandleIndy" is the one
>> proposed here. The translation in javac used is the one from #9746 in all
>> cases.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains four additional commits since
> the last revision:
>
> - Adding comments
> - Improving performance
> - Merge branch 'master' into JDK-8291966
> - 8291966: SwitchBootstrap.typeSwitch could be faster
I've merged with the current master, and tried to reflect the review feedback.
I tried to run a performance test:
[PatternsOptimizationTest.java.txt](https://github.com/openjdk/jdk/files/11589470/PatternsOptimizationTest.java.txt)
with these changes (the test uses a patch javac, and an current (old) version
of `SwitchBootstraps` - javac use the old bootstrap for classes that contain
`Legacy` in their name, new bootstrap in all other cases). The results when I
ran this were:
PatternsOptimizationTest.testHandleIndyLongSwitch thrpt 25 236.273 ±
49471.586 ops/s
PatternsOptimizationTest.testLegacyIndyLongSwitch thrpt 25 1341944.936 ±
163396.221 ops/s
PatternsOptimizationTest.testHandleIndyShortSwitch thrpt 25 9746865.005 ±
115379.432 ops/s
PatternsOptimizationTest.testLegacyIndyShortSwitch thrpt 25 5535534.370 ±
1275754.361 ops/s
So, this patch still seems to improve the state.
-
PR Comment: https://git.openjdk.org/jdk/pull/9779#issuecomment-1566682417
Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v3]
> The pattern matching switches are using a bootstrap method
> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch. Basically,
> for a switch like:
>
> switch (obj) {
> case String s when s.isEmpty() -> {}
> case String s -> {}
> case CharSequence cs -> {}
> ...
> }
>
>
> this method will produce a MethodHandle that will be analyze the provided
> selector value (`obj` in the example), and will return the case index to
> which the switch should jump. This method also accepts a (re)start index for
> the search, which is used to handle guards. For example, if the `s.isEmpty()`
> guard in the above sample returns false, the matching is restarted on the
> next case.
>
> The current implementation is fairly slow, it basically goes through the
> labels in a loop. The proposal here is to replace that with a MethodHandle
> structure like this:
>
> obj == null ? -1
> : switch (restartIndex) {
> case 0 -> obj instanceof String ? 0 : obj instanceof
> CharSequence ? 2 : ... ;
> case 1 -> obj instanceof String ? 1 : obj instanceof
> CharSequence ? 2 : ... ;
> case 2 -> obj instanceof CharSequence ? 2 : ... ;
> ...
> default -> ;
> }
>
>
> This appear to run faster than the current implementation, using testcase
> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these
> are the results
>
> PatternsOptimizationTest.testLegacyIndyLongSwitch thrpt 25 1515989.562
> ± 32047.918 ops/s
> PatternsOptimizationTest.testHandleIndyLongSwitch thrpt 25 2630707.585
> ± 37202.210 ops/s
>
> PatternsOptimizationTest.testLegacyIndyShortSwitch thrpt 25 6789310.900
> ± 61921.636 ops/s
> PatternsOptimizationTest.testHandleIndyShortSwitch thrpt 25 10771729.464
> ± 69607.467 ops/s
>
>
> The "LegacyIndy" is the current implementation, "HandleIndy" is the one
> proposed here. The translation in javac used is the one from #9746 in all
> cases.
Jan Lahoda has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains six commits:
- Reflecting review feedback.
- Merge branch 'master' into JDK-8291966
- Adding comments
- Improving performance
- Merge branch 'master' into JDK-8291966
- 8291966: SwitchBootstrap.typeSwitch could be faster
-
Changes: https://git.openjdk.org/jdk/pull/9779/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=9779=02
Stats: 197 lines in 2 files changed: 110 ins; 15 del; 72 mod
Patch: https://git.openjdk.org/jdk/pull/9779.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/9779/head:pull/9779
PR: https://git.openjdk.org/jdk/pull/9779
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java
On Wed, 24 May 2023 19:18:33 GMT, Aleksey Shipilev wrote:
> UUID is very important class that is used to track identities of objects in
> large scale systems. Yet, the coverage in JDK test is disappointing: it tests
> only 100 of UUID instances per test, which is way too small to detect
> collisions due to the bad randomness for example.
>
> I have some pending work to improve UUID performance, and tests should be
> improved.
>
> The improved test still runs in less than 5 seconds on my laptop.
Overall, this test-only change look OK to me. It updates the test to use a
`Set` instead of `List` for "contains" checks, which detect random generated
UUID collisions. The other part of this change is increasing the UUID
generation count/repeatition and a new test method which generates the UUIDs in
parallel and tries to detect any unexpected collisions.
I just have a couple of minor suggestions, which I have noted inline.
test/jdk/java/util/UUID/UUIDTest.java line 40:
> 38: private static final int COUNT = 1_000_000;
> 39:
> 40: static final Random generator = new Random();
Hello Aleksey, I realize this is unrelated to the changes in this PR, but since
we are updating this test, would using `jdk.test.lib.RandomFactory.getRandom()`
from test library be a better idea here? That would then print the seed in the
logs and could help debug any collisions more easily?
test/jdk/java/util/UUID/UUIDTest.java line 316:
> 314: UUID u2 = UUID.fromString(u1.toString());
> 315: if (u1.hashCode() != u2.hashCode()) {
> 316: throw new Exception("Equal UUIDs with different hash
> codes: " + u1 + " and " + u2);
Perhaps we should even print the hash codes that weren't matching to provide
assistance when debugging?
-
PR Comment: https://git.openjdk.org/jdk/pull/14134#issuecomment-1566645445
PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1208977029
PR Review Comment: https://git.openjdk.org/jdk/pull/14134#discussion_r1208978832
