base::JoinString: Pre-allocate the string to its final size.

base::JoinString: Pre-allocate the string to its final size.

This avoids re-allocation/copying costs as the string is constructed.

BUG=695333
Review-Url: https://codereview.chromium.org/2709983006
Cr-Commit-Position: refs/heads/master@{#454189}
Committed: https://chromium.googlesource.com/chromium/src/+/130c60771ce2b0a3e6ef84046905a7f00fcdb2dd

[Matt Giuca, 2017-02-23 06:08:27]

mgiuca@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org

[dcheng, 2017-02-23 06:29:59]

lgtm

[Matt Giuca, 2017-02-23 23:30:36]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2017-02-24 00:41:45]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 00:42:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 01:46:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 01:46:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Matt Giuca, 2017-02-28 23:10:13]

Looks like Dana is OOO and Daniel has relevant owners, so landing.

[Matt Giuca, 2017-02-28 23:10:19]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-02-28 23:10:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 00:05:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:05:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2017-03-01 00:32:53]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:34:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 01:27:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 01:27:34]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2017-03-01 02:38:39]

While the CQ was failing to finish, I did some profiling on a local build. The
histogram of parts.size() in all calls to JoinString on the browser and GPU
processes after a few minutes of browsing was this:

parts.size(): num_calls
1: 5946
2: 16
3: 18
4: 11
5: 8
7: 1
25: 1
30: 1
314: 98

In light of the fact that 97% of calls have just one string in the vector, and
given that this new implementation does a bunch of unnecessary work in the size
== 1 case, I thought it was prudent to add a fast-path case for size == 1 (just
returns the one string out of the vector). Is this OK?

PS. The 314s all come from the GPU process, which periodically calls JoinString
on all of the available GL extensions. This seems a bit insane especially since
it does about 10 of these on startup. I will investigate further...

[dcheng, 2017-03-01 03:12:08]

Out of curiosity, does it make a difference in actual #s?

[Matt Giuca, 2017-03-01 05:28:41]

On 2017/03/01 03:12:08, dcheng wrote:
> Out of curiosity, does it make a difference in actual #s?

Do you mean timing? I've never really been able to measure timing with any
degree of accuracy with Chrome. Given that we're talking about thousands of
events over a few minutes (and tens of events during startup), I can't imagine
it would make a measurable difference on its own. But I think it's still
important to do optimizations in base-level code because they'll make a
difference in aggregate.

[danakj, 2017-03-01 17:12:44]

On Tue, Feb 28, 2017 at 9:38 PM, <mgiuca@chromium.org> wrote:

> While the CQ was failing to finish, I did some profiling on a local build.
> The
> histogram of parts.size() in all calls to JoinString on the browser and GPU
> processes after a few minutes of browsing was this:
>
> parts.size(): num_calls
> 1: 5946
> 2: 16
> 3: 18
> 4: 11
> 5: 8
> 7: 1
> 25: 1
> 30: 1
> 314: 98
>
> In light of the fact that 97% of calls have just one string in the vector,
> and
> given that this new implementation does a bunch of unnecessary work in the
> size
> == 1 case, I thought it was prudent to add a fast-path case for size == 1
> (just
> returns the one string out of the vector). Is this OK?
>

Seems okay to me. Also, I didn't realize ur waiting for my review too since
dcheng's an owner there too, you can use his review here :)


>
> PS. The 314s all come from the GPU process, which periodically calls
> JoinString
> on all of the available GL extensions. This seems a bit insane especially
> since
> it does about 10 of these on startup. I will investigate further...
>
> https://codereview.chromium.org/2709983006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2017-03-01 18:00:34]

On 2017/03/01 05:28:41, Matt Giuca wrote:
> On 2017/03/01 03:12:08, dcheng wrote:
> > Out of curiosity, does it make a difference in actual #s?
> 
> Do you mean timing? I've never really been able to measure timing with any
> degree of accuracy with Chrome. Given that we're talking about thousands of
> events over a few minutes (and tens of events during startup), I can't imagine
> it would make a measurable difference on its own. But I think it's still
> important to do optimizations in base-level code because they'll make a
> difference in aggregate.

In general, I don't like to add special cases unless it makes some sort of
concrete improvement. It's true that just using the original code means there's
a bit of extra work, but it's a marginal bit of extra work: it's one extra
branch + a few extra simple math operations, which is noise that's probably
going to be lost in the overhead of doing an allocation and copying memory.

[Matt Giuca, 2017-03-01 23:48:25]

On 2017/03/01 17:12:44, danakj wrote:
> Seems okay to me. Also, I didn't realize ur waiting for my review too since
> dcheng's an owner there too, you can use his review here :)

I stopped waiting after I realised that :)

On 2017/03/01 18:00:34, dcheng wrote:
> In general, I don't like to add special cases unless it makes some sort of
> concrete improvement. It's true that just using the original code means
there's
> a bit of extra work, but it's a marginal bit of extra work: it's one extra
> branch + a few extra simple math operations, which is noise that's probably
> going to be lost in the overhead of doing an allocation and copying memory.

It's a bit more than that (iterating over a length-1 vector twice instead of
once, as well as creating an empty string and growing it later versus creating
it in one go), but you're right as a general rule we shouldn't add too many
special cases.

I think I could go either way but since the two reviewers said the opposite, and
I'm scared of introducing even a small performance regression, I'll stick with
Patch Set 3 and keep the special case here. (The tests definitely cover all of
the cases.)

[Matt Giuca, 2017-03-01 23:48:29]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2017-03-01 23:48:30]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2709983006/#ps60001
(title: "Fast-path when parts.size == 1 (extremely common).")

[commit-bot: I haz the power, 2017-03-01 23:49:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-02 00:44:40]

On 2017/03/01 23:48:25, Matt Giuca wrote:
> On 2017/03/01 17:12:44, danakj wrote:
> > Seems okay to me. Also, I didn't realize ur waiting for my review too since
> > dcheng's an owner there too, you can use his review here :)
> 
> I stopped waiting after I realised that :)
> 
> On 2017/03/01 18:00:34, dcheng wrote:
> > In general, I don't like to add special cases unless it makes some sort of
> > concrete improvement. It's true that just using the original code means
> there's
> > a bit of extra work, but it's a marginal bit of extra work: it's one extra
> > branch + a few extra simple math operations, which is noise that's probably
> > going to be lost in the overhead of doing an allocation and copying memory.
> 
> It's a bit more than that (iterating over a length-1 vector twice instead of
> once, as well as creating an empty string and growing it later versus creating
> it in one go), but you're right as a general rule we shouldn't add too many
> special cases.

Creating an empty string, reserving, and then copying into an exactly sized
string will have an almost identical profile to simply copying the string
literally to begin with... or at least it should be if we used SSO strings
instead of COW strings. Unfortunately, Linux uses COW strings, but I'm pretty
sure everywhere else does not.

> 
> I think I could go either way but since the two reviewers said the opposite,
and
> I'm scared of introducing even a small performance regression, I'll stick with
> Patch Set 3 and keep the special case here. (The tests definitely cover all of
> the cases.)

I don't really buy the performance argument without any numbers. =)

[commit-bot: I haz the power, 2017-03-02 00:59:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 00:59:07]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Matt Giuca, 2017-03-02 02:08:11]

On 2017/03/02 00:44:40, dcheng wrote:
> On 2017/03/01 23:48:25, Matt Giuca wrote:
> > On 2017/03/01 17:12:44, danakj wrote:
> > > Seems okay to me. Also, I didn't realize ur waiting for my review too
since
> > > dcheng's an owner there too, you can use his review here :)
> > 
> > I stopped waiting after I realised that :)
> > 
> > On 2017/03/01 18:00:34, dcheng wrote:
> > > In general, I don't like to add special cases unless it makes some sort of
> > > concrete improvement. It's true that just using the original code means
> > there's
> > > a bit of extra work, but it's a marginal bit of extra work: it's one extra
> > > branch + a few extra simple math operations, which is noise that's
probably
> > > going to be lost in the overhead of doing an allocation and copying
memory.
> > 
> > It's a bit more than that (iterating over a length-1 vector twice instead of
> > once, as well as creating an empty string and growing it later versus
creating
> > it in one go), but you're right as a general rule we shouldn't add too many
> > special cases.
> 
> Creating an empty string, reserving, and then copying into an exactly sized
> string will have an almost identical profile to simply copying the string
> literally to begin with... or at least it should be if we used SSO strings
> instead of COW strings. Unfortunately, Linux uses COW strings, but I'm pretty
> sure everywhere else does not.
> 
> > 
> > I think I could go either way but since the two reviewers said the opposite,
> and
> > I'm scared of introducing even a small performance regression, I'll stick
with
> > Patch Set 3 and keep the special case here. (The tests definitely cover all
of
> > the cases.)
> 
> I don't really buy the performance argument without any numbers. =)

OK I didn't realise you were strongly against this.

I'll land the approved version (Patch Set 4 == Patch Set 2), and do some proper
benchmarking later (on Windows, since you say Linux has weird string behaviour).

[Matt Giuca, 2017-03-02 02:08:38]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2017-03-02 02:08:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2709983006/#ps80001
(title: "Revert PS 3; removes the fast path.")

[commit-bot: I haz the power, 2017-03-02 02:09:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 05:59:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488420518672660,
"parent_rev": "60bfbf8a7d59d1f63b5a73f1ac10dee41c6a7ded", "commit_rev":
"130c60771ce2b0a3e6ef84046905a7f00fcdb2dd"}

[commit-bot: I haz the power, 2017-03-02 06:00:47]

Description was changed from

==========
base::JoinString: Pre-allocate the string to its final size.

This avoids re-allocation/copying costs as the string is constructed.

BUG=695333
==========

to

==========
base::JoinString: Pre-allocate the string to its final size.

This avoids re-allocation/copying costs as the string is constructed.

BUG=695333

Review-Url: https://codereview.chromium.org/2709983006
Cr-Commit-Position: refs/heads/master@{#454189}
Committed:
https://chromium.googlesource.com/chromium/src/+/130c60771ce2b0a3e6ef84046905...
==========

[commit-bot: I haz the power, 2017-03-02 06:00:48]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/130c60771ce2b0a3e6ef84046905...