Skip to content

Conversation

SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Jun 4, 2018

When requesting columns names from database adapters AR:Result
would dup/freeze column names, this prefers using fstrings which
cuts down on repeat allocations

Attributes that are retained keep these fstrings around for the long
term

Note, this has the highest impact on "short" result sets, eg: Topic.first where you can void allocating the number of columns * String.

See also: https://samsaffron.com/archive/2018/02/16/reducing-string-duplication-in-ruby for detailed explanation of uminus

per @rafaelfranca we are fine including uminus in master now. cc @tenderlove @sgrif

@rails-bot
Copy link

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@@ -125,7 +125,7 @@ def hash_rows
begin
# We freeze the strings to prevent them getting duped when
# used as keys in ActiveRecord::Base's @attributes hash
columns = @columns.map { |c| c.dup.freeze }
columns = @columns.map { |c| -c }
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to change to columns = @columns.map(&:-@) for a small speed improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdewater yes I can confirm this is worth it to change, I will amend this PR tomorrow

https://gist.github.com/SamSaffron/805bf43016cc897c0f0c19078e7ad171

@bdewater
Copy link
Contributor

bdewater commented Jun 5, 2018

It's the reason Rubocop has Style/SymbolProc. There's a lot of context in #16833

benchmark.rb for this case:

require 'benchmark/ips'

COLUMNS = %w(id foo bar baz qux test yolo)

def block
  COLUMNS.map { |c| -c }
end

def to_proc
  COLUMNS.map(&:-@)
end

Benchmark.ips do |x|
  x.report("block")   { block }
  x.report("to_proc") { to_proc }
  x.compare!
end
Warming up --------------------------------------
               block    65.000k i/100ms
             to_proc    71.129k i/100ms
Calculating -------------------------------------
               block    774.817k (± 1.4%) i/s -      3.900M in   5.034500s
             to_proc    863.802k (± 1.2%) i/s -      4.339M in   5.023677s

Comparison:
             to_proc:   863801.9 i/s
               block:   774816.7 i/s - 1.11x  slower

@rafaelfranca
Copy link
Member

LGTM! Let me know when you update the PR to include the &:-@ change.

When requesting columns names from database adapters AR:Result
would dup/freeze column names, this prefers using fstrings which
cuts down on repeat allocations

Attributes that are retained keep these fstrings around for the long
term

Note, this has the highest impact on "short" result sets, eg: Topic.first where you can void allocating the number of columns * String.
@SamSaffron SamSaffron force-pushed the master branch 2 times, most recently from 88a2f1f to a46dcb7 Compare June 5, 2018 23:54
@SamSaffron
Copy link
Contributor Author

@rafaelfranca done! 🎊

@matthewd matthewd merged commit d0d3e96 into rails:master Jun 6, 2018
@rafaelfranca
Copy link
Member

❤️

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.

5 participants