Looking at the history of this method, I'm not sure why it doesn't use
`pluck`. I see that `@members` is later handled as an array, but my impression
is that this initial population reads from a relation anyway, so it should be
fine to `pluck`. Perhaps I'm missing something?
Also I appreciate that this is only used by an endpoint that is covered by
cgimap (if think?), so there's no production impact. But this is still
faster, more idiomatic, and easier to read.
I took the time to run a benchmark. This runs over all relations in London (as
per currently in my local dataset):
```
Testing 31339 relations
user system total real
members_pluck 11.756483 0.644002 12.400485 ( 16.140237)
members 18.120993 1.043173 19.164166 ( 23.377983)
members_pluck (threaded) 15.545434 2.086329 17.631763 ( 16.520121)
members (threaded) 25.014057 2.416027 27.430084 ( 26.083369)
```
<details>
<summary>Benchmark code</summary>
```ruby
# frozen_string_literal: true
require "benchmark"
NUM_THREADS = 10
RELATION_COUNT = Relation.count
BATCH_SIZE = RELATION_COUNT / NUM_THREADS
puts "Testing #{RELATION_COUNT} relations"
Relation.connection.disable_query_cache!
Benchmark.bm do |x|
x.report("members_pluck") do
Relation.all.each(&:members_pluck)
end
x.report("members") do
Relation.all.each(&:members)
end
x.report("members_pluck (threaded)") do
Relation.find_in_batches(:batch_size => BATCH_SIZE).map do |relations|
Thread.new do
relations.each(&:members_pluck)
end
end.map(&:join)
end
x.report("members (threaded)") do
Relation.find_in_batches(:batch_size => BATCH_SIZE).map do |relations|
Thread.new do
relations.each(&:members)
end
end.map(&:join)
end
end
```
This expects there being two versions of the method:
```ruby
# FIXME: is this really needed?
def members
@members ||= relation_members.map do |member|
[member.member_type, member.member_id, member.member_role]
end
end
def members_pluck
@members_pluck ||= relation_members.pluck(:member_type, :member_id,
:member_role)
end
```
</details>
You can view, comment on, or merge this pull request online at:
https://github.com/openstreetmap/openstreetmap-website/pull/6867
-- Commit Summary --
* Faster, more idiomatic, easier to read
-- File Changes --
M app/models/relation.rb (4)
-- Patch Links --
https://github.com/openstreetmap/openstreetmap-website/pull/6867.patch
https://github.com/openstreetmap/openstreetmap-website/pull/6867.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6867
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/[email protected]>
_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
