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

Memoize Database Field Names For Faster Fields Access #5809

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bomberby
Copy link

Background:
While working on improving performance on our app, I've noticed that the database_field_name is consistently taking long than expected when calling the same field repeatedly(very common when iterating over tens of thousands of objects or in inner loops)
While debugging I've noticed that Mongoid::Fields#database_field_name is taking a significant time to perform.

Comparison(very naive):

1000000.times do
  product.price
end

Before this change takes roughly 1.55 seconds, after it would take 0.99 seconds.
In more realistic cases such as validations of large number of objects as part of factories I've seen 15-20% differences

This code should be able to accept aliases and non-aliases and preserve the mapping, and save time on repeated access.
It is very possible I've missed some cache busters, please help me identify cases like that, as well as confirm my performance estimations, or if you can help with an even faster solution.

@@ -561,6 +562,7 @@ def add_defaults(field)
#
# @api private
def add_field(name, options = {})
@database_field_names[name] = nil if @database_field_names
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what the purpose of this line is; is this resetting some previously set field? If it's just for initialization, I'd prefer to leave it out, since the field would be nil implicitly, by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's meant as some sort of cache busting.
This is in case something unexpected like adding a field happens after we have accessed that field, for example, we have accessed field abc earlier as an alias, but later in the lifecycle, somehow that field added as an actual field.
I'm not certain about the exact flow, but perhaps we should also revoke the value for any alias that field might have.

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.

2 participants