RFR: 8333893: Optimization for StringBuilder append boolean & null [v20]

[altrisi]

On Fri, 18 Oct 2024 21:56:53 GMT, Shaojin Wen <swen at openjdk.org> wrote:

>> After PR https://github.com/openjdk/jdk/pull/16245, C2 optimizes stores into primitive arrays by combining values ​​into larger stores.
>> 
>> This PR rewrites the code of appendNull and append(boolean) methods so that these two methods can be optimized by C2.
>
> Shaojin Wen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 26 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/optim_str_builder_append_202406' into optim_str_builder_append_202406
>  - fix build error
>  - Merge remote-tracking branch 'upstream/master' into optim_str_builder_append_202406
>  - Merge remote-tracking branch 'upstream/master' into optim_str_builder_append_202406
>  - Merge remote-tracking branch 'origin/optim_str_builder_append_202406' into optim_str_builder_append_202406
>  - Merge remote-tracking branch 'upstream/master' into optim_str_builder_append_202406
>  - Merge remote-tracking branch 'upstream/master' into optim_str_builder_append_202406
>  - revert test
>  - Merge remote-tracking branch 'upstream/master' into optim_str_builder_append_202406
>  - Merge remote-tracking branch 'upstream/master' into optim_str_builder_append_202406
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/26d36e92...457735c9

I have concerns with the changes here, because of what it can cause when an (incorrect) program is sharing a StringBuilder between multiple threads.

Even without any reordering, a thread could start an `append(boolean|null)`, pass the `ensureCapacityInternal` call, then another thread up the count by enough to no longer have enough capacity. Then the first thread could proceed to read the new (no longer enough!) count, and write without any bounds checks outside of the array. Easy to reproduce with a debugger and some breakpoints.

The previous code would have thrown an exception at an explicit or implicit bounds check, but with the changes here that'd no longer happen.

<details>
<summary>For example</summary>

|(initial state)|
|-----------------|
| value.length = 16 |
| count = 11 |

| Thread 1                  | Thread 2           |
|---------------------------|--------------------|
| append(true)              |                    |
| -ensureCap(11+4)          |                    |
| --nothing to do            |                    |
|                           | append("str")      |
|                           | - ensureCap(11+3)  |
|                           | -- nothing to do      |
|                           | - ...              |
|                           | - this.count <- 14 |
| - cnt <- this.count (14!) |                    |
| - val <- this.value       |                    |
| - val[cnt] <-u 't'         |                    |
| - val[cnt+1] <-u 'r'        |                    |
| - val[cnt+2 (16!)] <-u 'u'        |                    |
| - ...                          |                    |
</details>

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

PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2628937397