On Tue, 7 Feb 2023 15:25:05 GMT, Claes Redestad <redes...@openjdk.org> wrote:

>> This adds a local, specialized `copyBytes` method to `String` that avoids 
>> certain redundant range checks and clamping that JIT has issues removing 
>> fully.
>> 
>> This has a small but statistically significant effect on `String` 
>> microbenchmarks, eg.:
>> 
>> Baseline
>> 
>> Benchmark                                            (size)  Mode  Cnt   
>> Score   Error  Units
>> StringConstructor.newStringFromArray                      7  avgt   15  
>> 16.817 ± 0.369  ns/op
>> StringConstructor.newStringFromArrayWithCharset           7  avgt   15  
>> 16.866 ± 0.449  ns/op
>> StringConstructor.newStringFromArrayWithCharsetName       7  avgt   15  
>> 22.198 ± 0.396  ns/op
>> 
>> 
>> Patch:
>> 
>> Benchmark                                            (size)  Mode  Cnt   
>> Score   Error  Units
>> StringConstructor.newStringFromArray                      7  avgt   15  
>> 15.477 ± 0.342  ns/op
>> StringConstructor.newStringFromArrayWithCharset           7  avgt   15  
>> 15.557 ± 0.352  ns/op
>> StringConstructor.newStringFromArrayWithCharsetName       7  avgt   15  
>> 21.272 ± 0.398  ns/op
>> 
>> 
>> Care has to be taken to ensure preconditions have been checked when using 
>> `checkBytes`. In the case of `String(AbstractStringBuilder)` there's a 
>> possible pre-existing issue where the constructor might either throw an 
>> exception or truncate the buffer if the builder byte array and length is not 
>> in agreement (theoretically possible if you clear/remove and call 
>> `trimToSize()` concurrently). Adding an explicit check here seem to be the 
>> right thing to do regardless of this RFE.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   copyrights

It is at least possible that splitting `Arrays::copyOfRange` would make it 
inline more readily in all use cases as well as this particular one.

By splitting, in this case, I mean moving the first three lines into a helper 
routine called `copyArrayChecks`, and returning `newLength` as a result.  That 
would make it inline better, I’m pretty sure.

It’s sad and true, until we get a better inliner, which of course will disrupt 
the ecosystem because there is no unique best answer to an inlining problem (if 
it is complex enough, and they are).  So for now we pretend to be good O-O 
programmers and code separate concerns in separate methods.

-------------

PR: https://git.openjdk.org/jdk/pull/12453

Reply via email to