8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings #5291
Conversation
|
👋 Welcome back redestad! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Microbenchmark results: Baseline Patched: |
|
Simplified further after realizing |
|
Hi, just curious how have you found out that the code should be extracted into a separate methods? Profiler? |
I saw that |
| } | ||
|
|
||
| private void putStringAt(int index, String str, int off, int end) { | ||
| inflateIfNeededFor(str); | ||
| str.getBytes(value, off, index, coder, end - off); | ||
| } | ||
|
|
||
| private void putStringAt(int index, String str) { |
There was a problem hiding this comment.
Can we replace all the calls to this method with calls to previous method as putStringAt(index, str, 0, str.length()) taking into account that in all usecases str.length() is already calculated into a local var?
There was a problem hiding this comment.
No, I don't think so. The only use of this I can find is at line 1298 which effectively adds a substring: putStringAt(dstOffset, (String) s, start, end);
There was a problem hiding this comment.
What about lines 582, 1003 and 1175? E.g. 582
public AbstractStringBuilder append(String str) {
if (str == null) {
return appendNull();
}
int len = str.length();
ensureCapacityInternal(count + len);
putStringAt(count, str); // couldn't it be putStringAt(count, str, 0, len);
count += len;
return this;
}Doing this here and in other places allows to rid private void putStringAt(int index, String str) completely and reduce one nested method call, right?
There was a problem hiding this comment.
I think you've got the wrong idea: We want to use putStringAt(int, String) now since it can call into String.getBytes(String, int, byte), which has a simpler and more efficient implementation than String.getBytes(String, int, int, byte, int), since it avoids a couple of << coder operations. This makes up for most of the improvement between my initial and the current version of this patch.
(There's also no nested delegation from putStringAt(int, String) to putStringAt(int, String, int, int) in my proposed patch.)
There was a problem hiding this comment.
You are right, I've applied your patch locally incompletely and misunderstood it, my bad :)
| private void putStringAt(int index, String str, int off, int end) { | ||
| if (getCoder() != str.coder()) { | ||
| private void inflateIfNeededFor(String input) { | ||
| if (COMPACT_STRINGS && (coder != input.coder())) { |
There was a problem hiding this comment.
I'm not completely sure whether it's a good idea in terms of maintainability, but I think this can be simplified a bit more. Currently in both String and ASB we have implementations of coder() very much alike:
// ASB
final byte getCoder() {
return COMPACT_STRINGS ? coder : UTF16;
}
//String
byte getCoder() {
return COMPACT_STRINGS ? coder : UTF16;
}Here we have this condition
if (COMPACT_STRINGS && (coder != input.getCoder())) {}where the right operand of && is evaluated only when COMPACT_STRINGS is true and hence it always returns the value of coder field. This means we can reference it directly as
if (COMPACT_STRINGS && (coder != input.coder)) {}There was a problem hiding this comment.
I'm not sure if this'd give us enough to motivate the refactor, especially since we'd have to widen the visibility of String.coder. Maybe startup could be helped a little. Either way it feels out of scope for this change, don't you think?
|
@cl4es This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 12 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks for reviews, everyone! /integrate |
|
Going to push as commit 98fa533.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
Refactor to improve inlining, which helps some microbenchmarks exer StringBuilder.append(String)
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5291/head:pull/5291$ git checkout pull/5291Update a local copy of the PR:
$ git checkout pull/5291$ git pull https://git.openjdk.java.net/jdk pull/5291/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5291View PR using the GUI difftool:
$ git pr show -t 5291Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5291.diff