Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle [v2]
On Thu, 19 Jan 2023 23:37:17 GMT, Mandy Chung wrote: >> `VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to >> a static `Field`. It's fragile to assume that the location of a static >> field returned by `Unsafe.staticFieldBase` is a Class object.This >> changes the VarHandle implementation for static fields (i.e. >> `FieldStaticReadOnly` and `FieldStaticReadWrite` classes) to include the >> receiver type in addition to the base and offset. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > remove the base parameter which is unused Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8300693: Lower the compile threshold and reduce the iterations of warmup loop in VarHandles tests
On Thu, 19 Jan 2023 20:43:11 GMT, Mandy Chung wrote: > `java/lang/invoke/VarHandles` tests run with C1, C2 and tiered compilations > and the test cases are executed in the warm up loop with 2 iterations to > verify C1, C2 intrinsics. Default Tier4CompileThreshold is 15000. > > This PR proposes to scale the compile threshold to 0.1 such that the warm up > loop can be reduced to 2000 iterations. This will speed up the test > execution time. > > > Before: > make test-only TEST=open/test/jdk/java/lang/invoke/VarHandles 341.06s user > 14.65s system 563% cpu 1:03.14 total > > After: > make test-only TEST=open/test/jdk/java/lang/invoke/VarHandles 234.38s user > 13.08s system 535% cpu 46.218 total test/jdk/java/lang/invoke/VarHandles/VarHandleTestAccessBoolean.java line 284: > 282: > 283: // CompileThresholdScaling is set to 0.1 and ITERS to 2000 to reduce > 284: // the test execution Wouldn't this be clearer as an `@comment` in the test header prior to the `@run` statements that use the variables? - PR: https://git.openjdk.org/jdk/pull/12104
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Fri, 20 Jan 2023 01:56:16 GMT, Stuart Marks wrote: >> Okay, I see your point and to use apiNote consistently would require >> "converting" some of the existing text to apiNote too. >> >> I'm still mulling over Pattern.compile throwing OOME. An implNote is >> probably the right category for this, in which case it can start with "In >> the the JDK Reference Implementation ...". I assume the static >> Pattern.matches needs same, and also Pattern.matcher for the lazy >> compilation case. > > @AlanBateman > > I had previously discussed with @rgiulietti whether OOME or > PatternSyntaxException was more appropriate. The issue is that you might try > to compile a pattern that contains a character with N combining diacritics, > and it might work fine. But if you change that character to have N+1 > combining diacritics, it might throw OOME. There's no syntax difference, but > rather the issue is hitting an internal implementation limit. > > There's a bit of a precedent for throwing OOME in such cases. Various places > in the library try to grow arrays. If the required array size is greater than > MAX_VALUE, the library pre-emptively throws OOME without even trying to > allocate the array. > "In the the JDK Reference Implementation ..." I'm still not sure of the right style for the JDK to refer to itself. This is really the "Java SE Reference Implementation". Or perhaps it should just be "OpenJDK". Regardless, this kind of wording would stick out in a funny way, as it's not used very frequently. I'm content not having such a preamble. Having the text within `@implNote` is probably sufficient. - PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Thu, 19 Jan 2023 15:27:04 GMT, Raffaello Giulietti wrote: >> - Strengthen a computation that could overflow. >> - Specify that use of CANON_EQ could exhaust memory in the compilation phase. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8300207: Add a pre-check for the number of canonical equivalent > permutations in j.u.r.Pattern Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Tue, 17 Jan 2023 12:23:25 GMT, Alan Bateman wrote: >> The choice of a `` paragraph rather than `@apiNote` is for consistency >> with similar commentary paragraphs in the specs of `CASE_INSENSITIVE`, >> `UNICODE_CASE`, and `UNICODE_CHARACTER_CLASS`. >> >> I have no problems in using `@apiNote` instead, but then it would be better >> to apply the same for the other mentioned flags as well. > > Okay, I see your point and to use apiNote consistently would require > "converting" some of the existing text to apiNote too. > > I'm still mulling over Pattern.compile throwing OOME. An implNote is probably > the right category for this, in which case it can start with "In the the JDK > Reference Implementation ...". I assume the static Pattern.matches needs > same, and also Pattern.matcher for the lazy compilation case. @AlanBateman I had previously discussed with @rgiulietti whether OOME or PatternSyntaxException was more appropriate. The issue is that you might try to compile a pattern that contains a character with N combining diacritics, and it might work fine. But if you change that character to have N+1 combining diacritics, it might throw OOME. There's no syntax difference, but rather the issue is hitting an internal implementation limit. There's a bit of a precedent for throwing OOME in such cases. Various places in the library try to grow arrays. If the required array size is greater than MAX_VALUE, the library pre-emptively throws OOME without even trying to allocate the array. - PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8284493: Improve computeNextExponential tail performance and accuracy [v16]
On Tue, 4 Oct 2022 17:36:56 GMT, Chris Hennick wrote: >> This PR improves both the worst-case performance of `nextExponential` and >> `nextGaussian` and the distribution of output at the tails. It fixes the >> following imperfections: >> >> * Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a >> rounding error to accumulate at the tail of the distribution (probably >> starting around `2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1`); this PR >> fixes that by tracking the multiple of exponentialX0 as a long. (This >> distortion is worst when `x > 0x1.0p56` since in that case, a rounding error >> means `extra + x == extra`. >> * Reduces several equations using `Math.fma`. (This will almost certainly >> improve performance, and may or may not improve output distribution.) >> * Uses the newly-extracted `computeWinsorizedNextExponential` function to >> prevent `nextGaussian` from going into the `nextExponential` tail twice. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Add parameter to enable/disable fixed PRNG seed cc @jddarcy ... as another potential reviewer. Thanks! - PR: https://git.openjdk.org/jdk/pull/8131
Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle [v2]
On Thu, 19 Jan 2023 23:01:23 GMT, Paul Sandoz wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove the base parameter which is unused > > src/java.base/share/classes/java/lang/invoke/VarHandles.java line 187: > >> 185: // Required by instance static field handles >> 186: static Field getStaticFieldFromBaseAndOffset(Class receiverType, >> 187: Object base, > > `base` is now unused. Removed. Thanks for catching it. - PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle [v2]
On Thu, 19 Jan 2023 23:34:04 GMT, Mandy Chung wrote: >> `VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to >> a static `Field`. It's fragile to assume that the location of a static >> field returned by `Unsafe.staticFieldBase` is a Class object.This >> changes the VarHandle implementation for static fields (i.e. >> `FieldStaticReadOnly` and `FieldStaticReadWrite` classes) to include the >> receiver type in addition to the base and offset. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > remove the base parameter which is unused Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle [v2]
> `VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to > a static `Field`. It's fragile to assume that the location of a static > field returned by `Unsafe.staticFieldBase` is a Class object.This changes > the VarHandle implementation for static fields (i.e. `FieldStaticReadOnly` > and `FieldStaticReadWrite` classes) to include the receiver type in addition > to the base and offset. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: remove the base parameter which is unused - Changes: - all: https://git.openjdk.org/jdk/pull/12100/files - new: https://git.openjdk.org/jdk/pull/12100/files/d1e1f9f0..5616f777 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12100&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12100&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12100.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12100/head:pull/12100 PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8300589: Use @snippet and @linkplain in java.text.CollationKey and java.text.CompactNumberFormat
On Thu, 19 Jan 2023 23:20:52 GMT, Naoto Sato wrote:
>> This PR implements _JEP 413: Code Snippets in Java API Documentation_ for
>> [java.text.CollationKey](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CollationKey.html)
>> and
>> [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html).
>>
>> Code examples using ... blocks are replaced with the @ snippet
>> syntax where applicable.
>>
>> Additionally, for
>> [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html)
>> internal plain text links using ... are replaced with @ linkplain.
>> (External links are kept the same)
>
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 387:
>
>> 385: * @param symbols the set of symbols to be used
>> 386: * @param compactPatterns an array of
>> 387: *{@linkplain CompactNumberFormat##compact_number_patterns
>
> Can eliminate `CompactNumberFormat` here.
Ah good point, it's the same file. Will make the change, thanks!
-
PR: https://git.openjdk.org/jdk/pull/12108
Re: RFR: 8300589: Use @snippet and @linkplain in java.text.CollationKey and java.text.CompactNumberFormat
On Thu, 19 Jan 2023 22:25:51 GMT, Justin Lu wrote:
> This PR implements _JEP 413: Code Snippets in Java API Documentation_ for
> [java.text.CollationKey](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CollationKey.html)
> and
> [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html).
>
> Code examples using ... blocks are replaced with the @ snippet
> syntax where applicable.
>
> Additionally, for
> [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html)
> internal plain text links using ... are replaced with @ linkplain.
> (External links are kept the same)
Looks good. Minor suggestions follow.
src/java.base/share/classes/java/text/CompactNumberFormat.java line 387:
> 385: * @param symbols the set of symbols to be used
> 386: * @param compactPatterns an array of
> 387: *{@linkplain CompactNumberFormat##compact_number_patterns
Can eliminate `CompactNumberFormat` here.
src/java.base/share/classes/java/text/CompactNumberFormat.java line 415:
> 413: * @param symbols the set of symbols to be used
> 414: * @param compactPatterns an array of
> 415: *{@linkplain CompactNumberFormat##compact_number_patterns
Same as above.
-
PR: https://git.openjdk.org/jdk/pull/12108
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Wed, 18 Jan 2023 12:43:31 GMT, fabioromano1 wrote: >> The enanchment is useful for applications that make heavy use of BitSet >> objects as sets of integers, and therefore they need to make a lot of calls >> to cardinality() method, which actually require linear time in the number of >> words in use by the bit set. >> This optimization reduces the cost of calling cardinality() to constant >> time, as it simply returns the value of the field, and it also try to make >> as little effort as possible to update the field, when needed. >> >> Moreover, it has been implemented a new method for testing wheter a bit set >> includes another bit set (i.e., the set of true bits of the parameter is a >> subset of the true bits of the instance). > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Added author and reverse the cicle order in includes(BitSet) I'll pile on: This optimization doesn't buy much in today's world, where most machines execute `bitCount` in one cycle. It saves a trivial loop. Over very large bitsets that saves something, but most bitsets are likely to be medium to small (across all use cases). And for large bitsets, if there is a problem with cardinality testing, the user should provide for it separately. Perhaps as a class which contains a bitset, plus the extra cached information. You might also cache the index of the first and last words containing a set bit. There's no end to what one might cache: It's data, and you can derive information from it. That's a job for users, not for the library. Also, as folks have noted, adding a speedup in one place pushes slowdowns in other places. Worst of all, there are new race conditions in the "improved" data structure. I don't recommend this change. If the data structure were immutable, I might think otherwise (as String caches its hashcode), but the race conditions really spoil it for me. - PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle
On Thu, 19 Jan 2023 19:14:38 GMT, Mandy Chung wrote: > `VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to > a static `Field`. It's fragile to assume that the location of a static > field returned by `Unsafe.staticFieldBase` is a Class object.This changes > the VarHandle implementation for static fields (i.e. `FieldStaticReadOnly` > and `FieldStaticReadWrite` classes) to include the receiver type in addition > to the base and offset. src/java.base/share/classes/java/lang/invoke/VarHandles.java line 187: > 185: // Required by instance static field handles > 186: static Field getStaticFieldFromBaseAndOffset(Class receiverType, > 187: Object base, `base` is now unused. - PR: https://git.openjdk.org/jdk/pull/12100
Re: RFR: 8300589: Use @snippet and @linkplain in java.text.CollationKey and java.text.CompactNumberFormat
On Thu, 19 Jan 2023 22:25:51 GMT, Justin Lu wrote: > This PR implements _JEP 413: Code Snippets in Java API Documentation_ for > [java.text.CollationKey](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CollationKey.html) > and > [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html). > > Code examples using ... blocks are replaced with the @ snippet > syntax where applicable. > > Additionally, for > [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html) > internal plain text links using ... are replaced with @ linkplain. > (External links are kept the same) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12108
Integrated: 8300586: Refactor code examples to use @snippet in java.text.Collator
On Wed, 18 Jan 2023 19:36:51 GMT, Justin Lu wrote: > This PR implements _JEP 413: Code Snippets in Java API Documentation_ for > [java.text.Collator](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/Collator.html). > > Code examples using ... blocks are replaced with the @ snippet > syntax where applicable. > > Additionally, the code examples have been slightly tweaked to increase > readability. This pull request has now been integrated. Changeset: 9b97699b Author:Justin Lu Committer: Naoto Sato URL: https://git.openjdk.org/jdk/commit/9b97699be50966672d382a6f288a543ab42bdfd0 Stats: 10 lines in 1 file changed: 1 ins; 0 del; 9 mod 8300586: Refactor code examples to use @snippet in java.text.Collator Reviewed-by: iris, lancea, naoto - PR: https://git.openjdk.org/jdk/pull/12080
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Wed, 18 Jan 2023 12:43:31 GMT, fabioromano1 wrote: >> The enanchment is useful for applications that make heavy use of BitSet >> objects as sets of integers, and therefore they need to make a lot of calls >> to cardinality() method, which actually require linear time in the number of >> words in use by the bit set. >> This optimization reduces the cost of calling cardinality() to constant >> time, as it simply returns the value of the field, and it also try to make >> as little effort as possible to update the field, when needed. >> >> Moreover, it has been implemented a new method for testing wheter a bit set >> includes another bit set (i.e., the set of true bits of the parameter is a >> subset of the true bits of the instance). > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Added author and reverse the cicle order in includes(BitSet) On Thu, Jan 19, 2023 at 1:15 PM fabioromano1 ***@***.***> wrote: > The patch has passed all existing tests of the class > Existing tests are very unlikely to be good enough when introducing a new invariant that all mutating methods need to maintain. As an example of what to look out for, you want to eliminate the possibility of an exception being thrown between the update of the long[] and the update of the cardinality field, and testing for that sort of thing is rarely done well. > Message ID: ***@***.***> > - PR: https://git.openjdk.org/jdk/pull/11837
Integrated: 8300356: Refactor code examples to use @snippet in java.text.CollationElementIterator
On Tue, 17 Jan 2023 23:14:53 GMT, Justin Lu wrote: > This PR implements _JEP 413: Code Snippets in Java API Documentation_ for > [java.text.CollationElementIterator](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CollationElementIterator.html). > > Code examples using ... blocks are replaced with the @ snippet > syntax where applicable. > > > Additionally, removed a floating colon in the code example This pull request has now been integrated. Changeset: fbbb27e7 Author:Justin Lu Committer: Naoto Sato URL: https://git.openjdk.org/jdk/commit/fbbb27e77085c346a251e75527af8b21e76f7fc5 Stats: 12 lines in 1 file changed: 0 ins; 2 del; 10 mod 8300356: Refactor code examples to use @snippet in java.text.CollationElementIterator Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/12055
RFR: 8300589: Use @snippet and @linkplain in java.text.CollationKey and java.text.CompactNumberFormat
This PR implements _JEP 413: Code Snippets in Java API Documentation_ for [java.text.CollationKey](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CollationKey.html) and [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html). Code examples using ... blocks are replaced with the @ snippet syntax where applicable. Additionally, for [java.text.CompactNumberFormat](https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/text/CompactNumberFormat.html) internal plain text links using ... are replaced with @ linkplain. (External links are kept the same) - Commit messages: - Use linkplain for plain text internal links - Swap to snippet Changes: https://git.openjdk.org/jdk/pull/12108/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12108&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300589 Stats: 15 lines in 2 files changed: 0 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/12108.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12108/head:pull/12108 PR: https://git.openjdk.org/jdk/pull/12108
Integrated: JDK-8300698: Missing @since tag for ClassFileFormatVersion.RELEASE_21
On Thu, 19 Jan 2023 21:52:16 GMT, Joe Darcy wrote: > The addition of the new enum constant for ClassFileFormatVersion.RELEASE_21 > neglected to include an @since tag; this should be corrected. This pull request has now been integrated. Changeset: f2a1eb98 Author:Joe Darcy URL: https://git.openjdk.org/jdk/commit/f2a1eb980437b43cde222755dbf427d7916cf9e2 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8300698: Missing @since tag for ClassFileFormatVersion.RELEASE_21 Reviewed-by: rriggs, mchung - PR: https://git.openjdk.org/jdk/pull/12107
Re: RFR: JDK-8300698: Missing @since tag for ClassFileFormatVersion.RELEASE_21
On Thu, 19 Jan 2023 21:52:16 GMT, Joe Darcy wrote: > The addition of the new enum constant for ClassFileFormatVersion.RELEASE_21 > neglected to include an @since tag; this should be corrected. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12107
Re: RFR: JDK-8300698: Missing @since tag for ClassFileFormatVersion.RELEASE_21
On Thu, 19 Jan 2023 21:52:16 GMT, Joe Darcy wrote: > The addition of the new enum constant for ClassFileFormatVersion.RELEASE_21 > neglected to include an @since tag; this should be corrected. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12107
RFR: JDK-8300698: Missing @since tag for ClassFileFormatVersion.RELEASE_21
The addition of the new enum constant for ClassFileFormatVersion.RELEASE_21 neglected to include an @since tag; this should be corrected. - Commit messages: - JDK-8300698: Missing @since tag for ClassFileFormatVersion.RELEASE_21 Changes: https://git.openjdk.org/jdk/pull/12107/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12107&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300698 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12107.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12107/head:pull/12107 PR: https://git.openjdk.org/jdk/pull/12107
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Wed, 18 Jan 2023 12:43:31 GMT, fabioromano1 wrote:
>> The enanchment is useful for applications that make heavy use of BitSet
>> objects as sets of integers, and therefore they need to make a lot of calls
>> to cardinality() method, which actually require linear time in the number of
>> words in use by the bit set.
>> This optimization reduces the cost of calling cardinality() to constant
>> time, as it simply returns the value of the field, and it also try to make
>> as little effort as possible to update the field, when needed.
>>
>> Moreover, it has been implemented a new method for testing wheter a bit set
>> includes another bit set (i.e., the set of true bits of the parameter is a
>> subset of the true bits of the instance).
>
> fabioromano1 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added author and reverse the cicle order in includes(BitSet)
> On Thu, Jan 19, 2023 at 4:29 AM fabioromano1 ***@***.***> wrote: Like other
> reviewers, changing the performance tradeoffs in BitSet make me
> uncomfortable. 30 years of code has adapted to the current performance
> tradeoffs. Those users who really need O(1) cardinality() can fairly easily
> implement it in client code (and probably have). The spirit of BitSet is
> space efficiency, and adding another field negates that. cardinality() is
> O(N) but in practice should be very efficient due to linear traversal and
> (presumably) the use of hardware bitcount instructions for each word (fix
> that if not true). There may be a case for a BitSet rewrite (e.g. to support
> large sparse bitsets) once we have compact value types. Leaving to the client
> the responsibility of implementation isn't against the principle of
> information hiding and encapsulation?
> The client already has a pretty good implementation of cardinality(). One can
> see the effort the JVM engineers put into optimizing bitCounting arrays of
> longs. The proposed change is an example of something that happens a lot -
> something helps a particular use case while extracting a small but perpetual
> tax on all other users of the JDK, (here including future maintainers of
> BitSet). Adding a redundant field to BitSet increases the risk of not keeping
> the new field in sync. Are we sure we've tested that every mutating method
> correctly updates the new field? What about new methods added in future years?
> […](#)
> --- Just to show that we care about BitSet performance, here's a (start of a)
> change that I **would** make:
> --- a/src/java.base/share/classes/java/util/BitSet.java +++
> b/src/java.base/share/classes/java/util/BitSet.java @@ -897,6 +897,8 @@
> public class BitSet implements Cloneable, java.io.Serializable { * @SInCE 1.4
> */ public int cardinality() { + final int wordsInUse = this.wordsInUse; +
> final long[] words = this.words; int sum = 0; for (int i = 0; i < wordsInUse;
> i++) sum += Long.bitCount(words[i]); I bet that provides a measurable
> performance improvement, at least with some current VMs. But probably warmed
> up C2 doesn't need this help.
The patch has passed all existing tests of the class. Anyway, I decided to do
instead a subclass that implements this enhancement, in order to avoid creating
the problems shown here.
-
PR: https://git.openjdk.org/jdk/pull/11837
RFR: 8300693: Lower the compile threshold and reduce the iterations of warmup loop in VarHandles tests
`java/lang/invoke/VarHandles` tests run with C1, C2 and tiered compilations and the test cases are executed in the warm up loop with 2 iterations to verify C1, C2 intrinsics. Default Tier4CompileThreshold is 15000. This PR proposes to scale the compile threshold to 0.1 such that the warm up loop can be reduced to 2000 iterations. This will speed up the test execution time. Before: make test-only TEST=open/test/jdk/java/lang/invoke/VarHandles 341.06s user 14.65s system 563% cpu 1:03.14 total After: make test-only TEST=open/test/jdk/java/lang/invoke/VarHandles 234.38s user 13.08s system 535% cpu 46.218 total - Commit messages: - 8300693: Lower the compile threshold and reduce the iterations of warmup loop in VarHandles tests Changes: https://git.openjdk.org/jdk/pull/12104/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12104&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300693 Stats: 182 lines in 27 files changed: 54 ins; 30 del; 98 mod Patch: https://git.openjdk.org/jdk/pull/12104.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12104/head:pull/12104 PR: https://git.openjdk.org/jdk/pull/12104
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Wed, 18 Jan 2023 12:43:31 GMT, fabioromano1 wrote:
>> The enanchment is useful for applications that make heavy use of BitSet
>> objects as sets of integers, and therefore they need to make a lot of calls
>> to cardinality() method, which actually require linear time in the number of
>> words in use by the bit set.
>> This optimization reduces the cost of calling cardinality() to constant
>> time, as it simply returns the value of the field, and it also try to make
>> as little effort as possible to update the field, when needed.
>>
>> Moreover, it has been implemented a new method for testing wheter a bit set
>> includes another bit set (i.e., the set of true bits of the parameter is a
>> subset of the true bits of the instance).
>
> fabioromano1 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added author and reverse the cicle order in includes(BitSet)
On Thu, Jan 19, 2023 at 4:29 AM fabioromano1 ***@***.***>
wrote:
> Like other reviewers, changing the performance tradeoffs in BitSet make me
> uncomfortable.
>
> 30 years of code has adapted to the current performance tradeoffs. Those
> users who really need O(1) cardinality() can fairly easily implement it in
> client code (and probably have).
>
> The spirit of BitSet is space efficiency, and adding another field negates
> that.
>
> cardinality() is O(N) but in practice should be very efficient due to
> linear traversal and (presumably) the use of hardware bitcount instructions
> for each word (fix that if not true).
>
> There may be a case for a BitSet rewrite (e.g. to support large sparse
> bitsets) once we have compact value types.
>
> Leaving to the client the responsibility of implementation isn't against
> the principle of information hiding and encapsulation?
>
The client already has a pretty good implementation of cardinality().
One can see the effort the JVM engineers put into optimizing bitCounting
arrays of longs.
The proposed change is an example of something that happens a lot -
something helps a particular use case while extracting a small but
perpetual tax on all other users of the JDK, (here including future
maintainers of BitSet).
Adding a redundant field to BitSet increases the risk of not keeping the
new field in sync. Are we sure we've tested that every mutating method
correctly updates the new field? What about new methods added in future
years?
---
Just to show that we care about BitSet performance, here's a (start of a)
change that I **would** make:
--- a/src/java.base/share/classes/java/util/BitSet.java
+++ b/src/java.base/share/classes/java/util/BitSet.java
@@ -897,6 +897,8 @@ public class BitSet implements Cloneable,
java.io.Serializable {
* @since 1.4
*/
public int cardinality() {
+final int wordsInUse = this.wordsInUse;
+final long[] words = this.words;
int sum = 0;
for (int i = 0; i < wordsInUse; i++)
sum += Long.bitCount(words[i]);
I bet that provides a measurable performance improvement, at least with
some current VMs.
But probably warmed up C2 doesn't need this help.
-
PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: 8284493: Improve computeNextExponential tail performance and accuracy [v16]
On Tue, 4 Oct 2022 17:36:56 GMT, Chris Hennick wrote: >> This PR improves both the worst-case performance of `nextExponential` and >> `nextGaussian` and the distribution of output at the tails. It fixes the >> following imperfections: >> >> * Repeatedly adding DoubleZigguratTables.exponentialX0 to extra causes a >> rounding error to accumulate at the tail of the distribution (probably >> starting around `2*exponentialX0 == 0x1.e46eff20739afp3 ~ 15.1`); this PR >> fixes that by tracking the multiple of exponentialX0 as a long. (This >> distortion is worst when `x > 0x1.0p56` since in that case, a rounding error >> means `extra + x == extra`. >> * Reduces several equations using `Math.fma`. (This will almost certainly >> improve performance, and may or may not improve output distribution.) >> * Uses the newly-extracted `computeWinsorizedNextExponential` function to >> prevent `nextGaussian` from going into the `nextExponential` tail twice. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Add parameter to enable/disable fixed PRNG seed @JimLaskey May I please have a review on this? - PR: https://git.openjdk.org/jdk/pull/8131
Re: RFR: 8300236: Use VarHandle access in Data(Input | Output)Stream classes
On Wed, 18 Jan 2023 16:34:57 GMT, Per Minborg wrote: > This PR proposes using a performance optimization using a new supported API > for operations similar to those found in `java.io.Bits` Performance looks promising for serialization (values in us/operation): | Java 20 | Java 21 | Improvement -- | -- | -- | -- SerializeBenchmark.serializeData | 7.283 | 6.793 | 6.7% SerializeBenchmark.serializeRecord | 7.275 | 6.733 | 7.5%  - PR: https://git.openjdk.org/jdk/pull/12076
Re: RFR: 8300236: Use VarHandle access in Data(Input | Output)Stream classes
On Wed, 18 Jan 2023 16:34:57 GMT, Per Minborg wrote:
> This PR proposes using a performance optimization using a new supported API
> for operations similar to those found in `java.io.Bits`
Some comments:
src/java.base/share/classes/jdk/internal/util/Bits.java line 43:
> 41: }
> 42:
> 43: public static final class BigEndian {
Are there other names that improve the readability of the uses?
For example `ByteArray.getShort()` and `ByteArray.getDouble`.
There's been a move to drop `get` and `put` prefixes where they are unnecessary.
Get and put prefixes do make them similar to the existing methods, but...
Is it just a readable to say:
`float degrees = ByteArray.float(buf);` // for get
`ByteArray.int(buf, degrees);` // for put
Maybe, maybe not
src/java.base/share/classes/jdk/internal/util/Bits.java line 161:
> 159: }
> 160:
> 161: public static final class BigEndianAtZero {
I'd merge these methods (with zero offset) in with the previous class; the
overloads with offsets are sufficient to distinguish them and it would make
discovery more natural. (omitting this class).
src/java.base/share/classes/jdk/internal/util/Bits.java line 284:
> 282:
> 283: // Alternative with an internal buffer fixed att offset zero.
> 284: public static final class BigEndianAtZeroBuffer {
I'd keep it simple an leave the buffer management to the caller. (Omitting this
class).
-
PR: https://git.openjdk.org/jdk/pull/12076
RFR: 8300236: Use VarHandle access in Data(Input | Output)Stream classes
This PR proposes using a performance optimization using a new supported API for operations similar to those found in `java.io.Bits` - Commit messages: - Remove white spaces - Remove white space - Fix jcheck issues - Update copyright years - Reformat javadoc - Prepare for review - Fix typo - Rename methods and parameters - Recompose classes and methods - Rework BigEndianAtZeroBuffer - ... and 2 more: https://git.openjdk.org/jdk/compare/4cd166ff...ce1d5765 Changes: https://git.openjdk.org/jdk/pull/12076/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12076&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300236 Stats: 1939 lines in 11 files changed: 1281 ins; 560 del; 98 mod Patch: https://git.openjdk.org/jdk/pull/12076.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12076/head:pull/12076 PR: https://git.openjdk.org/jdk/pull/12076
RFR: 8297757: VarHandles.getStaticFieldFromBaseAndOffset should get the receiver type from VarHandle
`VarHandles.getStaticFieldFromBaseAndOffset` maps a base/offset/fieldType to a static `Field`. It's fragile to assume that the location of a static field returned by `Unsafe.staticFieldBase` is a Class object.This changes the VarHandle implementation for static fields (i.e. `FieldStaticReadOnly` and `FieldStaticReadWrite` classes) to include the receiver type in addition to the base and offset. - Commit messages: - 8297757: Re-examine assumption in VarHandles.getStaticFieldFromBaseAndOffset that base is a Class object Changes: https://git.openjdk.org/jdk/pull/12100/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12100&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297757 Stats: 36 lines in 2 files changed: 3 ins; 2 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/12100.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12100/head:pull/12100 PR: https://git.openjdk.org/jdk/pull/12100
ZIP entry copy without recompression
Hi, A common use case for java.util.zip in build tools involves copying entries from a ZipFile or ZipInputStream to a ZipOutputStream without actually modifying the data. Example use cases include minification (make a JAR with only the reachable classes) and merging (combine several JAR files into one uberjar). Inflating an entry just to immediately deflate it again with no modifications seems wasteful. The following draft PR introduces ZipFileInflaterInputStream.transferTo which copies compressed ZipFile data directly to ZipOutputStream's raw data stream: https://github.com/openjdk/jdk/pull/12099 I'm typically seeing a 15 X improvement when copying xalan.jar to a ZipOutputStream backed by a buffered FileOutputStream, or 22 X when backed by OutputStream.nullOutputStream(). The PR current stays completely on the happy path and is mostly there to experiment and to show the potential performance benefits. There is currently not much focus on validation or correctness. Copying files to a different path is not supported, neither is copying from a ZipInputStream. I initially considered creating new methods for raw copies, but opted to minimize changes to public APIs, that's why I'm overriding transferTo. The PR is not intended for regular review, but as a starting point for a discussion about the usefulness of the idea and the general solution space. If we can reach consensus on such a discussion, I'll probably be happy to work on a more complete PR. Cheers, Eirik.
Integrated: 8300647: Miscellaneous hashCode improvements in java.base
On Thu, 19 Jan 2023 11:45:12 GMT, Claes Redestad wrote: > Went through the jdk and found a few more places where > `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc > methods could be replaced with a plain call to `java.util.Arrays` > equivalents. This patch addresses that. > > After this, #12068, and #12077 I think we're reaching the limit of sensible > direct use of `AS::vectorizedHashCode`. I've found a few places outside of > java.base where `vectorizedHashCode` might be applicable, but none that seem > important enough to warrant an export of this method outside of java.base. This pull request has now been integrated. Changeset: d85243f0 Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/d85243f02b34d03bd7af63a5bcbc73f500f720df Stats: 37 lines in 2 files changed: 1 ins; 31 del; 5 mod 8300647: Miscellaneous hashCode improvements in java.base Reviewed-by: stsypanov, rriggs - PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
On Thu, 19 Jan 2023 13:46:26 GMT, Claes Redestad wrote: >> Went through the jdk and found a few more places where >> `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc >> methods could be replaced with a plain call to `java.util.Arrays` >> equivalents. This patch addresses that. >> >> After this, #12068, and #12077 I think we're reaching the limit of sensible >> direct use of `AS::vectorizedHashCode`. I've found a few places outside of >> java.base where `vectorizedHashCode` might be applicable, but none that seem >> important enough to warrant an export of this method outside of java.base. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert ZipFileSystem changes since it's additionally built as a library > dependency and can't rely on jdk.internal Thanks for reviewing! - PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Tue, 17 Jan 2023 14:55:18 GMT, Raffaello Giulietti wrote: >> Okay, I see your point and to use apiNote consistently would require >> "converting" some of the existing text to apiNote too. >> >> I'm still mulling over Pattern.compile throwing OOME. An implNote is >> probably the right category for this, in which case it can start with "In >> the the JDK Reference Implementation ...". I assume the static >> Pattern.matches needs same, and also Pattern.matcher for the lazy >> compilation case. > > There's no hard limit for the number of combining marks in the Unicode > specification, even though in practice it never reaches the implementation > limit. A high number of combining marks is thus more akin to a a resource > exhaustion condition than to anything else, IMO. > > Even today, compilation of a pattern risks throwing an OOME anyway when > trying to generate the permutations. Pre-emptively throwing an OOME just > anticipates and extrapolates this behavior beyond the `int` limit of array > lengths. > > Alternatively, compilation (greedy or lazy) could throw > `PatternSyntaxException`, although there is not really something wrong with > syntax. > > I'll add `@implNote` to the other methods you mention. The CSR will be updated once this PR stabilizes. - PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
On Thu, 19 Jan 2023 13:46:26 GMT, Claes Redestad wrote: >> Went through the jdk and found a few more places where >> `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc >> methods could be replaced with a plain call to `java.util.Arrays` >> equivalents. This patch addresses that. >> >> After this, #12068, and #12077 I think we're reaching the limit of sensible >> direct use of `AS::vectorizedHashCode`. I've found a few places outside of >> java.base where `vectorizedHashCode` might be applicable, but none that seem >> important enough to warrant an export of this method outside of java.base. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert ZipFileSystem changes since it's additionally built as a library > dependency and can't rely on jdk.internal Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
> - Strengthen a computation that could overflow. > - Specify that use of CANON_EQ could exhaust memory in the compilation phase. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern - Changes: - all: https://git.openjdk.org/jdk/pull/12027/files - new: https://git.openjdk.org/jdk/pull/12027/files/8ae8f86e..64c2dfc3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12027&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12027&range=00-01 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12027.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12027/head:pull/12027 PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
On Thu, 19 Jan 2023 13:46:26 GMT, Claes Redestad wrote: >> Went through the jdk and found a few more places where >> `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc >> methods could be replaced with a plain call to `java.util.Arrays` >> equivalents. This patch addresses that. >> >> After this, #12068, and #12077 I think we're reaching the limit of sensible >> direct use of `AS::vectorizedHashCode`. I've found a few places outside of >> java.base where `vectorizedHashCode` might be applicable, but none that seem >> important enough to warrant an export of this method outside of java.base. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert ZipFileSystem changes since it's additionally built as a library > dependency and can't rely on jdk.internal Marked as reviewed by stsypanov (Author). - PR: https://git.openjdk.org/jdk/pull/12093
Withdrawn: 8293630: Simplify TreeMap.get() with Comparator.naturalOrder()
On Wed, 17 Aug 2022 11:23:57 GMT, Sergey Tsypanov wrote: > We can use `Comparator.naturalOrder()` for cases when a `TreeMap` instance is > constructed without comparator. This allows to squash two branches in > `TreeMap.get()` into one. > > P.S. I think the comment of `TreeMap.getEntryUsingComparator()` is outdated. > Should we also change it? This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/9901
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Thu, 19 Jan 2023 14:03:38 GMT, fabioromano1 wrote: > Libraries cannot be all things to all users. A library provides a service > that would be difficult for a majority of users to implement on their own. > Sometimes a library needs specialization for certain use cases. That is why > we use subclassing and wrappers to extend existing libraries. > > I think you will find that most BitSet users (including the JDK itself) want > basic bit operations with a minimal overhead. If there was additional > overhead, many would then roll their own versions to avoid that overhead. We > have conflicting goals. > > BitSet has served all JDK users well since JDK 1.0. I think your concerns are > valid, but better cardinality would be best implemented as a subclass > (CardinalBitSet?) of BitSet. Once constructed, you could then propose the > addition to java.util with a CSR. > > If you want to make a case for `includes(BitSet)` that should be proposed as > a separate CSR. I think it will be better to call it "NaturalsBitSet" to highlight the fact that the class represents a set of non-negative integers, am I wrong? - PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
On Thu, 19 Jan 2023 13:46:26 GMT, Claes Redestad wrote: >> Went through the jdk and found a few more places where >> `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc >> methods could be replaced with a plain call to `java.util.Arrays` >> equivalents. This patch addresses that. >> >> After this, #12068, and #12077 I think we're reaching the limit of sensible >> direct use of `AS::vectorizedHashCode`. I've found a few places outside of >> java.base where `vectorizedHashCode` might be applicable, but none that seem >> important enough to warrant an export of this method outside of java.base. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert ZipFileSystem changes since it's additionally built as a library > dependency and can't rely on jdk.internal src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 415: > 413: } > 414: > 415: // Replace with general purpose method someday @cl4es Nice work addressing this 16+ year old comment! 👍 - PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Thu, 19 Jan 2023 13:24:03 GMT, Jim Laskey wrote: > Libraries cannot be all things to all users. A library provides a service > that would be difficult for a majority of users to implement on their own. > Sometimes a library needs specialization for certain use cases. That is why > we use subclassing and wrappers to extend existing libraries. > > I think you will find that most BitSet users (including the JDK itself) want > basic bit operations with a minimal overhead. If there was additional > overhead, many would then roll their own versions to avoid that overhead. We > have conflicting goals. > > BitSet has served all JDK users well since JDK 1.0. I think your concerns are > valid, but better cardinality would be best implemented as a subclass > (CardinalBitSet?) of BitSet. Once constructed, you could then propose the > addition to java.util with a CSR. > > If you want to make a case for `includes(BitSet)` that should be proposed as > a separate CSR. Ok, thanks a lot for the feedback. I will make a subclass instead. Have a nice day. - PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
> Went through the jdk and found a few more places where > `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc > methods could be replaced with a plain call to `java.util.Arrays` > equivalents. This patch addresses that. > > After this, #12068, and #12077 I think we're reaching the limit of sensible > direct use of `AS::vectorizedHashCode`. I've found a few places outside of > java.base where `vectorizedHashCode` might be applicable, but none that seem > important enough to warrant an export of this method outside of java.base. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Revert ZipFileSystem changes since it's additionally built as a library dependency and can't rely on jdk.internal - Changes: - all: https://git.openjdk.org/jdk/pull/12093/files - new: https://git.openjdk.org/jdk/pull/12093/files/5f36ebd7..b1507e69 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12093&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12093&range=00-01 Stats: 7 lines in 1 file changed: 2 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12093.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12093/head:pull/12093 PR: https://git.openjdk.org/jdk/pull/12093
Integrated: 8300119: CgroupMetrics.getTotalMemorySize0() can report invalid results on 32 bit systems
On Wed, 18 Jan 2023 13:20:06 GMT, Jonathan Dowland wrote: > This is a fix for https://bugs.openjdk.org/browse/JDK-8300119 > (CgroupMetrics.getTotalMemorySize0() can report invalid results on 32 bit > systems). Thanks to @jerboaa Severin Gehwolf for figuring out the solution. > > The problem is demonstrated by > test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java which fails on > 32-bit x86 in HEAD, and passes after applying this patch. > > I tested this on an amd64 system with a cross-compiled JDK. I needed to > override the default docker container for the test, to get one with a 32-bit > userland. (the precise choice was also dictated by ABI matching my main > development machine): > > > $JT_HOME/bin/jtreg -v -Djdk.test.docker.retain.image=true > -Djdk.test.docker.image.name=i386/debian > -Djdk.test.docker.image.version=testing-slim -jdk:$JAVA_HOME > test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java This pull request has now been integrated. Changeset: dea58efb Author:Jonathan Dowland Committer: Severin Gehwolf URL: https://git.openjdk.org/jdk/commit/dea58efb6280bb1d94daf208ac909aa013439397 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8300119: CgroupMetrics.getTotalMemorySize0() can report invalid results on 32 bit systems Reviewed-by: sgehwolf - PR: https://git.openjdk.org/jdk/pull/12069
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Wed, 18 Jan 2023 12:43:31 GMT, fabioromano1 wrote: >> The enanchment is useful for applications that make heavy use of BitSet >> objects as sets of integers, and therefore they need to make a lot of calls >> to cardinality() method, which actually require linear time in the number of >> words in use by the bit set. >> This optimization reduces the cost of calling cardinality() to constant >> time, as it simply returns the value of the field, and it also try to make >> as little effort as possible to update the field, when needed. >> >> Moreover, it has been implemented a new method for testing wheter a bit set >> includes another bit set (i.e., the set of true bits of the parameter is a >> subset of the true bits of the instance). > > fabioromano1 has updated the pull request incrementally with one additional > commit since the last revision: > > Added author and reverse the cicle order in includes(BitSet) Libraries cannot be all things to all users. A library provides a service that would be difficult for a majority of users to implement on their own. Sometimes a library needs specialization for certain use cases. That is why we use subclassing and wrappers to extend existing libraries. I think you will find that most BitSet users (including the JDK itself) want basic bit operations with a minimal overhead. If there was additional overhead, many would then roll their own versions to avoid that overhead. We have conflicting goals. BitSet has served all JDK users well since JDK 1.0. I think your concerns are valid, but better cardinality would be best implemented as a subclass (CardinalBitSet?) of BitSet. Once constructed, you could then propose the addition to java.util with a CSR. If you want to make a case for `includes(BitSet)` that should be proposed as a separate CSR. - PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: 8300487: Store cardinality as a field in BitSet [v5]
On Wed, 18 Jan 2023 21:51:02 GMT, Martin Buchholz wrote: > Like other reviewers, changing the performance tradeoffs in BitSet make me > uncomfortable. > > 30 years of code has adapted to the current performance tradeoffs. Those > users who really need O(1) cardinality() can fairly easily implement it in > client code (and probably have). > > The spirit of BitSet is space efficiency, and adding another field negates > that. > > cardinality() is O(N) but in practice should be very efficient due to linear > traversal and (presumably) the use of hardware bitcount instructions for each > word (fix that if not true). > > There may be a case for a BitSet rewrite (e.g. to support large sparse > bitsets) once we have compact value types. Leaving to the client the responsibility of implementation isn't against the principle of information hiding and encapsulation? - PR: https://git.openjdk.org/jdk/pull/11837
RFR: 8300647: Miscellaneous hashCode improvements in java.base
Went through the jdk and found a few more places where `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc methods could be replaced with a plain call to `java.util.Arrays` equivalents. This patch addresses that. After this, #12068, and #12077 I think we're reaching the limit of sensible direct use of `AS::vectorizedHashCode`. I've found a few places outside of java.base where `vectorizedHashCode` might be applicable, but none that seem important enough to warrant an export of this method outside of java.base. - Commit messages: - Miscellaneous hashCode improvements in java.base Changes: https://git.openjdk.org/jdk/pull/12093/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12093&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300647 Stats: 44 lines in 3 files changed: 3 ins; 33 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/12093.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12093/head:pull/12093 PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: 8300493: Use ArraysSupport.vectorizedHashCode in j.u.zip.ZipCoder
On Wed, 18 Jan 2023 16:53:04 GMT, Claes Redestad wrote: > `ZipCoder::checkedHashCode` emulates `StringLatin1::hashCode` but operates on > a `byte[]` subrange. It can profitably use the recently introduced > `ArraysSupport::vectorizedHashCode` method to see a speed-up, which > translates to a small but significant speed-up on `ZipFile` creation. > > Before: > > Benchmark (size) Mode Cnt Score Error Units > ZipFileOpen.openCloseZipFile 512 avgt 15 83007.325 ± 1446.716 ns/op > ZipFileOpen.openCloseZipFile1024 avgt 15 154550.631 ± 2166.673 ns/op > > After: > > Benchmark (size) Mode Cnt Score Error Units > ZipFileOpen.openCloseZipFile 512 avgt 15 79512.902 ± 814.449 ns/op > ZipFileOpen.openCloseZipFile1024 avgt 15 147892.522 ± 2744.017 ns/op FWIW the micro is derived from the sibling `ZipFileGetEntry` micro in the same directory. It's not exactly necessary for this use case to add such a benchmark, but I think there's value in verifying that optimizing `checkedHash` improves `ZipFile` setup and adding the micro might allow us to find further opportunities down the line. - PR: https://git.openjdk.org/jdk/pull/12077
