Optimization for StringBuilder append boolean and null

[wenshao]

Improve the performance of StringBuilder's append(boolean) and append(null).

When the coder is latin1, treat null/fals/true as an int. If the coder is utf16, treat it as a long, use ByteArrayLittleEndian.setInt/setLong to write it into buf.

The performance numbers under MacBookPro M1 Max are as follows:

-Benchmark                             Mode  Cnt   Score   Error  Units
-StringBuilders.toStringCharWithBool8  avgt   15  15.766 ? 0.355  ns/op
-StringBuilders.toStringCharWithNull8  avgt   15  10.723 ? 0.019  ns/op

+Benchmark                             Mode  Cnt   Score   Error  Units
+StringBuilders.toStringCharWithBool8  avgt   15  13.023 ? 0.141  ns/op (+21.07)
+StringBuilders.toStringCharWithNull8  avgt   15  8.480  ? 0.027  ns/op (+26.46)

Please review and don't hesitate to critique my approach and patch.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Reviewers

• Chen Liang (@liach - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15990

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

Using diff file

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

[bridgekeeper]

👋 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]

@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]

@cl4es could you please review my code? Thank you!

[cl4es]

Are these common enough that 2-3ns/op is a real win on any real app?

Instead of obfuscating core library code with ByteArrayLittleEndian I really want us to spend some time analyzing the trivial byte[] write operations and investigate if/how JITs can be improved here.

[wenshao]

Are these common enough that 2-3ns/op is a real win on any real app?

Instead of obfuscating core library code with ByteArrayLittleEndian I really want us to spend some time analyzing the trivial byte[] write operations and investigate if/how JITs can be improved here.

I think StringBuilder.append(boolean/null) are common operations, and now we know that using ByteArrayLittleEndian can improve performance. Does JIT have any plans to do such optimization?

[liach]

Can you try replacing the individual char puts with putStringAt with constant arguments "null" "false" "true" and check the performance results? I expect the benchmarks to show copying a constant byte array in full (the latin-1 bytes are stored in those string constants, not sure about how it's carried out exactly) versus putting the bytes into string with unaligned writes.

[wenshao]

Can you try replacing the individual char puts with putStringAt with constant arguments "null" "false" "true" and check the performance results? I expect the benchmarks to show copying a constant byte array in full (the latin-1 bytes are stored in those string constants, not sure about how it's carried out exactly) versus putting the bytes into string with unaligned writes.

The implementation using append(b ? "true" : "false") will be slower than the baseline version

    public AbstractStringBuilder append(boolean b) {
        if (isLatin1()) {
            return append(b ? "true" : "false");
        }
        ...
    }

[bridgekeeper]

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@wenshao This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[wenshao]

/open

[openjdk]

@wenshao This pull request is now open

[cl4es]

We now have a PR for merging stores efficiently: #16245 It would be good to run the microbenchmarks you have evaluated here on that PR branch and see if that optimization narrows the gap. I have some hope we'll be able to dial back the use of ByteArray(LittleEndian) without sacrificing performance.

[wenshao]

PR #16245 performs better

• MacBookPro M1 Pro

Benchmark                             Mode  Cnt   Score   Error  Units # master
StringBuilders.toStringCharWithBool8  avgt   15  16.127 ? 0.223  ns/op

Benchmark                             Mode  Cnt   Score   Error  Units # pr/15990
StringBuilders.toStringCharWithBool8  avgt   15  13.955 ? 0.181  ns/op

Benchmark                             Mode  Cnt   Score   Error  Units # pr/16245
StringBuilders.toStringCharWithBool8  avgt   15  12.519 ? 0.228  ns/op

[bridgekeeper]

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[liach]

Suggested change

Redundant line.

[wenshao]

This extra line existed before this PR

[wenshao]

The current implementation will have performance degradation when the coder is UTF16, but if combined with PR #16245, it will have very good performance.

The performance numbers under MacBookPro M1 Max are as follows:

Benchmark                                  Mode  Cnt    Score    Error  Units  # master
StringBuilders.toStringCharUTF16WithBool8  avgt   15   40.139 ?  0.113  ns/op
StringBuilders.toStringCharUTF16WithNull8  avgt   15   90.630 ?  5.380  ns/op
StringBuilders.toStringCharWithBool8       avgt   15   89.899 ?  2.695  ns/op
StringBuilders.toStringCharWithNull8       avgt   15   25.028 ?  0.150  ns/op

Benchmark                                  Mode  Cnt    Score    Error  Units  # PR 15990 (0d4d519) current version
StringBuilders.toStringCharUTF16WithBool8  avgt   15   44.336 ?  0.190  ns/op
StringBuilders.toStringCharUTF16WithNull8  avgt   15  126.908 ? 22.450  ns/op
StringBuilders.toStringCharWithBool8       avgt   15   64.320 ?  1.573  ns/op
StringBuilders.toStringCharWithNull8       avgt   15   23.825 ?  0.152  ns/op

Benchmark                                  Mode  Cnt    Score    Error  Units  # PR 16245
StringBuilders.toStringCharUTF16WithBool8  avgt   15   39.976 ?  0.179  ns/op
StringBuilders.toStringCharUTF16WithNull8  avgt   15   90.817 ?  5.681  ns/op
StringBuilders.toStringCharWithBool8       avgt   15   36.279 ?  1.273  ns/op
StringBuilders.toStringCharWithNull8       avgt   15   21.696 ?  0.109  ns/op

Benchmark                                  Mode  Cnt    Score    Error  Units  # PR 16245 + 15990
StringBuilders.toStringCharUTF16WithBool8  avgt   15   18.389 ?  0.408  ns/op
StringBuilders.toStringCharUTF16WithNull8  avgt   15   12.929 ?  0.023  ns/op
StringBuilders.toStringCharWithBool8       avgt   15   12.395 ?  0.176  ns/op
StringBuilders.toStringCharWithNull8       avgt   15    7.999 ?  0.109  ns/op

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[bridgekeeper]

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@wenshao This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.