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

Separate varargs and specific-arity names in JIT. Fixes #4482 #4498

Merged
merged 2 commits into from Feb 22, 2017

Conversation

Projects
None yet
1 participant
@headius
Member

headius commented Feb 21, 2017

Originally, varargs and specific-arity paths were emitted in toto
as their own JVM method bodies. With recent changes, the varargs
path now calls the specific-arity path, causing an extra
synthetic entry in the JVM stack trace. This confounded the
backtrace miner, since it had no marker with which to omit the
synthetic version, and so Kernel#caller and friends contained an
additional invalid entry. This in turn caused require_relative to
fail to find the caller's path, resulting in a nil path value
passed to File.realpath, which triggered this bug.

It likely only affected invokedynamic because our indy logic must
be calling no-arg methods via the varargs path (a separate issue
to be fixed).

My modification here explicitly separates the synthetic varargs
name from the specific-arity name by adding a known varargs marker
that can be filtered out when mining the backtrace.

This is a big hacky; the state being managed by the jit is very
ad-hoc and prone to bugs. It will need some additional cleanup
after 9.1.8.0.

See #4482.

Separate varargs and specific-arity names in JIT. Fixes #4482
Originally, varargs and specific-arity paths were emitted in toto
as their own JVM method bodies. With recent changes, the varargs
path now calls the specific-arity path, causing an extra
synthetic entry in the JVM stack trace. This confounded the
backtrace miner, since it had no marker with which to omit the
synthetic version, and so Kernel#caller and friends contained an
additional invalid entry. This in turn caused require_relative to
fail to find the caller's path, resulting in a nil path value
passed to File.realpath, which triggered this bug.

It likely only affected invokedynamic because our indy logic must
be calling no-arg methods via the varargs path (a separate issue
to be fixed).

My modification here explicitly separates the synthetic varargs
name from the specific-arity name by adding a known varargs marker
that can be filtered out when mining the backtrace.

This is a big hacky; the state being managed by the jit is very
ad-hoc and prone to bugs. It will need some additional cleanup
after 9.1.8.0.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 21, 2017

Member

The additional "issue" I mentioned is a non-issue for now. Indy will always call through the varargs path the first time a call site is bound, since it only has a single varargs binding path in InvokeSite. Fixing the issue would require having separate bootstrap logic for each arity, and would only save us the varargs array and overhead on the first (re)bind of a given call site.

Since most indy sites will eventually settle into one or more direct variable OR specific paths, this is not necessary. However, we may want to fix it for sites that fail to bind, such as those that have too many targets or that are rebound too many times.

Member

headius commented Feb 21, 2017

The additional "issue" I mentioned is a non-issue for now. Indy will always call through the varargs path the first time a call site is bound, since it only has a single varargs binding path in InvokeSite. Fixing the issue would require having separate bootstrap logic for each arity, and would only save us the varargs array and overhead on the first (re)bind of a given call site.

Since most indy sites will eventually settle into one or more direct variable OR specific paths, this is not necessary. However, we may want to fix it for sites that fail to bind, such as those that have too many targets or that are rebound too many times.

Allow indirect indy calls to still call specific arity up to 3.
Previously, indirect indy calls would always box arguments, which
affects failed sites, Java sites, and others. This fixes #4499 by
setting up fail paths for arities up to 3 arguments, same as the
non-indy call paths.

@headius headius merged commit 5a21276 into jruby:master Feb 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@headius headius deleted the headius:isolate-arity-names branch Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment