Oilpan: promptly free per-frame document heap vector.

Oilpan: promptly free per-frame document heap vector.

The per-frame updating of animations create a small vector
on the heap each time around. Most of the time the update checking
will not trigger additional heap allocations (or work), so by promptly
releasing the heap vector, the allocation pointer can simply be reset.

R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191615

[sof, 2015-03-09 10:24:05]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-03-09 10:24:06]

Please take a look/consider.

I haven't gauged the effect, but it seems like needless creation of garbage.

[haraken, 2015-03-09 11:02:14]

LGTM

My previous experiment of adding destructors to HeapVectors failed because of
the overhead of calling the destructors. A possible option would be to call a
destructor only for a large vector, but as far as I quickly experimented, it
looks like even the if branch to check the vector size is negligible...

[sof, 2015-03-10 08:01:07]

On 2015/03/09 11:02:14, haraken wrote:
> LGTM
> 
> My previous experiment of adding destructors to HeapVectors failed because of
> the overhead of calling the destructors. A possible option would be to call a
> destructor only for a large vector, but as far as I quickly experimented, it
> looks like even the if branch to check the vector size is negligible...

It could be restricted in other ways, I suppose:

 - only clear the vector/deque if the elements don't require finalization (e.g.,
Member<>)
 - only promptly free if the allocation point can be reset.

(For this CL, I'm checking that there aren't performance regressions.)

[sof, 2015-03-10 10:12:20]

On 2015/03/10 08:01:07, sof wrote:
> On 2015/03/09 11:02:14, haraken wrote:
> > LGTM
> > 
> > My previous experiment of adding destructors to HeapVectors failed because
of
> > the overhead of calling the destructors. A possible option would be to call
a
> > destructor only for a large vector, but as far as I quickly experimented, it
> > looks like even the if branch to check the vector size is negligible...
> 
> It could be restricted in other ways, I suppose:
> 
>  - only clear the vector/deque if the elements don't require finalization
(e.g.,
> Member<>)
>  - only promptly free if the allocation point can be reset.
> 
> (For this CL, I'm checking that there aren't performance regressions.)

blink_perf.layout (Windows) results are too noisy to be reliable -- one set of
runs points in one direction, the next one the other way --  but for other tests
that do exercise this per-frame code a bit, I see no slowdowns (or large gains.)
speedometer is improving slightly on jQuery-ToDoMVC, so I think this is worth
doing overall.

[sof, 2015-03-10 10:12:26]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-03-10 10:13:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/994483002/1

[commit-bot: I haz the power, 2015-03-10 11:30:07]

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

[dstockwell, 2015-03-10 22:38:30]

dstockwell@chromium.org changed reviewers:
+ dstockwell@chromium.org, haraken@chromium.org

[dstockwell, 2015-03-10 22:38:31]

https://codereview.chromium.org/994483002/diff/1/Source/core/page/PageAnimato...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/994483002/diff/1/Source/core/page/PageAnimato...
Source/core/page/PageAnimator.cpp:73: documents.clear();
This might be a reasonable optimiziation, but to a non-oilpan expert it's not
clear what the purpose is. Once the oilpan if-defs are removed there's nothing
to prevent someone from cleaning this up. Is there something we can do to
improve the readability here? Maybe a different named HeapVector?

[haraken, 2015-03-10 23:26:08]

https://codereview.chromium.org/994483002/diff/1/Source/core/page/PageAnimato...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/994483002/diff/1/Source/core/page/PageAnimato...
Source/core/page/PageAnimator.cpp:73: documents.clear();
On 2015/03/10 22:38:31, dstockwell wrote:
> This might be a reasonable optimiziation, but to a non-oilpan expert it's not
> clear what the purpose is. Once the oilpan if-defs are removed there's nothing
> to prevent someone from cleaning this up. Is there something we can do to
> improve the readability here? Maybe a different named HeapVector?

Yeah, that's something we're considering.

Option 1: Introduce StackVector.

Option 2: Add a destructor for an on-stack HeapVector so that the HeapVector is
promptly cleared. (But I'm failing at implementing the destructor in a way in
which it doesn't regress performance.)

Option 3: Explicitly add .clear() with a comment.