Prevent JIT bodies from strictly outliving methods inlined into them #7673
+1,127
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdmpapin
requested review from
vijaysun-omr,
dsouzai,
0xdaryl and
mstoodle
as code owners
jdmpapin
force-pushed
the
retained-method-set
branch
from
834b7c0 to
7f057e8
Compare
|
Updated to add a node flag |
jdmpapin
force-pushed
the
retained-method-set
branch
from
7f057e8 to
464d3de
Compare
|
Rebased onto a base commit that's compatible with the new base commit for eclipse-openj9/openj9#21216 |
|
Pushed updates in a separate commit. I intend to squash them later. These go along with the updates that have just been pushed to eclipse-openj9/openj9#21216. |
This is largely in preparation for constant references, but it should also have benefits for code motion. For more detail, see the Motivation section of the doc comment for RetainedMethodSet. Much of this commit is OMR::RetainedMethodSet, which describes a set of methods that will remain loaded under certain assumptions. This is a base class that is meant to be subclassed in any downstream project that allows methods to be dynamically unloaded. During inlining, each TR_CallSite and TR_CallTarget will now have an associated RetainedMethodSet. Often, all methods named at call sites and all selected call targets can be established to outlive their callers, and transitively the outermost method. In this case, nothing of note happens. However, sometimes a call site is refined based on a known object, and the refined method is not already known to outlive its caller. In this case, a nested RetainedMethodSet is created to represent the refined method (in addition to the surrounding inline context). Any further nested call sites and call targets can therefore take this refined method for granted. If the call site is actually inlined, then we record a "keepalive" for the refined method. The idea of a keepalive is that the JIT compiler should create an additional reference to prevent that method from being unloaded as long as the JIT body is still valid. In this commit, no such reference is created because we do not yet have a mechanism to do so. They will be implemented later as extra constant references. In the meantime, the JIT will simply assume that anything covered by a keepalive will remain loaded on its own. This assumption isn't great, but it's essentially the same as our existing pervasive assumption that known object indices will remain accurate. Similarly, sometimes a call target is selected based on profiling or a single implementer, and the target method is not already known to outlive the base method named at the call site. In this case, a nested RetainedMethodSet is created to represent the target method (in addition to the surrounding inline context). Any further nested call sites and call targets can therefore take this target method for granted. If the call target is actually inlined, then we record a "bond" for the target method. The idea of a bond is to limit the lifetime of the JIT body to prevent it from strictly outliving the inlined method. This should be accomplished in a downstream project by creating an unload assumption so that when the inlined method is unloaded, the JIT body will be invalidated (similarly to preexistence). If the dontInlineUnloadableMethods option is set, then instead of requesting bonds, inliner will refuse to inline targets that would required them. In this case, assertions check after optimization that no inlined method will be unexpectedly unloaded.
- Move to a single set of keepalives and a single set of bonds per tree. This way, keepalives and bonds don't have to bubble up toward the root one level at a time, which makes it easier to make call-site attestations non-destructive. It also means that sets that should generate keepalives and bonds are now allowed to do so in any order. - Replace attestLinkedCallee...() with withLinkedCalleeAttested(). It now creates a child set if needed instead of mutating the receiver. This way, while building the tree for inlining, the set at a given point has contents based on the call path leading to it, independent of any other call sites or call targets. - Collect information pertaining to the retained methods analysis for each call site during inlining. In particular, remember whether the call site was refined, and if so, what the refined method was, and also remember which call sites generated keepalives and bonds. This information will be needed in case the analysis is to be repeated later (e.g. for OpenJ9 JITServer). - Call keepalive/bond in preorder instead of in postorder since it's now possible to do so. - Assert that no bonds are created with dontInlineUnloadableMethods set. - Add a doc comment for withKeepalivesAttested(). - Add withBondsAttested() similar to withKeepalivesAttested(). It's not currently used, but it will be used later for const refs. - Allow the per-tree set of keepalives and bonds to be extended by a downstream project. This will make it possible to remember the contents of the receivers of the keepalive() and bond() calls, which will allow for withKeepalivesAttested() and withBondsAttested() to be done without needing to scan. - When tracing the bond methods, add a project-determined note. This will be used by JITServer to warn that the set of bonds is imprecise and that the real set will be determined by the client later on. - When checking the inlining table for dontInlineUnloadableMethods, don't bother calling withKeepalivesAttested() unless there are actually some inlined sites, and trace a blank line at the end. - Fix a namespace end comment. - Remove the assertion that was added to the TR_CallTarget constructor because an equivalent one has been added by an earlier inliner change.
jdmpapin
force-pushed
the
retained-method-set
branch
from
a404ec3 to
b1f748e
Compare
|
Rebased onto a base commit that's compatible with the new base commit for eclipse-openj9/openj9#21216. Also corrected a slight omission in the "Update |
|
Draft again because of eclipse-openj9/openj9#21216 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is largely in preparation for constant references, but it should also have benefits for code motion. For more detail, see the Motivation section of the doc comment for
RetainedMethodSet.Much of this commit is
OMR::RetainedMethodSet, which describes a set of methods that will remain loaded under certain assumptions. This is a base class that is meant to be subclassed in any downstream project that allows methods to be dynamically unloaded.During inlining, each
TR_CallSiteandTR_CallTargetwill now have an associatedRetainedMethodSet. Often, all methods named at call sites and all selected call targets can be established to outlive their callers, and transitively the outermost method. In this case, nothing of note happens.However, sometimes a call site is refined based on a known object, and the refined method is not already known to outlive its caller. In this case, a nested
RetainedMethodSetis created to represent the refined method (in addition to the surrounding inline context). Any further nested call sites and call targets can therefore take this refined method for granted. If the call site is actually inlined, then we record a "keepalive" for the refined method. The idea of a keepalive is that the JIT compiler should create an additional reference to prevent that method from being unloaded as long as the JIT body is still valid. In this commit, no such reference is created because we do not yet have a mechanism to do so. They will be implemented later as extra constant references. In the meantime, the JIT will simply assume that anything covered by a keepalive will remain loaded on its own. This assumption isn't great, but it's essentially the same as our existing pervasive assumption that known object indices will remain accurate.Similarly, sometimes a call target is selected based on profiling or a single implementer, and the target method is not already known to outlive the base method named at the call site. In this case, a nested
RetainedMethodSetis created to represent the target method (in addition to the surrounding inline context). Any further nested call sites and call targets can therefore take this target method for granted. If the call target is actually inlined, then we record a "bond" for the target method. The idea of a bond is to limit the lifetime of the JIT body to prevent it from strictly outliving the inlined method. This should be accomplished in a downstream project by creating an unload assumption so that when the inlined method is unloaded, the JIT body will be invalidated (similarly to preexistence).If the
dontInlineUnloadableMethodsoption is set, then instead of requesting bonds, inliner will refuse to inline targets that would required them. In this case, assertions check after optimization that no inlined method will be unexpectedly unloaded.