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

19% reduction is string object allocations per request #12879

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@tenderlove
Member

tenderlove commented Nov 14, 2013

Benchmark is here:

https://github.com/tenderlove/ko1-test-app/blob/d4acd6aba86e3be3a16368296277bc973db8d1a7/perf.rake#L96-L109

I ran it like this:

KO1TEST_PATH=/users/sign_in RAILS_ENV=production rake -f perf.rake allocated_objects

It runs against a basic devise login page.

I started with {:T_STRING=>1688.029} per request, and after this patch is applied it's {:T_STRING=>1380.029} per request.

Merits

  • Eliminates many string object allocations per request
  • Savings grow as your ERb templates grow (the patch freezes string literals in ERb templates)

Demerits

  • freeze eliminates allocations on Ruby 2.1, but is extra overhead on Ruby <= 2.0
19% reduction is string object allocations per request
Benchmark is here:

  https://github.com/tenderlove/ko1-test-app/blob/d4acd6aba86e3be3a16368296277bc973db8d1a7/perf.rake#L96-L109

I ran it like this:

  KO1TEST_PATH=/users/sign_in RAILS_ENV=production rake -f perf.rake allocated_objects

It runs against a basic devise login page.
@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Nov 14, 2013

Member

I forgot to mention, the overhead in other versions of Ruby is that you have to pay extra CPU time for the call to freeze. I haven't done any benchmarks on the impact.

Member

tenderlove commented Nov 14, 2013

I forgot to mention, the overhead in other versions of Ruby is that you have to pay extra CPU time for the call to freeze. I haven't done any benchmarks on the impact.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 14, 2013

Contributor

Good job ❤️! It is a pity that we have to do this manually though, it makes the code uglier. :(

Contributor

josevalim commented Nov 14, 2013

Good job ❤️! It is a pity that we have to do this manually though, it makes the code uglier. :(

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Nov 14, 2013

Member

Would moving such common String#freeze into common module make sense, instead of having ".freeze" clutter the code?

Member

vipulnsward commented Nov 14, 2013

Would moving such common String#freeze into common module make sense, instead of having ".freeze" clutter the code?

@@ -427,7 +427,9 @@ def radio_button_tag(name, value, checked = false, options = {})
def submit_tag(value = "Save changes", options = {})
options = options.stringify_keys
tag :input, { "type" => "submit", "name" => "commit", "value" => value }.update(options)
tag :input, { "type".freeze => "submit".freeze,

This comment has been minimized.

@vipulnsward

vipulnsward Nov 14, 2013

Member

having the freeze out of the method would be more faster, would it?

@vipulnsward

vipulnsward Nov 14, 2013

Member

having the freeze out of the method would be more faster, would it?

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 14, 2013

Member

it is the same.

@rafaelfranca

rafaelfranca Nov 14, 2013

Member

it is the same.

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 14, 2013

Member

i.e. It is the same on Ruby 2.1

@rafaelfranca

rafaelfranca Nov 14, 2013

Member

i.e. It is the same on Ruby 2.1

This comment has been minimized.

@vipulnsward

vipulnsward Nov 15, 2013

Member

This comment has been minimized.

@spastorino

spastorino Nov 18, 2013

Member

@vipulnsward use a more recent Ruby 2.1

@spastorino

spastorino Nov 18, 2013

Member

@vipulnsward use a more recent Ruby 2.1

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Nov 14, 2013

Member

@tenderlove Thanks! 👍

Member

vipulnsward commented Nov 14, 2013

@tenderlove Thanks! 👍

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Nov 14, 2013

:shipit:

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Nov 15, 2013

Member

👍 ➡️ 🚢 ☝️

Member

guilleiguaran commented Nov 15, 2013

👍 ➡️ 🚢 ☝️

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Nov 18, 2013

Member

@tenderlove 👍

This part

NAME      = 'name'.freeze
ID        = 'id'.freeze
INDEX     = 'index'.freeze
MULTIPLE  = 'multiple'.freeze
NAMESPACE = 'namespace'.freeze

Don't need to be stored in constants right now. Unless you want to do execute fast in <= 2.0 and in that case everything should be moved to constants making the .freeze trick useless :P.

So maybe it's better to use just one way of solving the problem?

Member

spastorino commented Nov 18, 2013

@tenderlove 👍

This part

NAME      = 'name'.freeze
ID        = 'id'.freeze
INDEX     = 'index'.freeze
MULTIPLE  = 'multiple'.freeze
NAMESPACE = 'namespace'.freeze

Don't need to be stored in constants right now. Unless you want to do execute fast in <= 2.0 and in that case everything should be moved to constants making the .freeze trick useless :P.

So maybe it's better to use just one way of solving the problem?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 18, 2013

Member

@spastorino no it is needed. If the string is used as Hash key we need to store in the constant and call freeze.

irb(main):001:0> x = "foo"
=> "foo"
irb(main):002:0> y = { x => true }
=> {"foo"=>true}
irb(main):003:0> y.keys.first.object_id == x.object_id
=> false
irb(main):004:0>
irb(main):004:0> x = "foo".freeze
=> "foo"
irb(main):005:0> y = { x => true }
=> {"foo"=>true}
irb(main):006:0> y.keys.first.object_id == x.object_id
=> true
irb(main):007:0>

PS.: snippet stolen from @tenderlove's explanation in the campfire room.

Member

rafaelfranca commented Nov 18, 2013

@spastorino no it is needed. If the string is used as Hash key we need to store in the constant and call freeze.

irb(main):001:0> x = "foo"
=> "foo"
irb(main):002:0> y = { x => true }
=> {"foo"=>true}
irb(main):003:0> y.keys.first.object_id == x.object_id
=> false
irb(main):004:0>
irb(main):004:0> x = "foo".freeze
=> "foo"
irb(main):005:0> y = { x => true }
=> {"foo"=>true}
irb(main):006:0> y.keys.first.object_id == x.object_id
=> true
irb(main):007:0>

PS.: snippet stolen from @tenderlove's explanation in the campfire room.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Nov 18, 2013

Member

I'd like to test the performance impact on 2.0 before merging. I don't think it will cost much, but I don't want people to yell at me either.

Member

tenderlove commented Nov 18, 2013

I'd like to test the performance impact on 2.0 before merging. I don't think it will cost much, but I don't want people to yell at me either.

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Nov 19, 2013

Member

@rafaelfranca ahh ya, you will always pay some tax using Ruby < 2.1. Same thing will be true for string literals appearing inside methods. Solution for Ruby < 2.1 move everything to constants, solution for Ruby >= 2.1 call freeze.

Member

spastorino commented Nov 19, 2013

@rafaelfranca ahh ya, you will always pay some tax using Ruby < 2.1. Same thing will be true for string literals appearing inside methods. Solution for Ruby < 2.1 move everything to constants, solution for Ruby >= 2.1 call freeze.

def render(&block)
options = @options.stringify_keys
tag_value = options.delete("value")
tag_value = options.delete("value".freeze)

This comment has been minimized.

@fxn

fxn Nov 20, 2013

Member

VALUE here like FOR?

@fxn

fxn Nov 20, 2013

Member

VALUE here like FOR?

This comment has been minimized.

@fxn

fxn Nov 20, 2013

Member

Ah, nevermind, I see those are extracted because they have more than one occurrence.

@fxn

fxn Nov 20, 2013

Member

Ah, nevermind, I see those are extracted because they have more than one occurrence.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 20, 2013

Member

So the improvement is great, but It is a pity that the code is uglier and only works for 2.1.

The JVM has a string pool for string literals, I wonder if we could imitate that in code. Having a string pool per Rails component:

module ActionView
  module StringPool
    NAME = 'name'.freeze
    ...
  end
end

and then using those constants instead of string literals.

What do you think?

Member

fxn commented Nov 20, 2013

So the improvement is great, but It is a pity that the code is uglier and only works for 2.1.

The JVM has a string pool for string literals, I wonder if we could imitate that in code. Having a string pool per Rails component:

module ActionView
  module StringPool
    NAME = 'name'.freeze
    ...
  end
end

and then using those constants instead of string literals.

What do you think?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Nov 21, 2013

Member

@tenderlove I have a patched up version of discourse here that runs on Rails master. (You'll need Redis; all the specs should pass.)

They also have some pretty realistic benchmark scripts under /scripts. I haven't tried running your script on there yet, but if you run into any problems, feel free to let me know!

Member

chancancode commented Nov 21, 2013

@tenderlove I have a patched up version of discourse here that runs on Rails master. (You'll need Redis; all the specs should pass.)

They also have some pretty realistic benchmark scripts under /scripts. I haven't tried running your script on there yet, but if you run into any problems, feel free to let me know!

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 21, 2013

Member

@chancancode I'll try to run some benchmarks myself, but in case you'd like to play around I have implemented three string pools in this branch.

They collapse the 477 most common string literals down to 21 unique objects. Goal is to reduce string allocation.

Member

fxn commented Nov 21, 2013

@chancancode I'll try to run some benchmarks myself, but in case you'd like to play around I have implemented three string pools in this branch.

They collapse the 477 most common string literals down to 21 unique objects. Goal is to reduce string allocation.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 21, 2013

Member

@chancancode just in case, not yet ready, the suite is broken right now. Going to work on it.

Member

fxn commented Nov 21, 2013

@chancancode just in case, not yet ready, the suite is broken right now. Going to work on it.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 22, 2013

Member

Just for the record, all is green (and replaced literals are 470 now).

Member

fxn commented Nov 22, 2013

Just for the record, all is green (and replaced literals are 470 now).

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Nov 22, 2013

Member

@fxn 💯 for StringPool 's

Member

vipulnsward commented Nov 22, 2013

@fxn 💯 for StringPool 's

def add_default_name_and_id_for_value(tag_value, options)
if tag_value.nil?
add_default_name_and_id(options)
else
specified_id = options["id"]
specified_id = options[ID]

This comment has been minimized.

@charliesome

charliesome Nov 22, 2013

Contributor

There's no need to pull this out into a constant on 2.1, even if the same string literal is repeated many times.

The "".freeze optimization performs deduplication across the entire program.

@charliesome

charliesome Nov 22, 2013

Contributor

There's no need to pull this out into a constant on 2.1, even if the same string literal is repeated many times.

The "".freeze optimization performs deduplication across the entire program.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 24, 2013

Member

So I have done two things here as a proof of concept:

  • Extract the most repeated string literals in a few components to their respective string pools.
  • Run this sample app and inspect ObjectSpace to see counters per string, resulting in other constants, either in the pools or close to their usage, depending on the number of occurrences.

That's in my branch and yields a 16% reduction in string allocation per request (without freezing ERB, running Ruby 2.0). From 1649 to 1377.

It has to be highlighted that strings are the most common data type in those inspections.

I believe this is encouraging, and readability is not bad given this benefit. I am going to play around with Discourse a bit next week inspecting ObjectSpace.

Member

fxn commented Nov 24, 2013

So I have done two things here as a proof of concept:

  • Extract the most repeated string literals in a few components to their respective string pools.
  • Run this sample app and inspect ObjectSpace to see counters per string, resulting in other constants, either in the pools or close to their usage, depending on the number of occurrences.

That's in my branch and yields a 16% reduction in string allocation per request (without freezing ERB, running Ruby 2.0). From 1649 to 1377.

It has to be highlighted that strings are the most common data type in those inspections.

I believe this is encouraging, and readability is not bad given this benefit. I am going to play around with Discourse a bit next week inspecting ObjectSpace.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Feb 18, 2014

Member

Did we do this? Should I close this?

Member

tenderlove commented Feb 18, 2014

Did we do this? Should I close this?

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Feb 18, 2014

Member

AFAIK this particular patch has not been applied, and albeit I maintain the string pools branch up to date, I am not sure there's enough consensus to merge.

Member

fxn commented Feb 18, 2014

AFAIK this particular patch has not been applied, and albeit I maintain the string pools branch up to date, I am not sure there's enough consensus to merge.

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Feb 18, 2014

Member

I think we are waiting to @jeremy for the approval of string pools branch 😄

Member

guilleiguaran commented Feb 18, 2014

I think we are waiting to @jeremy for the approval of string pools branch 😄

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Feb 18, 2014

Member

I think this or string pools branch should be merged for 4.1. It makes a huge difference without drawbacks. So 👍

Member

spastorino commented Feb 18, 2014

I think this or string pools branch should be merged for 4.1. It makes a huge difference without drawbacks. So 👍

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 20, 2015

Member

This was already done.

Member

rafaelfranca commented Oct 20, 2015

This was already done.

@arthurnn arthurnn deleted the fewer_strings branch Oct 27, 2015

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