-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Detect cold CallTarget invalidation and reset its profile #11610
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
Conversation
...racle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotOptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...racle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotOptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...m.oracle.truffle.runtime/src/com/oracle/truffle/runtime/OptimizedTruffleRuntimeListener.java
Show resolved
Hide resolved
truffle/src/com.oracle.truffle.runtime/src/com/oracle/truffle/runtime/OptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
truffle/src/com.oracle.truffle.runtime/src/com/oracle/truffle/runtime/OptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...src/jdk.graal.compiler.test/src/jdk/graal/compiler/truffle/test/MaximumCompilationsTest.java
Outdated
Show resolved
Hide resolved
logic for resetting call target profile.
@chumer, I did a refactoring of the code following your suggestions. Basically, I changed two things:
- I dropped the changes related to resetting number of compilations based on time. The approach now is to reset
successfullRecompilationCountwhen the method was invalidated because it was cold. - I reverted the changes in
HotspotOptimizedCallTargetto detect that the method was invalidated. I implemented the idea that you suggested: verifying the invalidation reason of the call target when it's about to be recompiled.
I tested the latest changes using the approach described in the issue ticket and everything worked fine. mx unittest and mx gate also didn't raise any issue.
Gentle ping. @chumer - Please, take a look when you can.
...racle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotOptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...racle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotOptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...racle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotOptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...ruffle.runtime/src/com/oracle/truffle/runtime/OptimizedTruffleRuntimeListenerDispatcher.java
Outdated
Show resolved
Hide resolved
...racle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotOptimizedCallTarget.java
Outdated
Show resolved
Hide resolved
...e/src/com.oracle.truffle.runtime/src/com/oracle/truffle/runtime/OptimizedTruffleRuntime.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know that this is not racy with compilations installing code at the same time?
Or is this something we would accept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My view here is that no two threads will run this method concurrently, because the call only happens while compilationTask in the CT is != null. Also, my understanding is that even if for some reason the system ends up invalidating a "good" compilation because of a race condition: 1) for Truffle it shouldn't matter because it will at the same time be installing new code and 2) HotSpot will just drop the previous compilation from the CC because it will eventually be cold. Please, let me know if this doesn't make sense.
I moved this method to HotSpotOptimizedCallTarget::prepareForCompilation as you suggested in another comment.
@chumer - I believe I addressed all your comments. Please, take a look again when you can.
@jchalou please give this another pair of eyes. If its fine feel free to integrate.
@JohnTortugo any chance we could have a smoke test for this behavior? That is reasonably complex? Would be great to be notified if this breaks.
Closes: #11045
Please review this patch that modifies Truffle to:
Reset a CallTarget's profile when the
nmethodassociated with it was invalidated by HotSpot because the Code Cache's method flushing heuristics deemed the nmethod to be cold.Limit the number of compilations that a CallTarget can have within a time period. This doesn't change the current behavior, instead it just makes the constraint more flexible. I added a new parameter,
MaximumCompilationsWindow, that the user can set to specify a time window within which the number of compilations will be limited (currently the window is whole duration of application execution).Tests: