Skip to content
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

define method when hitting method missing #1

Closed
wants to merge 1 commit into from

Conversation

gregory
Copy link

@gregory gregory commented Feb 12, 2014

No description provided.

@gregory
Copy link
Author

gregory commented Feb 12, 2014

@gregory
Copy link
Author

gregory commented Feb 14, 2014

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

@mbj
Copy link

mbj commented Feb 14, 2014

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

@mbj
Copy link

mbj commented Feb 14, 2014

@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
Copy link
Author

gregory commented Feb 14, 2014

cool! How you like the commit @mbj ?

@mbj
Copy link

mbj commented Feb 14, 2014

@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
Copy link
Author

gregory commented Feb 14, 2014

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
Copy link

dkubb commented Feb 14, 2014

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
Copy link
Owner

solnic commented Feb 15, 2014

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.

@solnic solnic closed this Feb 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants