Oilpan: Remove reserveInitialCapacity from DistributedNodes' constructor

Oilpan: Remove reserveInitialCapacity from DistributedNodes' constructor

In short:

This CL is necessary to fix performance regressions observed in Oilpan.
This CL doesn't affect performance of non-Oilpan.

In details:

The reserveInitialCapacity(32) was introduced in https://chromiumcodereview.appspot.com/23766020.
As far as I read the CL description, it seems that the reserveInitialCapacity(32) was introduced
just to prevent a regression in micro-benchmarks without doing serious performance measurement
in realistic scenarios. The reserveInitialCapacity(32) may be working well in non-Oilpan,
but at the very least, it is causing a regression in Oilpan.

With Oilpan enabled, allocating the vector buffer (of size sizeof(Node)*32) adds a substantial
amount of pressures on the heap and thus increases a GC frequency. The regression
caused by the GCs is larger than the improvement caused by the pre-allocation.
Overall, it is better to not call reserveInitialCapacity(32) in Oilpan.
For example, document.createElement("select") is 2x slower in Oilpan than in non-Oilpan.

Thus this CL removes reserveInitialCapacity(32) from DistributedNodes' constructor.

This CL removes an unused method from DistributedNodes.

BUG=

Committed: https://crrev.com/c655d8c1a866eebe51f54f3d4803f3d3c19a523d
git-svn-id: svn://svn.chromium.org/blink/trunk@202244 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[haraken, 2015-09-10 07:50:56]

haraken@chromium.org changed reviewers:
+ hayato@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org

[haraken, 2015-09-10 07:50:56]

PTAL

https://codereview.chromium.org/1333813002/diff/1/Source/core/dom/shadow/Dist...
File Source/core/dom/shadow/DistributedNodes.h (left):

https://codereview.chromium.org/1333813002/diff/1/Source/core/dom/shadow/Dist...
Source/core/dom/shadow/DistributedNodes.h:63: const
WillBeHeapVector<RefPtrWillBeMember<Node>>& nodes() const { return m_nodes; }

This is unused.

[haraken, 2015-09-10 08:39:37]

If you really want to keep the initial capacity, another option would be to use
a vector with inline capacity (i.e., Vector<Node*, 32>). I'm fine with either
way.

[haraken, 2015-09-11 03:54:05]

tkent-san or hayato-san: friendly ping?

[hayato, 2015-09-11 07:07:30]

This CL reminds me of another CL: https://codereview.chromium.org/1257573002/

Looks a different approach. Both try to improve only the result of
micro-benchmark.
I guess the original motivation of reversing InitialCapacity is to improve the
performance in the real world.

I am aware that it's difficult to tell, but can this CL improve performance
other than the result of this benchmark? That's my concern.

[haraken, 2015-09-11 07:15:55]

On 2015/09/11 07:07:30, hayato wrote:
> This CL reminds me of another CL: https://codereview.chromium.org/1257573002/
> 
> Looks a different approach. Both try to improve only the result of
> micro-benchmark.
> I guess the original motivation of reversing InitialCapacity is to improve the
> performance in the real world.
> 
> I am aware that it's difficult to tell, but can this CL improve performance
> other than the result of this benchmark? That's my concern.

This CL just affects performance of oilpan builds. As far as I observe:

- This CL does not change performance of Shadow DOM micro-benchmarks in
non-oilpan.
- This CL fixes a bunch of performance regressions in oilpan.

If you're not willing to change the capacity, I'll just use a vector with inline
capacity of 32.

[haraken, 2015-09-11 07:17:35]

On 2015/09/11 07:15:55, haraken wrote:
> On 2015/09/11 07:07:30, hayato wrote:
> > This CL reminds me of another CL:
https://codereview.chromium.org/1257573002/
> > 
> > Looks a different approach. Both try to improve only the result of
> > micro-benchmark.
> > I guess the original motivation of reversing InitialCapacity is to improve
the
> > performance in the real world.
> > 
> > I am aware that it's difficult to tell, but can this CL improve performance
> > other than the result of this benchmark? That's my concern.
> 
> This CL just affects performance of oilpan builds. As far as I observe:
> 
> - This CL does not change performance of Shadow DOM micro-benchmarks in
> non-oilpan.
> - This CL fixes a bunch of performance regressions in oilpan.
> 
> If you're not willing to change the capacity, I'll just use a vector with
inline
> capacity of 32.

Just to clarify: Currently document.createElement("select") (or creating any
element that requires a shadow root) is 200% slower in oilpan than in
non-oilpan. This CL fixes that.

[hayato, 2015-09-11 08:58:37]

Hmm. It sounds you don't understand my point.

Suppose that Shadow DOM micro-benchmarks were written so that it used about 20
nodes which are distributed, this CL would decrease the performance, wouldn't
it?

[haraken, 2015-09-11 09:15:06]

On 2015/09/11 08:58:37, hayato wrote:
> Hmm. It sounds you don't understand my point.
> 
> Suppose that Shadow DOM micro-benchmarks were written so that it used about 20
> nodes which are distributed, this CL would decrease the performance, wouldn't
> it?

I changed it to use the inline capacity of size 32. Are you fine with this?

[Yuta Kitamura, 2015-09-11 09:23:43]

On 2015/09/11 08:58:37, hayato wrote:
> Hmm. It sounds you don't understand my point.
> 
> Suppose that Shadow DOM micro-benchmarks were written so that it used about 20
> nodes which are distributed, this CL would decrease the performance, wouldn't
> it?

haraken said:
> - This CL does not change performance of Shadow DOM micro-benchmarks in
non-oilpan.

So I suppose the answer is "no".

I think the reserveInitialCapacity call is a premature optimization
and it makes little sense to keep it as-is.

And IMO shadow DOM microbenchmarks are not realistic and have little
value, given its little adoption. The performance of <select>
creation is far more important and we should focus on it first.

[hayato, 2015-09-11 09:25:28]

btw, lgtm

I'm okay to this change, however, the CL's description should mention that we
don't know anything about the performance impact of this CL in the real world.

[haraken, 2015-09-11 09:26:08]

On 2015/09/11 09:25:28, hayato wrote:
> btw, lgtm
> 
> I'm okay to this change, however, the CL's description should mention that we
> don't know anything about the performance impact of this CL in the real world.

Will do.

Which do you prefer, PS1 or PS2?

[hayato, 2015-09-11 09:31:33]

Before landing, could you check how the benchmark result would differ between
this CL and https://codereview.chromium.org/1257573002/ ?

I'm interested in how both would differ in each test.

[haraken, 2015-09-11 09:37:19]

On 2015/09/11 09:31:33, hayato wrote:
> Before landing, could you check how the benchmark result would differ between
> this CL and https://codereview.chromium.org/1257573002/ ?
> 
> I'm interested in how both would differ in each test.

I'll measure it but these two CLs are totally orthogonal. We can apply both if
we want.

[hayato, 2015-09-11 09:38:47]

This CL improves nothing, however, this approach,
https://codereview.chromium.org/1257573002/, says that it would improves 4%. I'm
curious whether this is true or not in each test.

[hayato, 2015-09-11 09:53:03]

hayato@chromium.org changed reviewers:
+ cevans@google.com

[hayato, 2015-09-11 09:54:24]

Added cevans@ to reviewers. It looks
https://chromiumcodereview.appspot.com/23766020 introduced this default vector
sizing.

cevans@, are you okay to this change?

[haraken, 2015-09-11 09:55:58]

On 2015/09/11 09:54:24, hayato wrote:
> Added cevans@ to reviewers. It looks
> https://chromiumcodereview.appspot.com/23766020 introduced this default vector
> sizing.
> 
> cevans@, are you okay to this change?

cevans@ left the team...

If you're suspicious about potential performance impact, we can go with PS2. I'm
measuring performance of all shadow_dom benchmarks with/without this CL and that
CL.

[haraken, 2015-09-11 10:14:44]

On 2015/09/11 09:55:58, haraken wrote:
> On 2015/09/11 09:54:24, hayato wrote:
> > Added cevans@ to reviewers. It looks
> > https://chromiumcodereview.appspot.com/23766020 introduced this default
vector
> > sizing.
> > 
> > cevans@, are you okay to this change?
> 
> cevans@ left the team...
> 
> If you're suspicious about potential performance impact, we can go with PS2.
I'm
> measuring performance of all shadow_dom benchmarks with/without this CL and
that
> CL.

Here is a result:

http://haraken.info/a/results.html

master: ToT
inline: ToT + PS2
remove: ToT + PS1
patch: ToT + https://codereview.chromium.org/1257573002/

The result is a mixed and flaky bag and it's hard to say which configuation is
the best. (If so, I'd prefer "inline" or "remove".) What do you think?

[haraken, 2015-09-11 10:26:20]

On 2015/09/11 10:14:44, haraken wrote:
> On 2015/09/11 09:55:58, haraken wrote:
> > On 2015/09/11 09:54:24, hayato wrote:
> > > Added cevans@ to reviewers. It looks
> > > https://chromiumcodereview.appspot.com/23766020 introduced this default
> vector
> > > sizing.
> > > 
> > > cevans@, are you okay to this change?
> > 
> > cevans@ left the team...
> > 
> > If you're suspicious about potential performance impact, we can go with PS2.
> I'm
> > measuring performance of all shadow_dom benchmarks with/without this CL and
> that
> > CL.
> 
> Here is a result:
> 
> http://haraken.info/a/results.html
> 
> master: ToT
> inline: ToT + PS2
> remove: ToT + PS1
> patch: ToT + https://codereview.chromium.org/1257573002/
> 
> The result is a mixed and flaky bag and it's hard to say which configuation is
> the best. (If so, I'd prefer "inline" or "remove".) What do you think?

I manually ran the benchmarks a couple of times. The 3 - 5% regression in
LargeDistribution of "remove" looks real. "inline" doesn't involve the
regression.

So landing PS2 would be safer.

[Yuta Kitamura, 2015-09-11 13:20:57]

On 2015/09/11 10:26:20, haraken wrote:
> On 2015/09/11 10:14:44, haraken wrote:
> > On 2015/09/11 09:55:58, haraken wrote:
> > > On 2015/09/11 09:54:24, hayato wrote:
> > > > Added cevans@ to reviewers. It looks
> > > > https://chromiumcodereview.appspot.com/23766020 introduced this default
> > vector
> > > > sizing.
> > > > 
> > > > cevans@, are you okay to this change?
> > > 
> > > cevans@ left the team...
> > > 
> > > If you're suspicious about potential performance impact, we can go with
PS2.
> > I'm
> > > measuring performance of all shadow_dom benchmarks with/without this CL
and
> > that
> > > CL.
> > 
> > Here is a result:
> > 
> > http://haraken.info/a/results.html
> > 
> > master: ToT
> > inline: ToT + PS2
> > remove: ToT + PS1
> > patch: ToT + https://codereview.chromium.org/1257573002/
> > 
> > The result is a mixed and flaky bag and it's hard to say which configuation
is
> > the best. (If so, I'd prefer "inline" or "remove".) What do you think?
> 
> I manually ran the benchmarks a couple of times. The 3 - 5% regression in
> LargeDistribution of "remove" looks real. "inline" doesn't involve the
> regression.

You ran the test just twice, right? Usually, twice is too
few to tell there's a regression for sure.

I'd recommend at least 20 times or even 100 times (using
--page-repeat or whatever).

Generally speaking, it's pretty hard to understand the
performance results because the test results are very flaky.
It's better to leave that responsibility to a pro ;)

[Yuta Kitamura, 2015-09-11 13:23:00]

(And in my personal opinion, the set up of LargeDistribution
 is so unrealistic that I see little points in keeping that
 metrics.)

[haraken, 2015-09-11 13:23:40]

On 2015/09/11 13:20:57, Yuta Kitamura wrote:
> On 2015/09/11 10:26:20, haraken wrote:
> > On 2015/09/11 10:14:44, haraken wrote:
> > > On 2015/09/11 09:55:58, haraken wrote:
> > > > On 2015/09/11 09:54:24, hayato wrote:
> > > > > Added cevans@ to reviewers. It looks
> > > > > https://chromiumcodereview.appspot.com/23766020 introduced this
default
> > > vector
> > > > > sizing.
> > > > > 
> > > > > cevans@, are you okay to this change?
> > > > 
> > > > cevans@ left the team...
> > > > 
> > > > If you're suspicious about potential performance impact, we can go with
> PS2.
> > > I'm
> > > > measuring performance of all shadow_dom benchmarks with/without this CL
> and
> > > that
> > > > CL.
> > > 
> > > Here is a result:
> > > 
> > > http://haraken.info/a/results.html
> > > 
> > > master: ToT
> > > inline: ToT + PS2
> > > remove: ToT + PS1
> > > patch: ToT + https://codereview.chromium.org/1257573002/
> > > 
> > > The result is a mixed and flaky bag and it's hard to say which
configuation
> is
> > > the best. (If so, I'd prefer "inline" or "remove".) What do you think?
> > 
> > I manually ran the benchmarks a couple of times. The 3 - 5% regression in
> > LargeDistribution of "remove" looks real. "inline" doesn't involve the
> > regression.
> 
> You ran the test just twice, right? Usually, twice is too
> few to tell there's a regression for sure.
> 
> I'd recommend at least 20 times or even 100 times (using
> --page-repeat or whatever).
> 
> Generally speaking, it's pretty hard to understand the
> performance results because the test results are very flaky.
> It's better to leave that responsibility to a pro ;)

I ran the test 10 times :)

I'll run it 100 times overnight.

[haraken, 2015-09-11 13:25:01]

On 2015/09/11 13:23:00, Yuta Kitamura wrote:
> (And in my personal opinion, the set up of LargeDistribution
>  is so unrealistic that I see little points in keeping that
>  metrics.)

Yes, I don't think it's worth spending a lot of time in measuring benchmarks
here. IMHO, I'd like to just land PS2 or if you don't like it, I want to enable
this CL only in ENABLE(OILPAN).

[esprehn, 2015-09-14 02:07:02]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-09-14 02:07:02]

Don't we shrink this later? That would mean inline capacity is producing more
waste.

[haraken, 2015-09-14 04:58:29]

On 2015/09/14 02:07:02, esprehn wrote:
> Don't we shrink this later? That would mean inline capacity is producing more
> waste.

Yes, it can shrink later (but not always).

Maybe would it be better to land PS1 with #if ENABLE(OILPAN)?

All of the discussions in this thread are just gaming unrealistic
micro-benchmarks. Whatever approach we take, we can invent a scenario that
regresses performance.

[haraken, 2015-09-14 05:02:55]

On 2015/09/14 04:58:29, haraken wrote:
> On 2015/09/14 02:07:02, esprehn wrote:
> > Don't we shrink this later? That would mean inline capacity is producing
more
> > waste.
> 
> Yes, it can shrink later (but not always).
> 
> Maybe would it be better to land PS1 with #if ENABLE(OILPAN)?
> 
> All of the discussions in this thread are just gaming unrealistic
> micro-benchmarks. Whatever approach we take, we can invent a scenario that
> regresses performance.

PS3 added #if ENABLE(OILPAN) so that the change doesn't affect non-oilpan builds
at all. PTAL.

[hayato, 2015-09-14 05:29:06]

If you can agree that it doesn't make sense to improve the result of unrealistic
micro-benchmarks, what's the purpose of this CL?

[haraken, 2015-09-14 05:35:32]

On 2015/09/14 05:29:06, hayato wrote:
> If you can agree that it doesn't make sense to improve the result of
unrealistic
> micro-benchmarks, what's the purpose of this CL?

To fix the clear regressions in the micro-benchmarks on Oilpan builds. Of course
we can show separate data to clarify how much regression in each benchmark is
attributed to the DistributedNodes' constructor, but it is much clearer to just
land this CL than to justify the regression.

PS3 doesn't affect non-oilpan builds at all.

[hayato, 2015-09-14 05:36:34]

I'm not saying that we shouldn't land this CL, but the description of this CL
should be rewritten.

It should mention where and why this default vector sizing was introduced and
explain the reason why the default vector sizing shouldn't be introduced there.

[hayato, 2015-09-14 05:39:02]

On 2015/09/14 05:36:34, hayato wrote:
> I'm not saying that we shouldn't land this CL, but the description of this CL
> should be rewritten.
> 
> It should mention where and why this default vector sizing was introduced and
> explain the reason why the default vector sizing shouldn't be introduced
there.

s/default vector sizing/reserveInitialCapacity(32)/

[haraken, 2015-09-14 06:12:15]

On 2015/09/14 05:39:02, hayato wrote:
> On 2015/09/14 05:36:34, hayato wrote:
> > I'm not saying that we shouldn't land this CL, but the description of this
CL
> > should be rewritten.
> > 
> > It should mention where and why this default vector sizing was introduced
and
> > explain the reason why the default vector sizing shouldn't be introduced
> there.
> 
> s/default vector sizing/reserveInitialCapacity(32)/

Updated the CL description. PTAL.

[hayato, 2015-09-14 06:26:30]

Please mention the original CL in the description:
https://chromiumcodereview.appspot.com/23766020

> The reserveInitialCapacity(32) was introduced with the assumption that
each shadow root will have 32 distributed nodes.

As far as I can read from the CL, there is no such an assumption, I think.
It looks that reverveInitialCapacity(32) was introduced to prevent a regression
in the micro benchmark *without* considering any semantics around there.

You should make use of this fact to justify your CL. :)

[haraken, 2015-09-14 06:41:10]

On 2015/09/14 06:26:30, hayato wrote:
> Please mention the original CL in the description:
> https://chromiumcodereview.appspot.com/23766020
> 
> > The reserveInitialCapacity(32) was introduced with the assumption that
> each shadow root will have 32 distributed nodes.
> 
> As far as I can read from the CL, there is no such an assumption, I think.
> It looks that reverveInitialCapacity(32) was introduced to prevent a
regression
> in the micro benchmark *without* considering any semantics around there.
> 
> You should make use of this fact to justify your CL. :)

Done.

[hayato, 2015-09-14 08:02:58]

Thanks.

"Don't reserve initial capacity" is always useful for Oilpan?

I think *reservingInitialCapacity* is a common technique to improve the
performance in C++, *usually*, however, this is no longer true in Oilpan world?
Is there any guideline how `vector` should be used in Oilpan world?

[sof, 2015-09-14 08:08:15]

On 2015/09/14 08:02:58, hayato wrote:
> Thanks.
> 
> "Don't reserve initial capacity" is always useful for Oilpan?
> 
> I think *reservingInitialCapacity* is a common technique to improve the
> performance in C++, *usually*, however, this is no longer true in Oilpan
world?
> Is there any guideline how `vector` should be used in Oilpan world?

I think you need to clarify why exactly the reservation is there in the first
place. I've only seen guesswork thus far in this review, but perhaps i've
overlooked one of the many replies.

[haraken, 2015-09-14 08:10:27]

On 2015/09/14 08:02:58, hayato wrote:
> Thanks.
> 
> "Don't reserve initial capacity" is always useful for Oilpan?
> 
> I think *reservingInitialCapacity* is a common technique to improve the
> performance in C++, *usually*, however, this is no longer true in Oilpan
world?
> Is there any guideline how `vector` should be used in Oilpan world?

- reserveInitialCapacity(N) is still useful in oilpan if N elements are really
going to be allocated in the near future. However, the performance gain you can
get by having reserveInitialCapacity(N) would be small, because oilpan
implements an allocator that can expand/shrink vectors without reallocating the
contents. (In non-oilpan, each expand/shrink involves a reallocation & copying
of the entire vector.)

- Unnecessary reserveInitialCapacity(N) regresses performance in oilpan.

In conclusion, in oilpan it is better to not call reserveInitialCapacity(N)
unless you're pretty sure that N elements are going to be allocated in the near
future.

[hayato, 2015-09-14 10:00:42]

https://codereview.chromium.org/1333813002/diff/40001/Source/core/dom/shadow/...
File Source/core/dom/shadow/DistributedNodes.h (right):

https://codereview.chromium.org/1333813002/diff/40001/Source/core/dom/shadow/...
Source/core/dom/shadow/DistributedNodes.h:45: // This is a performance
optimization on oilpan builds. Allocating
I'm afraid that this comment implies that pre-allocating a vector buffer is
*always* bad and should be avoided in Oilpan.

If that's not true, could you update this comment to tell why allocation a
vector buffer is bad *here*, in this particular case, mentioning the purpose of
*this* reserveInitialCapacity?

[haraken, 2015-09-14 11:47:12]

On 2015/09/14 10:00:42, hayato wrote:
>
https://codereview.chromium.org/1333813002/diff/40001/Source/core/dom/shadow/...
> File Source/core/dom/shadow/DistributedNodes.h (right):
> 
>
https://codereview.chromium.org/1333813002/diff/40001/Source/core/dom/shadow/...
> Source/core/dom/shadow/DistributedNodes.h:45: // This is a performance
> optimization on oilpan builds. Allocating
> I'm afraid that this comment implies that pre-allocating a vector buffer is
> *always* bad and should be avoided in Oilpan.
> 
> If that's not true, could you update this comment to tell why allocation a
> vector buffer is bad *here*, in this particular case, mentioning the purpose
of
> *this* reserveInitialCapacity?

Fixed the comment.

Please do clarify all the points you're not satisfied with in the next review.
This CL is blocking Oilpan's shipping.

[esprehn, 2015-09-14 15:37:41]

What I mean is
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

this reserve initial capacity in the constructor doesn't make sense, it should
be in ElementShadow::distribute when we create this on the stack since later
when we do setDistributeNodes we'll shrink it to the right size. Today the
default one in <content> is just higher peak memory until the first
distribution.

I'm not sure a huge comment is needed, you can just do

// Oilpan can handle allocations faster so it doesn't need this.
#if !ENABLE(OILPAN)
  distributedNodes.reserveCapacity(32);
#endif

in ElementShadow::distribute and remove the line from the constructor.

[haraken, 2015-09-14 23:26:19]

On 2015/09/14 15:37:41, esprehn wrote:
> What I mean is
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> this reserve initial capacity in the constructor doesn't make sense, it should
> be in ElementShadow::distribute when we create this on the stack since later
> when we do setDistributeNodes we'll shrink it to the right size. Today the
> default one in <content> is just higher peak memory until the first
> distribution.
> 
> I'm not sure a huge comment is needed, you can just do
> 
> // Oilpan can handle allocations faster so it doesn't need this.
> #if !ENABLE(OILPAN)
>   distributedNodes.reserveCapacity(32);
> #endif
> 
> in ElementShadow::distribute and remove the line from the constructor.

Doesn't this mean that you want to land
https://codereview.chromium.org/1257573002/?

I'm fine with going with Elliott's approach, but please help us land this CL
first. We don't want to delay Oilpan's shipping any more.

[esprehn, 2015-09-14 23:57:24]

On 2015/09/14 at 23:26:19, haraken wrote:
> On 2015/09/14 15:37:41, esprehn wrote:
> > What I mean is
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > this reserve initial capacity in the constructor doesn't make sense, it
should
> > be in ElementShadow::distribute when we create this on the stack since later
> > when we do setDistributeNodes we'll shrink it to the right size. Today the
> > default one in <content> is just higher peak memory until the first
> > distribution.
> > 
> > I'm not sure a huge comment is needed, you can just do
> > 
> > // Oilpan can handle allocations faster so it doesn't need this.
> > #if !ENABLE(OILPAN)
> >   distributedNodes.reserveCapacity(32);
> > #endif
> > 
> > in ElementShadow::distribute and remove the line from the constructor.
> 
> Doesn't this mean that you want to land
https://codereview.chromium.org/1257573002/?
>

Maybe, sizing to m_nodes is not the same as sizing to 32.
 
> I'm fine with going with Elliott's approach, but please help us land this CL
first. We don't want to delay Oilpan's shipping any more.

When do you plan to ship?

[haraken, 2015-09-15 00:26:42]

On 2015/09/14 23:57:24, esprehn wrote:
> On 2015/09/14 at 23:26:19, haraken wrote:
> > On 2015/09/14 15:37:41, esprehn wrote:
> > > What I mean is
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > this reserve initial capacity in the constructor doesn't make sense, it
> should
> > > be in ElementShadow::distribute when we create this on the stack since
later
> > > when we do setDistributeNodes we'll shrink it to the right size. Today the
> > > default one in <content> is just higher peak memory until the first
> > > distribution.
> > > 
> > > I'm not sure a huge comment is needed, you can just do
> > > 
> > > // Oilpan can handle allocations faster so it doesn't need this.
> > > #if !ENABLE(OILPAN)
> > >   distributedNodes.reserveCapacity(32);
> > > #endif
> > > 
> > > in ElementShadow::distribute and remove the line from the constructor.
> > 
> > Doesn't this mean that you want to land
> https://codereview.chromium.org/1257573002/
> >
> 
> Maybe, sizing to m_nodes is not the same as sizing to 32.
>  
> > I'm fine with going with Elliott's approach, but please help us land this CL
> first. We don't want to delay Oilpan's shipping any more.
> 
> When do you plan to ship?

As soon as possible (hopefully do the first attempt by the end of Sep). We're
now collecting full performance numbers.

[esprehn, 2015-09-15 00:29:41]

lgtm, please explain in your patch description the other change you made too. Is
that just never called?

https://codereview.chromium.org/1333813002/diff/60001/Source/core/dom/shadow/...
File Source/core/dom/shadow/DistributedNodes.h (left):

https://codereview.chromium.org/1333813002/diff/60001/Source/core/dom/shadow/...
Source/core/dom/shadow/DistributedNodes.h:63: const
WillBeHeapVector<RefPtrWillBeMember<Node>>& nodes() const { return m_nodes; }
unrelated?

[haraken, 2015-09-15 00:39:32]

On 2015/09/15 00:29:41, esprehn wrote:
> please explain in your patch description the other change you made too. Is
> that just never called?
> 
>
https://codereview.chromium.org/1333813002/diff/60001/Source/core/dom/shadow/...
> File Source/core/dom/shadow/DistributedNodes.h (left):
> 
>
https://codereview.chromium.org/1333813002/diff/60001/Source/core/dom/shadow/...
> Source/core/dom/shadow/DistributedNodes.h:63: const
> WillBeHeapVector<RefPtrWillBeMember<Node>>& nodes() const { return m_nodes; }
> unrelated?

Yeah, it's never called.

[haraken, 2015-09-15 00:40:09]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-09-15 00:40:09]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/1333813002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-09-15 00:40:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1333813002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1333813002/60001

[commit-bot: I haz the power, 2015-09-15 01:35:51]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202244

[hayato, 2015-09-15 04:44:33]

Is the benchmark result for Oilpan or non-Oilpan?

http://haraken.info/a/results.html

If that's for non-Oilpan, I would remove reserveInitialCapacity itself.

[haraken, 2015-09-15 04:45:45]

On 2015/09/15 04:44:33, hayato wrote:
> Is the benchmark result for Oilpan or non-Oilpan?
> 
> http://haraken.info/a/results.html
> 
> If that's for non-Oilpan, I would remove reserveInitialCapacity itself.

That's for non-oilpan.

[hayato, 2015-09-15 04:53:00]

On 2015/09/15 04:45:45, haraken wrote:
> On 2015/09/15 04:44:33, hayato wrote:
> > Is the benchmark result for Oilpan or non-Oilpan?
> > 
> > http://haraken.info/a/results.html
> > 
> > If that's for non-Oilpan, I would remove reserveInitialCapacity itself.
> 
> That's for non-oilpan.

Okay. I'll remove reserveInitalCapacity itself as another patch. It looks there
is no point keeping it.

What confused me is that you said that PS1 didn't have any performance impact of
the micro benchmarks in non-Oilpan, but PS1 actually changed the result of the
micro benchmark in non-Oilpan in a good manner.

[commit-bot: I haz the power, 2015-09-23 12:41:10]

Patchset 4 (id:??) landed as
https://crrev.com/c655d8c1a866eebe51f54f3d4803f3d3c19a523d