RFR: 8337066: Repeated call of StringBuffer.reverse with double byte string returns wrong result [v2]

[Tobias Hartmann]

On Fri, 27 Sep 2024 19:46:15 GMT, Igor Veresov <iveresov at openjdk.org> wrote:

>> This is essentially a defensive forward port of a solution for an issue discovered in 11, where a use of pinned load (for which we disable almost all transforms in `LoadNode::Ideal()`) leads to a graph shape that is not expected by `insert_anti_dependences()`
>> 
>> The `insert_anti_dependences()` assumes that the memory input for a load is the root of the memory subgraph that it has to search for the possibly conflicting store. Usually this is true if we run all the memory optimizations but with pinned we don't.
>> 
>> Compare the good graph shape (with control dependency set to `UnknownControl`):
>> <img width="1128" alt="Good graph" src="https://github.com/user-attachments/assets/d2c72d20-bc3c-403f-b340-e7cfe6d1796d">
>> 
>> With the graph produce with the nodes pinned:
>> <img width="1206" alt="Bad graph" src="https://github.com/user-attachments/assets/bbd14b37-584b-4906-973e-93c9046975a6">
>> 
>> With the "bad graph" loads and store don't share the same memory graph root, and therefore are not considered by `insert_anti_dependences()`. The solution, I think, could be to walk up the memory chain of the load, skipping MergeMems, in order to get to the real root and then run the precedence edge insertion algorithm from there.
>
> Igor Veresov 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 one additional commit since the last revision:
> 
>   8337066: Repeated call of StringBuffer.reverse with double byte string returns wrong result
>   Summary: Make sure insert_anti_dependencies() starts from the right root

Looks good to me otherwise. You might want to run performance testing just to make sure.

src/hotspot/share/opto/gcm.cpp line 750:

> 748:   Node* initial_mem = load->in(MemNode::Memory);
> 749: 
> 750:   // We don't optimize memory graph for pinned loads, so we may need to raise the

Suggestion:

  // We don't optimize the memory graph for pinned loads, so we may need to raise the

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21222#pullrequestreview-2353479727
PR Review Comment: https://git.openjdk.org/jdk/pull/21222#discussion_r1791256151