On Mon, 4 Sep 2023 06:54:40 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> This PR proposes adding a new method to BitSet that provides an immutable
>> snapshot of the set in the form of an `IntPredicate`.
>>
>> The predicate is eligible for constant folding.
>>
>> Here are some classes in the JDK that would benefit directly from
>> constant-folding of BitSets:
>>
>> PoolReader (6)
>> URLEncoder (1) - Updated in this PR
>> HtmlTree (2)
>>
>> More over, the implementation of the predicate is @ValueBased and this would
>> provide additional benefits in the future.
>>
>> Initial benchmarks with the URLEncoder show encouraging results:
>>
>>
>> Name (encodeChars) (maxLength) (unchanged) Cnt
>> Base Error Test Error Unit Diff%
>> URLEncodeDecode.testEncodeUTF8 6 1024 0 15
>> 2,371 ± 0,016 2,073 ± 0,184 ms/op 12,6% (p = 0,000*)
>> URLEncodeDecode.testEncodeUTF8 6 1024 75 15
>> 1,772 ± 0,013 1,387 ± 0,032 ms/op 21,8% (p = 0,000*)
>> URLEncodeDecode.testEncodeUTF8 6 1024 100 15
>> 1,230 ± 0,009 1,140 ± 0,011 ms/op 7,3% (p = 0,000*)
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Do not propagate Integer.MAX_VALUE BitSets
Looks good to me. Since we're not adding any public API I think it's ok to
include the `URLEncoder` changes so that we can use that benchmark as a proxy
for verifying `BitSet::asPredicate` performance.
src/java.base/share/classes/jdk/internal/util/ImmutableBitSetPredicate.java
line 60:
> 58: public static IntPredicate of(BitSet original) {
> 59: // Do not propagate the Integer.MAX_VALUE issue
> 60: if (Integer.toUnsignedLong(original.length()) >
> Integer.MAX_VALUE) {
I'm not so sure about this. While there are issues related to
`BitSet::set(Integer.MAX_VALUE)` (makes `BitSet::length` overflow to negative),
the `get/test` operation is not adversely affected. Since this is an internal
API adding limitations here seem a bit gratuitous and could instead be
discussed if/when taking this public.
test/jdk/java/util/BitSet/ImmutableBitSet.java line 2:
> 1: /*
> 2: * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
Suggestion:
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
-------------
Marked as reviewed by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15530#pullrequestreview-1609364523
PR Review Comment: https://git.openjdk.org/jdk/pull/15530#discussion_r1314830577
PR Review Comment: https://git.openjdk.org/jdk/pull/15530#discussion_r1314830746
