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

ZSuper does not require caller's binding/scope. #3909

Merged
merged 1 commit into from May 26, 2016

Conversation

Projects
None yet
2 participants
@headius
Member

headius commented May 20, 2016

This appears to pass all tests, and I could find no path from ZSuper's interpret method to anything requiring DynamicScope.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss May 21, 2016

Contributor

http://dev.jruby.codehaus.narkive.com/VK4b7dEd/jruby-dev-eliminating-block-as-binding-as-a-potential-deoptimizer and your comment there is how CAN_CAPTURE_CALLERS_BINDING flag was set up in ZSuper. I had set it way back in the day and it just propagated forward through all the code refactoring.

As for USES_ZSUPER and requiring dynscope, I think ec25a24 is where the link probably got severed. Till that time the dynscope nesting was walked up to determine call args.

Contributor

subbuss commented May 21, 2016

http://dev.jruby.codehaus.narkive.com/VK4b7dEd/jruby-dev-eliminating-block-as-binding-as-a-potential-deoptimizer and your comment there is how CAN_CAPTURE_CALLERS_BINDING flag was set up in ZSuper. I had set it way back in the day and it just propagated forward through all the code refactoring.

As for USES_ZSUPER and requiring dynscope, I think ec25a24 is where the link probably got severed. Till that time the dynscope nesting was walked up to determine call args.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss May 21, 2016

Contributor

That said, f67e618 made additional changes and added https://github.com/jruby/jruby/blob/915993886ae6663586d893b7d237f750068aceb5/core/src/main/java/org/jruby/ir/instructions/ArgScopeDepthInstr.java which uses

@JIT
public static RubyFixnum getArgScopeDepth(ThreadContext context, StaticScope currScope) {
int i = 0;
while (!currScope.isArgumentScope()) {
currScope = currScope.getEnclosingScope();
i++;
}
return context.runtime.newFixnum(i);
}
.... Worth checking that none of those depend on the presence of a dynamic scope.

Contributor

subbuss commented May 21, 2016

That said, f67e618 made additional changes and added https://github.com/jruby/jruby/blob/915993886ae6663586d893b7d237f750068aceb5/core/src/main/java/org/jruby/ir/instructions/ArgScopeDepthInstr.java which uses

@JIT
public static RubyFixnum getArgScopeDepth(ThreadContext context, StaticScope currScope) {
int i = 0;
while (!currScope.isArgumentScope()) {
currScope = currScope.getEnclosingScope();
i++;
}
return context.runtime.newFixnum(i);
}
.... Worth checking that none of those depend on the presence of a dynamic scope.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss May 21, 2016

Contributor

Additionally, we have a more consistent semantics for define_method and zsuper whereas MRI has more restricted semantics. So, worth also checking if implementing those restricted semantics of MRI changes any of this. /cc @enebo

Contributor

subbuss commented May 21, 2016

Additionally, we have a more consistent semantics for define_method and zsuper whereas MRI has more restricted semantics. So, worth also checking if implementing those restricted semantics of MRI changes any of this. /cc @enebo

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 25, 2016

Member

@subbuss I think the use of DynamicScope in the old logic was more because we had no easy way to get at the current scope's StaticScope. If we didn't actually need the DynamicScope, we'd still push a dummy scope. With these paths modified in this PR to just use the current StaticScope directly, I think we're ok.

What semantics of MRI are you referring to as "restricted semantics"? define_method obviously has more need for bindings and scopes, but I still don't think zsuper needs anything beyond StaticScope.

Member

headius commented May 25, 2016

@subbuss I think the use of DynamicScope in the old logic was more because we had no easy way to get at the current scope's StaticScope. If we didn't actually need the DynamicScope, we'd still push a dummy scope. With these paths modified in this PR to just use the current StaticScope directly, I think we're ok.

What semantics of MRI are you referring to as "restricted semantics"? define_method obviously has more need for bindings and scopes, but I still don't think zsuper needs anything beyond StaticScope.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss May 25, 2016

Contributor

Okay, I'll trust your and @enebo 's judgment on this since it has been a while and I don't remember all the details .. but, worth checking if the flags set for ArgScopeDepthInstr are correct.

Contributor

subbuss commented May 25, 2016

Okay, I'll trust your and @enebo 's judgment on this since it has been a while and I don't remember all the details .. but, worth checking if the flags set for ArgScopeDepthInstr are correct.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 26, 2016

Member

@subbuss ArgScopeDepthInstr appears to only use the StaticScope, and sets no flags. I think that's correct...for zsuper we really just need to walk up to the proper "method" scope so we can see what the arg structure was.

Member

headius commented May 26, 2016

@subbuss ArgScopeDepthInstr appears to only use the StaticScope, and sets no flags. I think that's correct...for zsuper we really just need to walk up to the proper "method" scope so we can see what the arg structure was.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 26, 2016

Member

The one failure is known to fail sporadically, and that will hopefully be improved by #3929. Proceeding with merge.

Member

headius commented May 26, 2016

The one failure is known to fail sporadically, and that will hopefully be improved by #3929. Proceeding with merge.

@headius headius added this to the JRuby 9.1.3.0 milestone May 26, 2016

@headius headius merged commit 6d1062e into jruby:master May 26, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@headius headius deleted the headius:zsuper_sans_binding branch May 26, 2016

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