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

Identify item by key object instead of key string representation #27

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

DouweM
Copy link
Contributor

@DouweM DouweM commented Nov 23, 2018

Imagine I have the following code:

class File < ActiveRecord::Base
  belongs_to :project

  def self.lazy(project, file_name)
    BatchLoader.for(file_name).batch(key: project) do |file_names, loader, args|
      args[:key].files.where(name: file_names).each do |file|
        loader.call(file.name, file)
      end
    end
  end
end

project = Project.first
same_project = Project.find(project.id)

file_a = File.lazy(project, "file_a")
file_b = File.lazy(same_project, "file_b")

Right now, calling file_a.name followed by file_b.name will result in two queries being executed, because the string representations of project and same_project are different (they contain the actual Ruby object_id), which means the block_hash_keys will be different as well.

With the change in this PR, calling file_a.name followed by file_b.name will result in only one batch query being executed, since project and same_project have the same #hash and implement #eql? to regard each other as identical, which means the block_hash_keys will be identical for hash keying purposes.

In this specific example, I could also have used key: project.id and called File.where(project_id: args[:key], name: file_names) instead of args[:key].files.where(name: file_names), but that is not an option in some of the batch loaders in the GitLab code base.

In general, I think it makes sense for BatchLoader to follow the same equality rules here that Ruby usually would if the user has specifically overwritten #hash and #eql?.

@@ -9,7 +9,7 @@ class ExecutorProxy
def initialize(default_value, key, &block)
@default_value = default_value
@block = block
@block_hash_key = "#{key}#{block.source_location}"
@block_hash_key = [block.source_location, key]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to reverse key and block.source_location, since key is used to subcategorize.

@coveralls
Copy link

coveralls commented Nov 23, 2018

Coverage Status

Coverage increased (+0.03%) to 99.759% when pulling b8e2af3 on DouweM:patch-1 into 1f0048b on exAspArk:master.

@DouweM
Copy link
Contributor Author

DouweM commented Nov 29, 2018

@exAspArk Please have a look at this contribution!

@exAspArk
Copy link
Owner

@DouweM hey, thank you! I'll take a look tomorrow.

@exAspArk
Copy link
Owner

@DouweM LGTM! Just one thing, could you please write a test which uses instance objects as a key?

@DouweM
Copy link
Contributor Author

DouweM commented Dec 3, 2018

@exAspArk Test added!

@exAspArk exAspArk merged commit 90eacbe into exAspArk:master Dec 3, 2018
@exAspArk
Copy link
Owner

exAspArk commented Dec 3, 2018

@DouweM awesome, thanks a lot for your contribution! 🙌

I released your changes in 1.2.2.

tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Dec 3, 2018
Include's Douwe's performance enhancement to identify item by key object
instead of key string representation:

exAspArk/batch-loader#27
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Dec 4, 2018
Include's Douwe's performance enhancement to identify item by key object
instead of key string representation:

exAspArk/batch-loader#27
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.

3 participants