Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use @TruffleBoundary instead of transferToInterpreter() #12375

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 27, 2025

Pull Request Description

Attempt to fix #12374 by using @TruffleBoundary instead of CompilerDirectives.transferToInterpreter().

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 27, 2025
@JaroslavTulach JaroslavTulach self-assigned this Feb 27, 2025
@JaroslavTulach
Copy link
Member Author

Looks like the issue got fixed:

obrazek

many Column_Numeric benchmarks are back where they were before #12295

@Akirathan (and @steve-s), you are right! Never use transferToInterpreter()!

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Feb 27, 2025
@Akirathan
Copy link
Member

Akirathan commented Feb 27, 2025

@Akirathan (and @steve-s), you are right! Never use transferToInterpreter()!

When I was working on https://github.com/oracle/fastr with @steve-s, the rule of a thumb was: "If you ever feel like using transferToInterpreter, don't. And use transferToInterpreterAndInvalidate instead". I wish I would remember the reasoning. @steve-s, once you have some time, we would love to hear a brief explanation.

GitHub
A high-performance implementation of the R programming language, built on GraalVM. - oracle/fastr

@mergify mergify bot merged commit a11e071 into develop Feb 27, 2025
73 of 76 checks passed
@mergify mergify bot deleted the wip/jtulach/NoTransferToInterpreter12374 branch February 27, 2025 10:21
@steve-s
Copy link

steve-s commented Feb 27, 2025

My understanding is that transferToInterpreter() is like a deopt. It must reconstruct the state (pull values from registers, etc.) so that the method can continue running in the interpreter. @TruffleBoundary is regular call with the drawbacks of calls: kills compiler assumptions, caller needs to save registers, etc.

@JaroslavTulach
Copy link
Member Author

My understanding is that transferToInterpreter() is like a deopt. It must reconstruct the state (pull values from registers, etc.) so that the method can continue running in the interpreter.

E.g. it really transfers to JVM interpreter. The name is clear, but for some reason I started to use it in situations when I just wanted to escape PE - and that is wrong.

@TruffleBoundary is regular call with the drawbacks of calls: kills compiler assumptions, caller needs to save registers, etc.

To escape PE - @TruffleBoundary is (the only) way to go. We'll review our transferInterpreter usages and (probably) replace them with @TruffleBoundary method call.

Thank you, Štěpáne.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stdlibs benchmark regressions 2025-02-24
5 participants