Multiple fixes for forced JIT and indy-based Java dispatch#7913
Merged
headius merged 8 commits intojruby:masterfrom Aug 30, 2023
Merged
Multiple fixes for forced JIT and indy-based Java dispatch#7913headius merged 8 commits intojruby:masterfrom
headius merged 8 commits intojruby:masterfrom
Conversation
Arguments passed to either instance or static method must match arity of target Java method. Not sure how the -1 got in here but it may have been thinking in terms of a MethodType, which will have the self object as the first argument for instance methods. Also not sure how this got past testing. More indy testing of JI calls must be done. Fixes jruby#7904
Missed test case related to jruby#7904
This moves the indy-enabled Java 8 spec:ji run to the "latest LTS" group (currently 21-ea) and adds additional flags there to force compilation. The latter change is needed in order for some types of tests to fully compile (like rspec specs, which usually have only blocks down to the root of the script). Both flags are required to force compilation of whole files (e.g. specs) or individual methods and blocks (if whole file fails or new methods are evaluated at runtime). This should help catch more indy issues.
headius
force-pushed
the
indy_ji_arity_mismatch
branch
from
3de6e8c to
b4fd261
Compare
The logic here would perform several checks to see if the Java method were bindable to the given call site. If all checks passed, a handle would be constructed and saved into the Java invoker. However, those same checks were not performed for future calls at different call sites, causing those sites to mismatch arity if they differed from the originating site. The fix here does two things to prevent the cached handle from being used when it doesn't match: * Finish all return value conversions so there are fewer unbindable cases and non after acquiring the direct handle. * Move the remaining checks above the use of the cached handle, so we can be assured that a handle would be bound to this site with or without the cache. Because there's only ever one NativeCall form and one cache entry, we know it will be the same signature and arity at this point, and can safely use the cached handle. In the future, when we improve this logic to support multiple overloads for a target Java method, combined with multiple incoming arities (and the other bail-out cases) this logic will need to cache handles on a per-shape basis (or accept not caching more than one shape).
In jruby#7589 I reduced the call site size for functional calls ("self" calls) by omitting the caller argument normally used to check visibility. This change was never applied to the Java invokedynamic binding logic, resulting in arity mismatches when a Java method is called from within an instance of itself or when calling a Java method through an alias, since the alias is rebound as a self call. This change specializes the number of arguments to drop from the beginning of the argument list based additionally upon whether it is a functional call site: * static and functional: drop 2 (context and self) * static and normal: drop 3 (context and caller and self) * instance and functional: drop 1 (context) * instance and normal: drop 2 (context and caller)
Caching the entire handle embeds call site-specific adaptations into it, making it unsuitable for other shapes of call site (e.g functional site without caller versus normal site with caller) and with no way to detect that situation. Instead, this change caches only the adaptations made to the original direct method handle, which will be the same for all consumers.
headius
changed the title
headius
changed the title
JIT-compiled methods do not populate ThreadContext file and line, so those will reflect whatever the last interpreted frame was.
headius
force-pushed
the
indy_ji_arity_mismatch
branch
from
0046bd5 to
4a7b6b5
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Several fixes here:
We could use a better system for doing these adaptations and caching them appropriately for the different call site shapes, but this should work reasonably well for now. If a call site is stable, we should not have to re-adapt the method handle too frequently (or ever again after the first time), and the heavier adaptations that apply to all call sites will be reused.
Fixes #7904