Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Simplify some of the block-handling complexity with instance_exec() #421

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

Currently a very unusual trick (https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L1186-1191) is used to ensure some blocks execute in the correct context (I assume this is a hold-over from 1.8.6 support where instance_exec() was not available.)

Now that Sinatra no longer supports 1.8.6 we can maybe switch from the define_method / remove_method approach and use instance_exec() instead.

Lambda-like arity semantics are maintained by wrapping the block in a proc that checks arity. As in the previous situation an exception is made for arity 0 blocks, where normal block semantics hold.

Replaced define_method/remove_method (in generate_method) with instan…
…ce_exec() to simplify code.

Now that 1.8.6 is no longer supported we no longer need the define_method/remove_method trick and can make
use of Ruby's own mechanism for this purpose - instance_exec(). Lambda-like arity semantics are maintained
by wrapping the block in a proc that checks arity. As in the previous situation an exception is made for arity 0 blocks,
where normal block semantics hold.
Owner

rkh commented Nov 30, 2011

instance_exec is slower (rebinding on every call rather than once) and results in uglier stack traces

@rkh, the generate_method approach uses UnboundMethod objects, which need to be rebound and called each time too, and in my benchmarks the instance_exec approach is significantly faster http://pastie.org/2944070.

The code can be further simplified by getting rid of the arity check (wrap_with_arity_check()). I only added this method because you include tests for arity.

Owner

rkh commented Dec 1, 2011

In the benchmarks I did for 1.3.0, the unbound method approach is way faster. The issue with your benchmark is, that you always use the same instance: https://gist.github.com/1415044

banister commented Dec 1, 2011

@rkh, interesting. The backtrace argument is also a good one.

I didn't investigate too deeply, I just had this issue filed on Pry, saw your generate_method code and thought 'this is a good place for an instance_exec refactor' .

Feel free to close this pull request :)

Owner

rkh commented Dec 1, 2011

You pull request makes sense, though, and in fact, this is a relict from when we used to support 1.8.6, but for the above reasons I'm actually in favor of keeping it that way.

What was that benchmarking thing you used there?

@rkh rkh closed this Dec 1, 2011

banister commented Dec 1, 2011

Not my benchmarks, actually another programmer did those benchmarks and im not sure what tool it was :/

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