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

PERF: Improve performance of where when using an array of values #39022

Merged
merged 1 commit into from
May 5, 2020

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Apr 23, 2020

This is a smaller alternative of performance improvement, without
refactoring type casting mechanism #39009.

This is relatively a smaller change (but about 40% faster than before),
so I think this could be easier reviewed without discuss about
refactoring type casting mechanism.

This just makes attribute.in(values) less allocation from an array of
casted nodes to one casted array node.

ids = (1..1000).each.map do |n|
  Post.create!.id
end

Benchmark.ips do |x|
  x.report("where with ids") do
    Post.where(id: ids).to_a
  end

  x.report("where with sanitize") do
    Post.where(ActiveRecord::Base.sanitize_sql(["id IN (?)", ids])).to_a
  end

  x.compare!
end

Before:

Warming up --------------------------------------
      where with ids     7.000  i/100ms
 where with sanitize    13.000  i/100ms

Calculating -------------------------------------
      where with ids     70.661  (± 5.7%) i/s -    357.000  in   5.072771s
 where with sanitize    130.993  (± 7.6%) i/s -    663.000  in   5.096085s

Comparison:
 where with sanitize:      131.0 i/s
      where with ids:       70.7 i/s - 1.85x  slower

After:

Warming up --------------------------------------
      where with ids    10.000  i/100ms
 where with sanitize    13.000  i/100ms

Calculating -------------------------------------
      where with ids     98.174  (± 7.1%) i/s -    490.000  in   5.012851s
 where with sanitize    132.289  (± 8.3%) i/s -    663.000  in   5.052728s

Comparison:
 where with sanitize:      132.3 i/s
      where with ids:       98.2 i/s - 1.35x  slower

cc @eileencodes @tenderlove @rafaelfranca

@kamipo kamipo force-pushed the perf_where_in branch 5 times, most recently from fa1377e to e36578e Compare April 28, 2020 23:51
This is a smaller alternative of performance improvement, without
refactoring type casting mechanism rails#39009.

This is relatively a smaller change (but about 40% faster than before),
so I think this could be easier reviewed without discuss about
refactoring type casting mechanism.

This just makes `attribute.in(values)` less allocation from an array of
casted nodes to one casted array node.

```ruby
ids = (1..1000).each.map do |n|
  Post.create!.id
end

Benchmark.ips do |x|
  x.report("where with ids") do
    Post.where(id: ids).to_a
  end

  x.report("where with sanitize") do
    Post.where(ActiveRecord::Base.sanitize_sql(["id IN (?)", ids])).to_a
  end

  x.compare!
end
```

Before:

```
Warming up --------------------------------------
      where with ids     7.000  i/100ms
 where with sanitize    13.000  i/100ms

Calculating -------------------------------------
      where with ids     70.661  (± 5.7%) i/s -    357.000  in   5.072771s
 where with sanitize    130.993  (± 7.6%) i/s -    663.000  in   5.096085s

Comparison:
 where with sanitize:      131.0 i/s
      where with ids:       70.7 i/s - 1.85x  slower
```

After:

```
Warming up --------------------------------------
      where with ids    10.000  i/100ms
 where with sanitize    13.000  i/100ms

Calculating -------------------------------------
      where with ids     98.174  (± 7.1%) i/s -    490.000  in   5.012851s
 where with sanitize    132.289  (± 8.3%) i/s -    663.000  in   5.052728s

Comparison:
 where with sanitize:      132.3 i/s
      where with ids:       98.2 i/s - 1.35x  slower
```
@kamipo kamipo merged commit 9817d74 into rails:master May 5, 2020
@kamipo kamipo deleted the perf_where_in branch May 5, 2020 03:06
eileencodes added a commit that referenced this pull request May 5, 2020
This reverts commit 9817d74, reversing
changes made to d326b02.

Just making this easier to merge our PR in. Otherwise there's tons of
conflicts and our PR is faster.
@tenderlove
Copy link
Member

Hey! We reverted this so we could merge #39009 in. 🙇

@kamipo
Copy link
Member Author

kamipo commented May 5, 2020

ah... I'll work based on current master anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants