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

Speed up init with a refactor #37

Closed
wants to merge 3 commits into from

Conversation

thomsbg
Copy link
Contributor

@thomsbg thomsbg commented Sep 4, 2012

Hopefully this makes for some clearer code inside the initialize method in Her::Model::ORM.

Instead of using writer_method_defined? inside the loop, make use of a use_setter_methods helper function which both calls setter methods and returns a hash of the remaining key / values.

I did this after noticing in profiling output that significant time was being spent in Class#instance_methods (which writer_method_defined? was using inside a loop).

@travisbot
Copy link

This pull request passes (merged f807c64 into e33489b).

@travisbot
Copy link

This pull request passes (merged e1f5782 into e33489b).

@wingrunr21
Copy link

I'd like to see this merged. We have some large objects being pulled in via JSON and it is proving to be expensive to build all of them. Profiling reveals writer_method_defined? is responsible for a good chunk of it.

@remi
Copy link
Owner

remi commented Nov 6, 2012

I merged the branch into master. Tests pass, but could you guys check if everything is still working in your applications? Thanks!

@thomsbg
Copy link
Contributor Author

thomsbg commented Nov 6, 2012

fwiw, I've been running this code in production for a month or so, no problems.

@wingrunr21
Copy link

My app is also looking solid

@remi
Copy link
Owner

remi commented Nov 6, 2012

fwiw, I've been running this code in production for a month or so, no problems.

Sweet. Just curious, can you tell me on what app/project you’re using Her? 😄

@thomsbg
Copy link
Contributor Author

thomsbg commented Nov 6, 2012

I work for Vox Media, where we're using Her to consume an internal API that powers the games data on Polygon, and the Products data on The Verge

@wingrunr21
Copy link

I work for CustomInk. We are moving off of ActiveResource for consuming data from our catalog management system.

@remi
Copy link
Owner

remi commented Nov 6, 2012

Wow, this is awesome. So glad to hear other people are using it for real 😀

@remi remi closed this Nov 6, 2012
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.

4 participants