Hello,
thanks for doing this! I've checked it myself with similar benchmark
@Warmup(iterations = 10, time = 1)
@Measurement(iterations = 10, time = 1)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(value = 3, jvmArgsAppend = {"-Xms4g", "-Xmx4g", "-XX:+UseParallelGC"})
public class SubstringAndStripBenchmark {
private static final String str = "Tolstoy";
@Benchmark
public String substring() {
return str.substring(1, 1);
}
}
Which gave me this output:
Before
Benchmark Mode
Cnt Score Error Units
SubstringAndStripBenchmark.substring avgt
30 5.877 ± 0.066 ns/op
SubstringAndStripBenchmark.substring:·gc.alloc.rate avgt
30 4325.945 ± 47.259 MB/sec
SubstringAndStripBenchmark.substring:·gc.alloc.rate.norm avgt
30 40.003 ± 0.001 B/op
SubstringAndStripBenchmark.substring:·gc.churn.G1_Eden_Space avgt
30 4338.893 ± 86.555 MB/sec
SubstringAndStripBenchmark.substring:·gc.churn.G1_Eden_Space.norm avgt
30 40.122 ± 0.647 B/op
SubstringAndStripBenchmark.substring:·gc.churn.G1_Survivor_Space avgt
30 0.015 ± 0.003 MB/sec
SubstringAndStripBenchmark.substring:·gc.churn.G1_Survivor_Space.norm avgt
30 ≈ 10⁻⁴ B/op
SubstringAndStripBenchmark.substring:·gc.count avgt
30 557.000 counts
SubstringAndStripBenchmark.substring:·gc.time avgt
30 387.000 ms
After
SubstringAndStripBenchmark.substring avgt
30 2.423 ± 0.172 ns/op
SubstringAndStripBenchmark.substring:·gc.alloc.rate avgt
30 0.001 ± 0.001 MB/sec
SubstringAndStripBenchmark.substring:·gc.alloc.rate.norm avgt
30 ≈ 10⁻⁵ B/op
SubstringAndStripBenchmark.substring:·gc.count avgt
30 ≈ 0 counts
So with this change we save 40 bytes when empty String returned and execution
takes > 2x less time.
With best regards,
Sergey Tsypanov
26.02.2020, 16:11, "Claes Redestad" <claes.redes...@oracle.com>:
> Hi,
>
> I took the liberty to file
> https://bugs.openjdk.java.net/browse/JDK-8240094 for this.
>
> Webrev: http://cr.openjdk.java.net/~redestad/8240094/open.00/
>
> I added a trivial micro and verified that there's a roughly 2.5x
> improvement in this targeted test on my machine (~10 ns/op -> ~4ns/op),
> while staying neutral on a selection of benchmarks that exercise
> newString.
>
> /Claes
>
> On 2020-02-26 14:19, Claes Redestad wrote:
>> On 2020-02-26 11:01, Сергей Цыпанов wrote:
>>> Currently we have
>>>
>>> public static String stripLeading(byte[] value) {
>>> int left = indexOfNonWhitespace(value);
>>> if (left == value.length) {
>>> return "";
>>> }
>>> return (left != 0) ? newString(value, left, value.length - left) :
>>> null;
>>> }
>>>
>>> With the patch we change behaviour of this method for the case when
>>> value.length == 0:
>>>
>>> public static String stripLeading(byte[] value) {
>>> int left = indexOfNonWhitespace(value);
>>> return (left != 0) ? newString(value, left, value.length - left) :
>>> null;
>>> }
>>>
>>> Unlike original method this code returns null instead of "" for empty
>>> array.
>>> This does not affect caller (String.stripLeading()) i. e. visible
>>> behaviour
>>> remains the same for String user, but is it OK in general?
>>
>> One observable difference on the public API I think will happen here is
>> that new String("").stripLeading() == "" will change from true to false,
>> since the null return from the inner method mean we return the argument.
>>
>> From a compatibility point of view I think this should be fine, as
>> the identity of the returned empty string isn't specified. I don't think
>> a CSR is required, but bringing it up since others think otherwise.
>>
>> Overall I like how the patch now cleans things up a bit on top of
>> (likely) optimizing a few edge cases, and can volunteer to sponsor it if
>> no one else has spoken up already.
>>
>> Doing some quick performance testing and possibly adding a
>> microbenchmark before push would be good.
>>
>> Thanks!
>>
>> /Claes
