8336831: Optimize StringConcatHelper.simpleConcat #20253

wenshao · 2024-07-19T13:10:43Z

Currently simpleConcat is implemented using mix and prepend, but in this simple scenario, it can be implemented in a simpler way and can improve performance.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8336831: Optimize StringConcatHelper.simpleConcat (Enhancement - P4)

Reviewers

Chen Liang (@liach - Reviewer)
Claes Redestad (@cl4es - Reviewer)
Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20253/head:pull/20253
$ git checkout pull/20253

Update a local copy of the PR:
$ git checkout pull/20253
$ git pull https://git.openjdk.org/jdk.git pull/20253/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20253

View PR using the GUI difftool:
$ git pr show -t 20253

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20253.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-19T13:11:38Z

👋 Welcome back wenshao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-07-19T13:12:19Z

@wenshao 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:

8336831: Optimize StringConcatHelper.simpleConcat

Reviewed-by: liach, redestad, rriggs

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 6 new commits pushed to the master branch:

4c91d5c: 8322133: getParameterSpec(ECGenParameterSpec.class) on EC AlgorithmParameters does not return standard names
2f2223d: 8336944: Shenandoah: Should only relativize stack chunks for successful evacuations
8efcb40: 8335823: Update --release 23 symbol information for JDK 23 build 33
8e1f17e: 8327054: DiagnosticCommand Compiler.perfmap does not log on output()
0e555b5: 8204582: Extra spaces in jlink documentation make it incorrect.
a2a236f: 8335939: Hide element writing across the ClassFile API

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach, @cl4es, @RogerRiggs) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-07-19T13:12:42Z

@wenshao The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

wenshao · 2024-07-19T13:13:45Z

Below are the performance numbers running on a MacBook M1 Pro, which is 8.71% faster

-Benchmark                             (intValue)  Mode  Cnt  Score   Error  Units
-StringConcat.concatMethodConstString        4711  avgt   15  5.499 ? 0.046  ns/op

+Benchmark                             (intValue)  Mode  Cnt  Score   Error  Units
+StringConcat.concatMethodConstString        4711  avgt   15  5.058 ? 0.012  ns/op +8.71%

mlbridge · 2024-07-19T13:31:56Z

Webrevs

src/java.base/share/classes/java/lang/StringConcatHelper.java

liach · 2024-07-19T17:35:45Z

Also beware of #19927: it simplifies the prepend part. How does that patch run in your benchmark?

wenshao · 2024-07-19T19:23:26Z

I have changed it to a shared newArray, including out of bounds checking, The performance improvement is still the same as before.

-Benchmark                             (intValue)  Mode  Cnt  Score   Error  Units (master)
-StringConcat.concatMethodConstString        4711  avgt   15  5.437 ? 0.073  ns/op

+Benchmark                             (intValue)  Mode  Cnt  Score   Error  Units (38a6f5602f0f1f4a9eec4b7f0ac1b5c4e661f667)
+StringConcat.concatMethodConstString        4711  avgt   15  5.027 ? 0.080  ns/op +8.15%

wenshao · 2024-07-19T20:13:51Z

Also beware of #19927: it simplifies the prepend part. How does that patch run in your benchmark?

The performance comparison I made is based on the latest master branch (c25c4896ad9ef031e3cddec493aef66ff87c48a7), after #19927.

src/java.base/share/classes/java/lang/StringConcatHelper.java

liach · 2024-07-19T20:43:42Z

As annoying and risky as this first appeared, this patch is actually in quite good shape: The usage of getBytes is similar to those in StringUTF16, and there's no reason to further complicate this by splitting the handling into StringLatin1 and StringUTF16. 👍

Another question for you: can you check out the original form over here too? 781fb29#diff-f8131d8a48caf7cfc908417fad241393c2ef55408172e9a28dcaa14b1d73e1fbL1968-L1981

simpleConcat is required to create new String objects and String.concat can just return the argument/original when one part is not empty. Is there any value to extract a common doConcat handling both non-empty strings, called by both methods after handling the empty cases?

wenshao · 2024-07-19T21:10:44Z

@liach is this what you want to change it to?

class String {
    public String concat(String str) {
        if (str.isEmpty()) {
            return this;
        }
        return StringConcatHelper.doConcat(this, str);
    }
}

class StringConcatHelper {
    @ForceInline
    static String simpleConcat(Object first, Object second) {
        String s1 = stringOf(first);
        String s2 = stringOf(second);
        if (s1.isEmpty()) {
            // newly created string required, see JLS 15.18.1
            return new String(s2);
        }
        if (s2.isEmpty()) {
            // newly created string required, see JLS 15.18.1
            return new String(s1);
        }
        return doConcat(s1, s2);
    }

    @ForceInline
    static String doConcat(String s1, String s2) {
        byte coder = (byte) (s1.coder() | s2.coder());
        int newLength = (s1.length() + s2.length()) << coder;
        byte[] buf = newArray(newLength);
        s1.getBytes(buf, 0, coder);
        s2.getBytes(buf, s1.length(), coder);
        return new String(buf, coder);
    }
}

liach · 2024-07-19T21:23:20Z

Yep. I think this looks cleaner and avoids redundant checks on the String.concat path. What do you think? Is this slower according to your benchmarks?

src/java.base/share/classes/java/lang/String.java

liach · 2024-07-19T21:37:46Z

This patch looks ideal to me; but other reviewers may have different opinions on this review. I am ready to answer their questions for you.

I will approve your patch once you fixed the null and empty check in String.concat and the builds look good. Don't change your patch too often, as that means previous build results will be invalid.

Co-authored-by: Chen Liang <liach@openjdk.org>

wenshao · 2024-07-19T21:45:15Z

Below are the performance numbers of the latest version running on a MacBook M1 Pro, which is 9.19% faster

-Benchmark                             (intValue)  Mode  Cnt  Score   Error  Units (base c25c4896ad9ef031e3cddec493aef66ff87c48a7)
-StringConcat.concatMethodConstString        4711  avgt   15  5.440 ? 0.075  ns/op

+Benchmark                             (intValue)  Mode  Cnt  Score   Error  Units (current 69901157e4dae9018abd727956f60fd11b8fa252)
+StringConcat.concatMethodConstString        4711  avgt   15  4.982 ? 0.019  ns/op +9.19%

RogerRiggs · 2024-07-19T21:46:07Z

I'll take a look at this next week.

RogerRiggs · 2024-07-19T21:49:47Z

/reviewers 2 reviewer

openjdk · 2024-07-19T21:51:06Z

@RogerRiggs
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

cl4es · 2024-07-19T22:28:40Z

FWIW one of the ideas when implementing StringConcatHelper.simpleConcat was that by using the primitives used by the StringConcatFactory as straightforwardly as possible the method acts as a documentation-of-sorts or guide to understand how the SCF expression trees are built up. It's not perfect, though. Concatenation of a String + constant would be handled differently for one. So perhaps the value as a guide is not high.

I'm also experimenting with replacing the MH-based strategy with spinning hidden, shareable classes instead. In that case we might actually be better off getting rid of the long indexCoder hacks and generate code more similar to the doConcat you've come up with here. The main benefit of combining coder and index/length into a single long was to reduce the MH combinator overheads, but if we're spinning code with access to optimized primitives then that isn't really needed.

wenshao · 2024-07-22T07:24:08Z

If code is generated through ClassFile, how to access the private methods of classes such as String/StringConcatHelper?

In that case, you have to encode those methods as live MethodHandle objects, passed to the generated class with defineHiddenClassWithClassData (just pass the class data object) They can be retrieved in generated code with a CONDY using MethodHandles.classData. (This is how the MethodHandle LambdaForm classes referred to other MethodHandles)

You don't really need to worry about experimenting with bytecode generation right now. This patch as is is very good, and tier 1-3 tests pass. Bytecode gen is aimed at more complex scenarios with multiple constants and various types of arguments instead of this simple concat case.

@liach Can you give a sample code?

liach · 2024-07-22T14:28:12Z

@wenshao Answered at #20273 (comment) since this question is more closely related to that patch.

…at_202407 # Conflicts: # src/java.base/share/classes/java/lang/StringConcatHelper.java

cl4es

This change seems reasonable, and we will need the newArray(int) method in future work. This patch dials back on the idea that simpleConcat is an "explainer" for what StringConcatFactory is doing, but as it was already imprecise in that regard we should favor simplicity and performance here.

src/java.base/share/classes/java/lang/String.java

openjdk · 2024-07-23T12:07:09Z

@wenshao this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout optim_simple_concat_202407
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

liach

Merge conflict fix looks good.

liach · 2024-07-23T13:21:54Z

Please hold off a few hours before integration: I will see if @RogerRiggs has any concerns, and I will run the CI tests for your patch again.

RogerRiggs

looks good.

liach · 2024-07-23T15:53:48Z

Tier 1-3 tests pass.

wenshao · 2024-07-23T20:07:56Z

/integrate

openjdk · 2024-07-23T20:09:54Z

@wenshao
Your change (at version a2be39a) is now ready to be sponsored by a Committer.

liach · 2024-07-23T20:19:23Z

/sponsor

openjdk · 2024-07-23T20:21:10Z

Going to push as commit 476d2ae.
Since your change was applied there have been 6 commits pushed to the master branch:

4c91d5c: 8322133: getParameterSpec(ECGenParameterSpec.class) on EC AlgorithmParameters does not return standard names
2f2223d: 8336944: Shenandoah: Should only relativize stack chunks for successful evacuations
8efcb40: 8335823: Update --release 23 symbol information for JDK 23 build 33
8e1f17e: 8327054: DiagnosticCommand Compiler.perfmap does not log on output()
0e555b5: 8204582: Extra spaces in jlink documentation make it incorrect.
a2a236f: 8335939: Hide element writing across the ClassFile API

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-23T20:21:16Z

@liach @wenshao Pushed as commit 476d2ae.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

optimize String.concat

Loading
Loading status checks…

062d313

openjdk bot added the core-libs label Jul 19, 2024

wenshao changed the title ~~Optimize StringConcatHelper.simpleConcat~~ 8336831: Optimize StringConcatHelper.simpleConcat Jul 19, 2024

openjdk bot added the rfr label Jul 19, 2024

liach suggested changes Jul 19, 2024

View reviewed changes

src/java.base/share/classes/java/lang/StringConcatHelper.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/lang/StringConcatHelper.java Outdated Show resolved Hide resolved

share newArray

38a6f56

liach reviewed Jul 19, 2024

View reviewed changes

src/java.base/share/classes/java/lang/StringConcatHelper.java Show resolved Hide resolved

wenshao added 2 commits July 20, 2024 05:29

extract a common doConcat handling both non-empty strings

a509b7c

handle null argument

9b39c4c

liach reviewed Jul 19, 2024

View reviewed changes

src/java.base/share/classes/java/lang/String.java Outdated Show resolved Hide resolved

wenshao and others added 2 commits July 20, 2024 05:37

add comments

2171ad9

Update src/java.base/share/classes/java/lang/String.java

6990115

Co-authored-by: Chen Liang <liach@openjdk.org>

Merge remote-tracking branch 'upstream/master' into optim_simple_conc…

20f8863

…at_202407 # Conflicts: # src/java.base/share/classes/java/lang/StringConcatHelper.java

cl4es approved these changes Jul 23, 2024

View reviewed changes

src/java.base/share/classes/java/lang/String.java Outdated Show resolved Hide resolved

openjdk bot added the merge-conflict label Jul 23, 2024

merge from master

61d6b64

openjdk bot removed the merge-conflict label Jul 23, 2024

liach approved these changes Jul 23, 2024

View reviewed changes

reduce change

a2be39a

cl4es approved these changes Jul 23, 2024

View reviewed changes

liach approved these changes Jul 23, 2024

View reviewed changes

openjdk bot added the ready label Jul 23, 2024

RogerRiggs approved these changes Jul 23, 2024

View reviewed changes

openjdk bot added the sponsor label Jul 23, 2024

openjdk bot added the integrated label Jul 23, 2024

openjdk bot closed this Jul 23, 2024

openjdk bot removed ready rfr sponsor labels Jul 23, 2024

graalvmbot mentioned this pull request Jul 29, 2024

[GR-56297] Update labsjdk to 24+8-jvmci-b01 oracle/graal#9406

Merged

wenshao mentioned this pull request Dec 28, 2024

[Backport]String concatenation & StringBuilder appendNull/appendBoolean dragonwell-project/dragonwell21#160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8336831: Optimize StringConcatHelper.simpleConcat #20253

8336831: Optimize StringConcatHelper.simpleConcat #20253

wenshao commented Jul 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 19, 2024

openjdk bot commented Jul 19, 2024 •

edited

Loading

openjdk bot commented Jul 19, 2024

wenshao commented Jul 19, 2024

mlbridge bot commented Jul 19, 2024 •

edited

Loading

liach commented Jul 19, 2024

wenshao commented Jul 19, 2024

wenshao commented Jul 19, 2024

liach commented Jul 19, 2024 •

edited

Loading

wenshao commented Jul 19, 2024

liach commented Jul 19, 2024

liach commented Jul 19, 2024

wenshao commented Jul 19, 2024

RogerRiggs commented Jul 19, 2024

RogerRiggs commented Jul 19, 2024

openjdk bot commented Jul 19, 2024

cl4es commented Jul 19, 2024

wenshao commented Jul 22, 2024

liach commented Jul 22, 2024

cl4es left a comment

openjdk bot commented Jul 23, 2024

liach left a comment

liach commented Jul 23, 2024

RogerRiggs left a comment

liach commented Jul 23, 2024

wenshao commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

liach commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

8336831: Optimize StringConcatHelper.simpleConcat #20253

8336831: Optimize StringConcatHelper.simpleConcat #20253

Conversation

wenshao commented Jul 19, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 19, 2024

openjdk bot commented Jul 19, 2024 • edited Loading

openjdk bot commented Jul 19, 2024

wenshao commented Jul 19, 2024

mlbridge bot commented Jul 19, 2024 • edited Loading

Webrevs

liach commented Jul 19, 2024

wenshao commented Jul 19, 2024

wenshao commented Jul 19, 2024

liach commented Jul 19, 2024 • edited Loading

wenshao commented Jul 19, 2024

liach commented Jul 19, 2024

liach commented Jul 19, 2024

wenshao commented Jul 19, 2024

RogerRiggs commented Jul 19, 2024

RogerRiggs commented Jul 19, 2024

openjdk bot commented Jul 19, 2024

cl4es commented Jul 19, 2024

wenshao commented Jul 22, 2024

liach commented Jul 22, 2024

cl4es left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 23, 2024

liach left a comment

Choose a reason for hiding this comment

liach commented Jul 23, 2024

RogerRiggs left a comment

Choose a reason for hiding this comment

liach commented Jul 23, 2024

wenshao commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

liach commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

wenshao commented Jul 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 19, 2024 •

edited

Loading

mlbridge bot commented Jul 19, 2024 •

edited

Loading

liach commented Jul 19, 2024 •

edited

Loading