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: avoid string allocations #17669

Merged
merged 1 commit into from Nov 20, 2014

Conversation

Projects
None yet
5 participants
@SamSaffron
Copy link
Contributor

SamSaffron commented Nov 18, 2014

I am pretty sure this is correct, its also more explicit about the behavior here.

@egilburg

This comment has been minimized.

Copy link
Contributor

egilburg commented Nov 19, 2014

String#to_s does not allocate new string, so don't see where you are saving on allocation here.

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

SamSaffron commented Nov 19, 2014

its cutting down on a method invocation but this may be erased anyway, not sure, we can omit this

@thedarkone

This comment has been minimized.

Copy link
Contributor

thedarkone commented Nov 19, 2014

@SamSaffron: I'm with @egilburg on this one, the PR doesn't make sense.

@SamSaffron SamSaffron force-pushed the SamSaffron:optimise_memory branch Nov 19, 2014

@SamSaffron SamSaffron force-pushed the SamSaffron:optimise_memory branch to a668b09 Nov 19, 2014

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

SamSaffron commented Nov 19, 2014

@thedarkone @egilburg @sgrif the impact of this is actually quite stark:

Post.includes(topic: :category).limit(1).take(1).first
Post.count

report = MemoryProfiler.report do
  Post.includes(topic: :category).limit(1).take(1).first
  Post.count
end

report.pretty_print

(ran in RAILS_ENV=production)

Before

Total allocated 1520

allocated memory by gem
-----------------------------------
activerecord/lib x 74812
activemodel/lib x 14880
pg-0.15.1 x 5491
arel-a04851702b0e x 2943
activesupport/lib x 680

After

Total allocated 1314

allocated memory by gem
-----------------------------------
activerecord/lib x 54329
activemodel/lib x 14880
pg-0.15.1 x 5491
arel-a04851702b0e x 2943
activesupport/lib x 680

Its a very noticeable chunk of memory being allocated.

@egilburg

This comment has been minimized.

Copy link
Contributor

egilburg commented Nov 19, 2014

@SamSaffron you changed the PR and now always do to_s so the original comment does no longer apply.

The only new thing now is the if (Symbol === field || String === field) clause which, if evauating to false, would result in field rather than arel_table[field] returned.

Under which conditions do you get a field variable there which contains something other than String or Symbol?

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

SamSaffron commented Nov 19, 2014

Under which conditions do you get a field variable there which contains something other than String or Symbol?

You get it arel nodes and arel attributes when constructing complex queries, Post.count generates an Arel::Nodes:Count , and the includes thingy generates an Arel::Attributes::Attribute

@egilburg

This comment has been minimized.

Copy link
Contributor

egilburg commented Nov 20, 2014

@SamSaffron ok now it makes sense, thanks!

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

SamSaffron commented Nov 20, 2014

np, thanks for reviewing sometimes I get a bit carried away with
optimising stuff, erasing a method call can be overdoing it.

On Thu, Nov 20, 2014 at 11:02 AM, Eugene Gilburg notifications@github.com
wrote:

@SamSaffron https://github.com/SamSaffron ok now it makes sense, thanks!


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

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Nov 20, 2014

I still need to track down the source of the arel nodes coming through here, want to make sure there isn't a deeper problem. If there isn't this is :shipit:

@thedarkone

This comment has been minimized.

Copy link
Contributor

thedarkone commented Nov 20, 2014

OK, the revised a668b09 now makes sense.

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Nov 20, 2014

this makes sense to me 🚢

sgrif added a commit that referenced this pull request Nov 20, 2014

Merge pull request #17669 from SamSaffron/optimise_memory
PERF: avoid string allocations

@sgrif sgrif merged commit 347c226 into rails:master Nov 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment