On Tue, 7 Feb 2023 18:35:32 GMT, John R Rose <jr...@openjdk.org> wrote:
> Our source code is a reference implementation, and people will look at this > change as evidence that `Arrays::copyOfRange` should be hand-inlined by savvy > coders. Surely we could also fix this small performance pothole by improving > C2’s treatment of `Arrays.copyOfRange`. That would benefit all users as well, > not just `String`. That is our preferred way to handle things. > > On the other hand, `String` is an important class and worthy of every tiny > tweak we give it. Do we need this fix now? If so, I suggest putting in a > comment in the code which says something like “normally one would use > Arrays.copyOfRange here, but we get slightly better code in this particular > case”. Also, regarding the bug against the JIT, I suggest that we back out > this change to `String` when that JIT bug is fixed. Perhaps the comment in > `String` should reference that bug. It might be that the redundant checks in `Arrays.copyOfRange` would be eliminated properly with more inlining, and that the issue here is that the affected `String` constructor is particularly gnarly. This gnarliness is due 1) the need to construct the `value` and `coder` in one go and 2) the lack of multiple return values. Give me a value-based record type so we can split apart the constructor into charset-specific methods with zero overhead and we might be able to split up the logic into better-sized chunks. But yes, I will add some commentary to the effect that this should ideally be handled by our JIT, along with comments that the method deliberately avoids safety checks. ------------- PR: https://git.openjdk.org/jdk/pull/12453
