String concat #20

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@kbrock
Contributor

kbrock commented Dec 15, 2012

Didn't really produce the savings I was expecting.
Basically, it is the same thing.

So don't accept this request.

But it is an alternative view of the problem.

So wanted to forward along.

@jeremyw jeremyw closed this Dec 17, 2012

value = modify(target.send(field))
- '%2.2d' % value
+ out << (value < 10 ? '0' : '') << value.to_s

This comment has been minimized.

@kbrock

kbrock Apr 20, 2015

Contributor

yea, this should be:

out << '0' if value < 10
out << value
@kbrock

kbrock Apr 20, 2015

Contributor

yea, this should be:

out << '0' if value < 10
out << value
@kbrock

This comment has been minimized.

Show comment
Hide comment
@kbrock

kbrock Apr 20, 2015

Contributor

@jeremyw any thoughts of this?

Funny. I was just about to suggest this PR again.
I did notice that the join code was converted to an each and <<. So most of the speed improvement is probably already realized.

Locally my raw benchmarks seem to suggest this could get 10% speed improvement. Mostly around the out << '0' use case.

If you are open to this idea, I'll rebase and play. Otherwise, if you really don't like it, I'll let live and probably ask again in another year or so.

Contributor

kbrock commented Apr 20, 2015

@jeremyw any thoughts of this?

Funny. I was just about to suggest this PR again.
I did notice that the join code was converted to an each and <<. So most of the speed improvement is probably already realized.

Locally my raw benchmarks seem to suggest this could get 10% speed improvement. Mostly around the out << '0' use case.

If you are open to this idea, I'll rebase and play. Otherwise, if you really don't like it, I'll let live and probably ask again in another year or so.

- result = ''
- @emitters.each { |e| result << e.format(target).to_s }
- result
+ def format(out, target)

This comment has been minimized.

@jeremyw

jeremyw Apr 20, 2015

Owner

I'm not sure why this is better. It seems like it just distributes string-building responsibility to the emitters.

@jeremyw

jeremyw Apr 20, 2015

Owner

I'm not sure why this is better. It seems like it just distributes string-building responsibility to the emitters.

This comment has been minimized.

@kbrock

kbrock Apr 20, 2015

Contributor

yes. that is all it is.
In the cases where you want to output more than 1 string, it ends up being quicker to lambda |result, target| result << str1 << str2 } than result << lambda { |target| str1 + str2 }.

Granted it is probably 10% in trivial benchmarks, but hey. that is something right?
(well, if you think it is much uglier, than that probably doesn't warrant the change)

It is hard to benchmark across these changes. I like benchmark-ips, but not sure how to do it across sweeping changes like this. Only can do it with lambdas.

@kbrock

kbrock Apr 20, 2015

Contributor

yes. that is all it is.
In the cases where you want to output more than 1 string, it ends up being quicker to lambda |result, target| result << str1 << str2 } than result << lambda { |target| str1 + str2 }.

Granted it is probably 10% in trivial benchmarks, but hey. that is something right?
(well, if you think it is much uglier, than that probably doesn't warrant the change)

It is hard to benchmark across these changes. I like benchmark-ips, but not sure how to do it across sweeping changes like this. Only can do it with lambdas.

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