RFR: 8278518: String(byte[], int, int, Charset) constructor and String.translateEscapes() miss bounds check elimination [v3]

[Roland Westrelin]

On Thu, 13 Jan 2022 18:18:30 GMT, John R Rose <jrose at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - whitespaces
>>  - more review
>>  - simple benchmark
>>  - benchmark
>
> This is a good improvement.  I agree that the type-flow pass "plays dumb" about graph structure; it was originally designed even "dumber".  The idea was that following phases could arrange loop structure all by themselves. But this gave up some early optimizations in the parsing phase, which is too early for loop inference.  So type-flow has gotten "smarter" over the years, to give the parsing phase more foresight about loop structure.  This new improvement is appropriate to that design.
> 
> As a matter of style and readability I suggest threading the `ciMethod` pointer (which is the root for getting profile info out) through instance variables rather than random extra arguments.  The problem with random extra arguments is they could be any (random) `ciMethod` but we know it's only the statically relevant method to the whole `ciTypeFlow` pass.  You can't tell from reading the code (except as a whole) what that random argument is, but you *can* if the `ciMethod` in question is the one stored in `ciTypeFlow::_method`.
> 
> So, I'd use the inner/outer class design pattern, where `Loop` gets an `_outer` link that points to the containing `ciTypeFlow`, which in turn has the `_method` field you need.  Just add a constructor argument to Loop to set up the `_outer` link and you don't need the ad hoc extra argument to `sorted_merge`.
> 
> I notice that you cleaned up `clone_loop_heads` in the same way, bravo.
> 
> I don't fully follow the open-coded sorting logic in `sorted_merge`, and I wish it could be made more clear.  The expression `current_count == lp_count` is particularly puzzling to me; either it should be a `<=` or `>=` comparison, or it is some sort of tie-breaking logic.  The role of all of those comparisons would be easier to follow if there were (I don't know if it's possible?) a separately factored (partial) ordering function, as a static helper function.  The problem with open-coding the comparison logic in an insertion sort algorithm, as you have here, is that if you have multiple sorting keys, it's really hard to verify that they are correctly applied.
> 
> Also (minor nit) you use the pattern `if (pred1) { break; }; if (pred2) { break; }` and also the equivalent pattern `if (pred1) { break; } else if (pred2) { break; }`.  I prefer the first, and would find the code more readable if you just picked one or the other in any case.

Thanks for the review @rose00. What about the updated change?

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

PR: https://git.openjdk.java.net/jdk/pull/7034