PERF: avoid string allocations

[SamSaffron]

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

[egilburg]

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

[SamSaffron]

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

[thedarkone]

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

[SamSaffron]

@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]

@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]

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]

@SamSaffron ok now it makes sense, thanks!

[SamSaffron]

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]

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

[thedarkone]

OK, the revised a668b09 now makes sense.

[arthurnn]

this makes sense to me 🚢