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: stop allocating the string "id" over and over #17658

Merged
merged 1 commit into from Nov 18, 2014

Conversation

Projects
None yet
7 participants
@SamSaffron
Copy link
Contributor

SamSaffron commented Nov 18, 2014

This one is HUGE, on our front page this cut down 5000 allocations of the string "id"

@fxn

This comment has been minimized.

Copy link
Member

fxn commented Nov 18, 2014

Yep, there are some hot literals like this. We've experimented with their extraction in a couple of branches but need to find a balance between code readability and performance impact. /cc @jeremy

Let's merge this one, but let me make clear for other contributors that we are not suddenly going to merge patches like this unless they are as justified as this one. Doing this at a greater scale is the thing we are exploring precisely.

Thanks Sam.

fxn added a commit that referenced this pull request Nov 18, 2014

Merge pull request #17658 from SamSaffron/optimise_memory
PERF: stop allocating the string "id" over and over

@fxn fxn merged commit 4fc493f into rails:master Nov 18, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

SamSaffron commented Nov 18, 2014

definitely only do this for stuff you can prove is a problem ideally using the memory_profiler gem, this particular one is called per column for every row of every attribute accessed, so it is very important.

@jonatack

This comment has been minimized.

Copy link
Contributor

jonatack commented Nov 18, 2014

@SamSaffron I'm curious, wouldn't using 'id'.freeze without creating a constant have the same effect?

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

SamSaffron commented Nov 18, 2014

yes, in Ruby 2.1 and above, but in 2.0 and below you will still get dupes.

On Wed, Nov 19, 2014 at 10:02 AM, Jon Atack notifications@github.com
wrote:

@SamSaffron https://github.com/SamSaffron I'm curious, would simply
using 'id'.freeze without creating a constant have the same effect?


Reply to this email directly or view it on GitHub
#17658 (comment).

@jonatack

This comment has been minimized.

Copy link
Contributor

jonatack commented Nov 18, 2014

Ah! Thank you for the reminder. Good to keep in mind.

@jjb

This comment has been minimized.

Copy link
Contributor

jjb commented Dec 5, 2014

yes, in Ruby 2.1 and above, but in 2.0 and below you will still get dupes.

Code optimizations for ruby 2.0 seems completely unnecessary to me — I imagine the vast majority of folks using 2.x will typically be upgrading to the newest 2.x. Folks stuck on 2.0 for some horrible enterprisey policy reasons are probably a very small set of users and their departments are clearly not optimizing their policies for performance to begin with.

Doing merely "id".freeze will be simpler, easier-to-maintain code, more clearly express intent, and is a simpler and less controversial solution to apply to other places in the codebase in a consistent manner.

my $0.02

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Dec 5, 2014

@jjb

This comment has been minimized.

Copy link
Contributor

jjb commented Dec 5, 2014

ah yeah, forgot about that. so, perhaps even more reason to not clutter the solution with accommodation for 2.0 users?

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 5, 2014

Because we still are not sure if calling freeze inline is the best solution
for the framework, talking about code style. I personally finds it very
ugly.
On Dec 5, 2014 4:11 PM, "John Bachir" notifications@github.com wrote:

ah yeah, forgot about that. so, perhaps even more reason to not clutter
the solution with accommodation for 2.0 users?


Reply to this email directly or view it on GitHub
#17658 (comment).

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Dec 5, 2014

We no longer go through this branch of code internally, anyway so the discussion is moot.

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