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

Add replace_methods argument to BatchLoader#batch #45

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

smcgivern
Copy link
Contributor

The existing cache argument does two things:

  1. #__sync uses it to set a @synced instance variable, which tells
    later calls to #__sync not to reload the value when another method is
    called on it.
  2. #__sync! uses it to replace methods on the proxied object with
    their 'real' equivalents.

This delegates responsibility for item 2 to the new replace_methods
argument instead. In testing (and in the benchmarks added here) we've
seen that #__replace_with! can actually take up an appreciable amount
of time, and removing it is faster, even though each individual method
call is slower.

The default for this new argument is to match cache. Only if it's
explicitly set to true or false will it not do that, and if it is
set to true and cache is false we raise an error, because that
doesn't make sense.

Closes #43.

lib/batch_loader.rb Outdated Show resolved Hide resolved
@smcgivern
Copy link
Contributor Author

@exAspArk would you mind taking a look, please?

README.md Show resolved Hide resolved
spec/benchmarks/caching.rb Show resolved Hide resolved
lib/batch_loader.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/batch_loader.rb Outdated Show resolved Hide resolved
@smcgivern smcgivern force-pushed the add-replace-methods-argument branch from da28873 to ffc1abe Compare April 25, 2019 08:11
@smcgivern
Copy link
Contributor Author

Thanks @exAspArk, I made a few changes and added questions for the others.

The existing `cache` argument does two things:

1. `#__sync` uses it to set a `@synced` instance variable, which tells
   later calls to `#__sync` not to reload the value when another method is
   called on it.
2. `#__sync!` uses it to replace methods on the proxied object with
   their 'real' equivalents.

This delegates responsibility for item 2 to the new `replace_methods`
argument instead. In testing (and in the benchmarks added here) we've
seen that `#__replace_with!` can actually take up an appreciable amount
of time, and removing it is faster, even though each individual method
call is slower.

The default for this new argument is to `true`. It's very likely that
`cache: false, replace_methods: true` will give surprising results.
@smcgivern smcgivern force-pushed the add-replace-methods-argument branch from ffc1abe to e2f4b67 Compare April 26, 2019 08:54
@exAspArk exAspArk merged commit 218379a into exAspArk:master Apr 29, 2019
@exAspArk
Copy link
Owner

@smcgivern thanks a lot! 🙌

@exAspArk
Copy link
Owner

I released your changes in v1.4.0 and added back your code (without raise) to avoid breaking changes 😸

@replace_methods = replace_methods.nil? ? cache : replace_methods

@smcgivern
Copy link
Contributor Author

Thanks @exAspArk! ❤️

I like this explanation:

You may consider avoiding the "up front payment" and "pay as you go" with #method_missing by disabling the method replacement:

I'll look at getting this into GitLab this week.

tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Apr 30, 2019
We've seen a significant performance penalty when using
`BatchLoader#__replace_with!`. This defines methods on the batch loader
that proxy to the 'real' object using send. The alternative is
`method_missing`, which is slower.  However, we've noticed that
`method_missing` can be faster if:

1. The objects being loaded have a large interface.
2. We don't call too many methods on the loaded object.

Avatar uploads meet both criteria above, so let's use the newly-released
feature in exAspArk/batch-loader#45.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60903
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Apr 30, 2019
We've seen a significant performance penalty when using
`BatchLoader#__replace_with!`. This defines methods on the batch loader
that proxy to the 'real' object using send. The alternative is
`method_missing`, which is slower.  However, we've noticed that
`method_missing` can be faster if:

1. The objects being loaded have a large interface.
2. We don't call too many methods on the loaded object.

Avatar uploads meet both criteria above, so let's use the newly-released
feature in exAspArk/batch-loader#45.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60903
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jun 13, 2019
In production, we've seen the rendering times of the merge request
widget increase as a result of loading commit data. BatchLoader attempts
to call replace_methods on the lazy object, but this has a significant
performance penalty for modules that have many methods. Disabling this
mode (exAspArk/batch-loader#45) appears to cut
load times by about 50% for MergeRequestsController#show.

Relates to https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/6941
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jun 13, 2019
In production, we've seen the rendering times of the merge request
widget increase as a result of loading commit data. BatchLoader attempts
to call replace_methods on the lazy object, but this has a significant
performance penalty for modules that have many methods. Disabling this
mode (exAspArk/batch-loader#45) appears to cut
load times by about 50% for MergeRequestsController#show.

Relates to https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/6941
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jun 13, 2019
We've seen a significant performance penalty when using
`BatchLoader#__replace_with!`. This defines methods on the batch loader
that proxy to the 'real' object using send. The alternative is
`method_missing`, which is slower.  However, we've noticed that
`method_missing` can be faster if:

1. The objects being loaded have a large interface.
2. We don't call too many methods on the loaded object.

In production, we've seen the rendering times of the merge request
widget increase as a result of loading commit data. BatchLoader attempts
to call replace_methods on the lazy object, but this has a significant
performance penalty. Disabling this mode
(exAspArk/batch-loader#45) appears to cut load
times by about 50% for MergeRequestsController#show.

Relates to https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/6941
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jun 13, 2019
We've seen a significant performance penalty when using
`BatchLoader#__replace_with!`. This defines methods on the batch loader
that proxy to the 'real' object using send. The alternative is
`method_missing`, which is slower.  However, we've noticed that
`method_missing` can be faster if:

1. The objects being loaded have a large interface.
2. We don't call too many methods on the loaded object.

In production, we've seen the rendering times of the merge request
widget increase as a result of loading commit data. BatchLoader attempts
to call replace_methods on the lazy object, but this has a significant
performance penalty. Disabling this mode
(exAspArk/batch-loader#45) appears to cut load
times by about 50% for MergeRequestsController#show.

Relates to https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/6941
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.

Split cache keyword from method replacement
2 participants