RFR: 8341696: C2: Non-fluid StringBuilder pattern bails out in OptoStringConcat [v5]

[Emanuel Peter]

On Fri, 10 Jan 2025 09:37:04 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

>> Extends stringopts to also recognize non-fluid uses of StringBuilder and optimize them the same way.
>> 
>> For example, this basic case was not optimized before and is optimized with this PR:
>> 
>> 
>> StringBuilder sb = new StringBuilder();
>> sb.append("a");
>> sb.append(a);
>> return sb.toString();
>
> Theo Weidmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make code more clear

Changes requested by epeter (Reviewer).

src/hotspot/share/opto/stringopts.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2009, 2025, Oracle and/or its affiliates. All rights reserved.

src/hotspot/share/opto/stringopts.cpp line 439:

> 437:       }
> 438: #endif
> 439:       return ProcessAppendResult::AbortOptimization;

Question: is it a problem that we already did `sc->add_control(cnode);` in this case?

src/hotspot/share/opto/stringopts.cpp line 480:

> 478: //   .append("bar")
> 479: //   .append(123)
> 480: //   .toString(); // "foobar123"+

Is the "+" on purpose here?

src/hotspot/share/opto/stringopts.cpp line 553:

> 551:             use->method()->name() == ciSymbols::object_initializer_name() &&
> 552:             use->method()->holder() == m->holder()) {
> 553:           //  Matched the constructor.

Suggestion:

          // Matched the constructor.

src/hotspot/share/opto/stringopts.cpp line 619:

> 617:           // - If we found an append, that's perfect. Nothing further to do.
> 618:           // - If this is a call to an unrelated method, validate_mem_flow() (and validate_control_flow())
> 619:           //   will later check if this call prevents the optimization. So nothing to do here.

Ok, so why do we continue on and not return yet?

src/hotspot/share/opto/stringopts.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2009, 2025, Oracle and/or its affiliates. All rights reserved.

test/hotspot/jtreg/compiler/stringopts/TestFluidAndNonFluid.java line 2:

> 1: /*
> 2:  * Copyright (c) 2024 Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved.

test/micro/org/openjdk/bench/vm/compiler/FluidSBBench.java line 3:

> 1: /*
> 2:  * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
> 3:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

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

PR Review: https://git.openjdk.org/jdk/pull/22537#pullrequestreview-2555868484
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918416050
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918439092
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918419945
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918425696
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918430541
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918441371
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918444675
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1918445360