search

solnic / charlatan

Neat delegation for ruby objects
MIT License
69 stars 3 forks source link

define method when hitting method missing #1

Closed gregory closed 10 years ago

gregory commented 10 years ago

http://www.jroller.com/dscataglini/entry/speeding_up_method_missing

gregory commented 10 years ago

@solnic looks like the build is failing for some reason, there is a segfault. What do you think about the PR?

mbj commented 10 years ago

@gregory 2.1.0 is simply a buggy MRI release. Do not worry about the segfault, its IMHO not your fault.

mbj commented 10 years ago

@gregory For reference we have similar crashes in most of the ROM gems that hit a specific VM state when metaprogramming. This is fixed in latest ruby-head.

gregory commented 10 years ago

cool! How you like the commit @mbj ?

mbj commented 10 years ago

@gregory I dont like runtime metaprogramming like these. I love code that "finishes" its metaprogramming and than begins to handle user input.

I know it will fix a performance problem.

But I'd like a more explicit approach in general, not a catch all like method_missing. My opinion is diametric to @solnic here.

One thing code wise: I'd probably break the implementation up into two methods, the #method_missing method body is IMHO to huge.

gregory commented 10 years ago

agree for the code wise, didn't want to refactor before it has been approved. For the method_missing, i'm not sure we have a lot of options to solve the delegation issue. It's a matter of balance.... magic always comes with a price :)

dkubb commented 10 years ago

Is this change in response to code that was profiled and #method_missing was found to be the bottleneck? Was this change tested to see if it actually resolved the specific bottleneck?

If this was "just in case" kind of change, I would probably not support it. I would much rather want to see a concrete example in real code where this solved a performance problem.

Also, this kind of trick may perform well on microbenchmarks, and may make code in a very narrow scope perform better, it also blows away the global method cache for the entire process. It's often the case where an individual method will perform better while at the same time slowing down everything else. Granted, given enough time all the common methods will be cached so no more methods will be dynamically added at runtime, but until that time it often causes lots of thrashing.

solnic commented 10 years ago

My reply would be inline with @mbj and @dkubb so I don't have any further comments. I suspect we could and maybe even should generate the methods but not "on demand" but up-front by inspecting the object which includes a charlatan module. We will see, for now let's just not worry about this. It can be easily added later on.