RFR: 8349183: [BACKOUT] Optimization for StringBuilder append boolean & null [v2]

Chen Liang liach at openjdk.org
Mon Feb 3 17:39:49 UTC 2025


On Mon, 3 Feb 2025 16:37:09 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which backs out the commit that was introduced for https://bugs.openjdk.org/browse/JDK-8333893?
>> 
>> The comment in the PR review of that issue https://github.com/openjdk/jdk/pull/19626#issuecomment-2628937397 explains what the issue is with the change that was integrated. Furthermore, one part of that original change introduced a few internal methods. These internal methods were then used in few other places within the JDK through https://bugs.openjdk.org/browse/JDK-8343650. As a result, this backout PR also reverts the change that was done in JDK-8343650.
>> 
>> The backout was done as follows, using `git revert` against the 2 relevant commits:
>> 
>> 
>> git revert 74ae3c688b37e693e20eb4e17c631897c5464400
>> git revert 5890d9438bbde88b89070052926a2eafe13d7b42
>> 
>> The revert of `5890d9438bbde88b89070052926a2eafe13d7b42` wasn't clean and I had to resolve a trivial conflict in `StringLatin1.java`.
>> 
>> tier1, tier2 and tier3 testing is currently in progress with this change. Once this is integrated into mainline, a corresponding backport will be done to `jdk24` branch with the requisite approvals.
>
> Jaikiran Pai has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision:
> 
>  - Revert "8333893: Optimization for StringBuilder append boolean & null"
>    
>    This reverts commit 5890d9438bbde88b89070052926a2eafe13d7b42.
>  - Revert "8343650: Reuse StringLatin1::putCharsAt and StringUTF16::putCharsAt"
>    
>    This reverts commit 74ae3c688b37e693e20eb4e17c631897c5464400.

Also for append(char): it uses putCharSB which does a check and calls unchecked putChar. putCharsAt is always checked.

src/java.base/share/classes/java/lang/StringUTF16.java line 1538:

> 1536:     public static int putCharsAt(byte[] value, int i, char c1, char c2, char c3, char c4) {
> 1537:         int end = i + 4;
> 1538:         checkBoundsBeginEnd(i, end, value);

We have this explicit check for null and true, false has another bound check. This backout version should be safe.

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

Marked as reviewed by liach (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23420#pullrequestreview-2590596912
PR Review Comment: https://git.openjdk.org/jdk/pull/23420#discussion_r1939778610


More information about the core-libs-dev mailing list