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

Implemented very simple caching to avoid unnecessary database queries #195

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brocktimus
Copy link
Contributor

I had some code which was calling #parent a lot. Rails :belongs_to caches this to avoid hitting the database unnecessarily. After migrating to ancestry I noticed it was much slower because every call to #parent resulted in the database being hit.

This is obviously an incredibly naive / trivial cache and doesn't bother to even try to cache the more complex methods.

We could make it an opt in argument passed to ::has_ancestry, but that seems really conservative.

Any idea how to test this? Can we assert on number of queries run? Otherwise I'll write a test now asserting on the object equality, potentially through #object_id.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 619ef0a on brocktimus:instance_method_optimizations into 46449bc on stefankroes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling c85c702 on brocktimus:instance_method_optimizations into 46449bc on stefankroes:master.

@vanderhoorn
Copy link
Contributor

Not necessarily against project styles, but in general humans tend be better at seeing rather than reading.

If you see {} you know it is a new (empty) hash, but with Hash.new you would have to read to know it is a new hash.

:)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling e25e7c0 on brocktimus:instance_method_optimizations into da7420f on stefankroes:master.

@kbrock
Copy link
Collaborator

kbrock commented Aug 27, 2018

I think this looks neat - but it makes me nervous.
I've seen a number of entity caching layers over the years for rails. They seem like such a good idea until they don't. Rails itself ended up going the caching sql route because they couldn't make it work.

I wonder if there is a way to optionally add this, as I'm leaning away from merging this.
But not against modifying the code to make this extension necessary.

@kbrock
Copy link
Collaborator

kbrock commented Sep 23, 2019

Rails has a standard mechanism for storing relationships in an object. This is cleared out when calling a reload!. Seems we should use the rails cache to store the values.

Also, with the introduction of rails 6.0, children.create no longer works without deprecations.
So I'm thinking we need to rework the way children is setup.

If we can turn it into a relationship, we gain a lot of great stuff. (like caching and eager includes)

@kbrock
Copy link
Collaborator

kbrock commented Oct 16, 2020

I'm currently trying to convert parent_id and ancestors as standard rails columns. I'm hoping that will lead us to being able to use standard associations (or slightly modified ones)

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