8353638: C2: deoptimization and re-execution cycle with StringBuilder

[marc-chevalier]

Unlike what was assumed at first, it is quite different from JDK-8346989. The problem is actually unrelated to StringBuilder, but has to do with the underlying array allocation.

Here, the problem is that the array allocation function, that is throwing when given a negative length, causes a deopt rather than using the compiled exception handlers. This is an old workaround, and the flag StressCompiledExceptionHandlers to rather use compiled handlers instead of deopting was added in JDK-8004741 in 2012. This flag is used in testing since october 2022.

So maybe it's time to use the compiled exception handlers! I propose to turn them on by default, and instead, add a diagnostic flag to deopt instead, in case something goes wrong. Doing so improve the performance to match the ones of C1 (both for direct array allocation, and StringBuilder construction). For instance, with the case given in the JBS issue:

Stop at level 0
CompileCommand: compileonly C.test* bool compileonly = true

real	0m4,277s
user	0m4,214s
sys	0m0,117s

Stop at level 1
CompileCommand: compileonly C.test* bool compileonly = true

real	0m4,104s
user	0m4,079s
sys	0m0,106s

Stop at level 2
CompileCommand: compileonly C.test* bool compileonly = true

real	0m4,308s
user	0m4,239s
sys	0m0,145s

Stop at level 3
CompileCommand: compileonly C.test* bool compileonly = true

real	0m4,304s
user	0m4,247s
sys	0m0,132s

Default (Stop at level 4)
CompileCommand: compileonly C.test* bool compileonly = true

real	0m4,086s
user	0m4,059s
sys	0m0,122s

I've run some tests (up to tier10), it seems all fine, ignoring the usual noise. I've checked with @dougxc, it shouldn't impact Graal as it doesn't use OptoRuntime.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8353638: C2: deoptimization and re-execution cycle with StringBuilder (Bug - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25149/head:pull/25149
$ git checkout pull/25149

Update a local copy of the PR:
$ git checkout pull/25149
$ git pull https://git.openjdk.org/jdk.git pull/25149/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25149

View PR using the GUI difftool:
$ git pr show -t 25149

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25149.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@marc-chevalier This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8353638: C2: deoptimization and re-execution cycle with StringBuilder

Reviewed-by: thartmann, chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 148 new commits pushed to the master branch:

• a6ebcf6: 8354674: AArch64: Intrinsify Unsafe::setMemory
• a8d8ffa: 8356395: Spec needs to be clarified for InterfaceAddress class level API documentation and getBroadcast() method
• f6b0f01: 8332947: [macos] OpenURIHandler events not received when AWT is embedded in another toolkit
• ... and 145 more: https://git.openjdk.org/jdk/compare/ad07426fab3396caefd7c08d924e085c1f6f61ba...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@marc-chevalier The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[marc-chevalier]

/label add hotspot-compiler

[openjdk]

@marc-chevalier
The hotspot-compiler label was successfully added.

[mlbridge]

Webrevs

• 01: Full - Incremental (3adc9d4e)
• 00: Full (361a7727)

[dean-long]

This may be a dumb question, but why would we want to optimize throwing NegativeArraySizeException? Maybe I'm missing something, but I would be surprised if any real apps would be helped by this fix. I know we have some optimizations for exception throwing, but I would expect those optimizations to be most useful for unavoidable exceptions, like NoClassDefFoundError.

[marc-chevalier]

I agree it's a corner case and it should matter little in real life. Nevertheless, having worst performance in C2 than in C1 is never very nice looking. But to be honest, if the problem were more involved, and the fix more complicated, it's not clear to me whether it would be worth it just for that, indeed.

BUT! This fix should also speedup any exception that happens during allocation. There seem to be quite some THROW_* in instanceKlass.cpp, for instance. And this means also for NoClassDefFoundError!

OptoRuntime::new_instance_C ends with deoptimize_caller_frame so would benefit and we have the call chain: OptoRuntime::new_instance_C -> InstanceKlass::initialize -> InstanceKlass::initialize_impl which contains

THROW_MSG(vmSymbols::java_lang_NoClassDefFoundError(), ss.as_string());

[TobiHartmann]

I agree that such exceptional cases are usually not worth optimizing but in this case we already emitted all the code to handle them, so why deopt? The change looks good to me.

[dean-long]

This change looks good, but do we still need StressCompiledExceptionHandlers after this? The only other use is in JavaThread::handle_async_exception, and it looks like it should be change too because it's dealing with the same issue.

[marc-chevalier]

I've found a failing test (maybe even two) related to asynchronous exceptions when using this flag. I suspect something is wrong there (JDK-8356648). It's also not clear to me whether the failure is directly linked to faulty exceptions handlers: asynchronous exceptions have a complicated (to me) handshake mechanism and maybe it's faulty without the deopts (or deopts makes it less likely). Unlike in JDK-8004741 (that added StressCompiledExceptionHandlers), I'm not hitting the assert "missing exception handler" (that still exists). Aside from being guarded by the same flag, the relation is not clear to me.

For allocations, I couldn't find any problem, and the logic seems simpler. I think it's fine to still use the one that works.

[chhagedorn]

Looks reasonable to me.

[chhagedorn]

For consistency with other flag definitions:

Suggested change

	product(bool, DeoptimizeOnAllocationException, false, DIAGNOSTIC, \
	"Deoptimize on exception during allocation instead of using the" \
	" compiled exception handlers") \
	product(bool, DeoptimizeOnAllocationException, false, DIAGNOSTIC, \
	"Deoptimize on exception during allocation instead of using the " \
	"compiled exception handlers") \

[marc-chevalier]

Indeed, it was a bit ugly. I think I have my spaces in row now.

[marc-chevalier]

I'm ready for new review (hopefully approval): since last review, I just fixed the spaces as @chhagedorn pointed out.

[marc-chevalier]

/integrate

And thanks again @TobiHartmann and @chhagedorn!

[openjdk]

Going to push as commit a0a3060.
Since your change was applied there have been 148 commits pushed to the master branch:

• a6ebcf6: 8354674: AArch64: Intrinsify Unsafe::setMemory
• a8d8ffa: 8356395: Spec needs to be clarified for InterfaceAddress class level API documentation and getBroadcast() method
• f6b0f01: 8332947: [macos] OpenURIHandler events not received when AWT is embedded in another toolkit
• ... and 145 more: https://git.openjdk.org/jdk/compare/ad07426fab3396caefd7c08d924e085c1f6f61ba...master

Your commit was automatically rebased without conflicts.

[openjdk]

@marc-chevalier Pushed as commit a0a3060.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.