Vector::insert and Vector::append may use memcpy for POD types

Vector::insert and Vector::append may use memcpy for POD types.

It will improve performance when POD types are copied, by providing
a missing implementation of VectorCopier and using it in
two places which were previously always taking the "new" allocation
path even though they could have used VectorCopier and used
memcpy for POD types.

NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168472

[darin (slow to review), 2014-02-24 20:26:14]

I think you should ask someone from wtf/OWNERS to review this.

[Mikhail, 2014-02-25 14:11:31]

https://codereview.chromium.org/178313002/diff/1/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/178313002/diff/1/Source/wtf/Vector.h#newcode166
Source/wtf/Vector.h:166: static void uninitializedCopy(const T* src, const T*
srcEnd, U* dst)
I'd change it to

template<typename U>
static void uninitializedCopy(const U* src, const U* srcEnd, T* dst)

because the type 'T' is usually used as a native vector type and the type 'U' -
as something else, otherwise it is a bit misleading imho.

[w.bielawski, 2014-02-26 16:40:44]

https://codereview.chromium.org/178313002/diff/1/Source/wtf/Vector.h
File Source/wtf/Vector.h (right):

https://codereview.chromium.org/178313002/diff/1/Source/wtf/Vector.h#newcode166
Source/wtf/Vector.h:166: static void uninitializedCopy(const T* src, const T*
srcEnd, U* dst)
On 2014/02/25 14:11:32, mikhail.pozdnyakov wrote:
> I'd change it to
> 
> template<typename U>
> static void uninitializedCopy(const U* src, const U* srcEnd, T* dst)
> 
> because the type 'T' is usually used as a native vector type and the type 'U'
-
> as something else, otherwise it is a bit misleading imho.

Done.

[Mikhail, 2014-02-27 09:30:22]

IMO this patch makes sense, considering that we have 'Vector::appendVector' API.
CC'ed WTF owners.

[eseidel, 2014-02-27 19:27:37]

Could you expand your description to explain why?

[Erik Corry, 2014-02-28 13:26:17]

LGTM

[Erik Corry, 2014-02-28 13:26:44]

On 2014/02/27 19:27:37, eseidel wrote:
> Could you expand your description to explain why?

Presumably memcpy is faster.

[w.bielawski, 2014-03-03 08:29:31]

The CQ bit was checked by testertelefonu@gmail.com

[commit-bot: I haz the power, 2014-03-03 08:29:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/testertelefonu@gmail.com/178313002/30001

[commit-bot: I haz the power, 2014-03-03 09:12:52]

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

[commit-bot: I haz the power, 2014-03-03 09:12:53]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Erik Corry, 2014-03-03 10:16:01]

This one needs Eric's LGTM to move forward (or another WTF owner).

[w.bielawski, 2014-03-03 10:50:42]

On 2014/03/03 09:12:53, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

eseidel do you accept this patch?

[eseidel, 2014-03-03 19:20:07]

You still haven't explained why in the description.

[w.bielawski, 2014-03-04 08:42:49]

On 2014/03/03 19:20:07, eseidel wrote:
> You still haven't explained why in the description.

OK, I just thought you've asked Erik for explanation.

[eseidel, 2014-03-04 08:47:05]

lgtm

OK, makes sense now, re-reading the code.  Thanks.

[commit-bot: I haz the power, 2014-03-04 08:47:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/testertelefonu@gmail.com/178313002/30001

[eseidel, 2014-03-04 08:48:04]

The CQ bit was unchecked by eseidel@chromium.org

[eseidel, 2014-03-04 08:48:20]

I've expanded your CL description based on the comments and my reading of the
code.

[eseidel, 2014-03-04 08:48:24]

The CQ bit was checked by eseidel@chromium.org

[w.bielawski, 2014-03-04 11:27:09]

On 2014/03/04 08:48:20, eseidel wrote:
> I've expanded your CL description based on the comments and my reading of the
> code.

Thanks. I wasn't aware that such expanded description is required.

[eseidel, 2014-03-04 17:16:21]

The description is what goes in the commit message.  In Blink we try to have
descriptive commit messages in order to aid code archeology.   This is a habit
we inherited from WebKit (which requires a ChangeLog message with every change):
http://www.webkit.org/coding/contributing.html#changelogs

[w.bielawski, 2014-03-05 11:17:40]

On 2014/03/04 17:16:21, eseidel wrote:
> The description is what goes in the commit message.  In Blink we try to have
> descriptive commit messages in order to aid code archeology.   This is a habit
> we inherited from WebKit (which requires a ChangeLog message with every
change):
> http://www.webkit.org/coding/contributing.html#changelogs

Can win_layout be somehow skipped? Looks like problem with buildbot itself.

[Mads Ager (chromium), 2014-03-05 12:10:15]

On 2014/03/05 11:17:40, w.bielawski wrote:
> On 2014/03/04 17:16:21, eseidel wrote:
> > The description is what goes in the commit message.  In Blink we try to have
> > descriptive commit messages in order to aid code archeology.   This is a
habit
> > we inherited from WebKit (which requires a ChangeLog message with every
> change):
> > http://www.webkit.org/coding/contributing.html#changelogs
> 
> Can win_layout be somehow skipped? Looks like problem with buildbot itself.

This is all looking good. Let's get this in. I'll kick it in and watch the bots.

[Mads Ager (chromium), 2014-03-05 12:10:22]

The CQ bit was unchecked by ager@chromium.org

[Mads Ager (chromium), 2014-03-05 12:10:46]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-05 12:11:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/testertelefonu@gmail.com/178313002/30001

[commit-bot: I haz the power, 2014-03-05 12:12:30]

Change committed as 168472