Oilpan: HeapVectorBacking should call destructors for its elements

Oilpan: HeapVectorBacking should call destructors for its elements

Currently HeapVectorBacking does not call destructors of its elements
and is leaking the objects. We must call them.

R=tkent
BUG=475801
TEST=HeapTest.VectorDestructors
TEST=HeapTest.DequePartObjectsExpand
TEST=HeapTest.HeapVectorPartObjects

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

[haraken, 2015-04-18 23:48:46]

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

[haraken, 2015-04-18 23:48:47]

LGTM

tkent-san for wtf/.

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

https://codereview.chromium.org/1086413003/diff/40001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:1290: for (const T* bufferEntry = bufferBegin; bufferEntry
!= bufferEnd; bufferEntry++)

Here would it be helpful to insert ASSERTs to verify that the elements between
[size(), capacity()] are correctly cleared out?

[sof, 2015-04-19 20:10:52]

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

https://codereview.chromium.org/1086413003/diff/40001/Source/wtf/Vector.h#new...
Source/wtf/Vector.h:1290: for (const T* bufferEntry = bufferBegin; bufferEntry
!= bufferEnd; bufferEntry++)
On 2015/04/18 23:48:47, haraken wrote:
> 
> Here would it be helpful to insert ASSERTs to verify that the elements between
> [size(), capacity()] are correctly cleared out?

Excellent suggestion. Added & by doing so I came across two places where buffer
slots ought to have been cleared, but were not: for swap() and resize().

I've added clearing to both of them here; if it is preferred to have this be
split up some, I can do that.

Notice that the failure to clear upon resize() is behind what crbug.com/469111
runs into.

[haraken, 2015-04-19 23:47:17]

On 2015/04/19 20:10:52, sof wrote:
> https://codereview.chromium.org/1086413003/diff/40001/Source/wtf/Vector.h
> File Source/wtf/Vector.h (right):
> 
>
https://codereview.chromium.org/1086413003/diff/40001/Source/wtf/Vector.h#new...
> Source/wtf/Vector.h:1290: for (const T* bufferEntry = bufferBegin; bufferEntry
> != bufferEnd; bufferEntry++)
> On 2015/04/18 23:48:47, haraken wrote:
> > 
> > Here would it be helpful to insert ASSERTs to verify that the elements
between
> > [size(), capacity()] are correctly cleared out?
> 
> Excellent suggestion. Added & by doing so I came across two places where
buffer
> slots ought to have been cleared, but were not: for swap() and resize().
> 
> I've added clearing to both of them here; if it is preferred to have this be
> split up some, I can do that.
> 
> Notice that the failure to clear upon resize() is behind what crbug.com/469111
> runs into.

Still LGTM

[tkent, 2015-04-20 00:16:47]

Does this have runtime performance impact?

[haraken, 2015-04-20 00:19:53]

On 2015/04/20 00:16:47, tkent wrote:
> Does this have runtime performance impact?

Worth measuring.

In theory:

- Either way, this CL is needed to fix leaks.
- The only vectors that are affected by this CL are the ones that contain
ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure if there
is any performance-sensitive vector in those vectors. (As far as I know,
HeapVector<RuleData> is the only performance-sensitive one but RuleData doesn't
require a destructor.)

[tkent, 2015-04-20 00:22:50]

On 2015/04/20 00:19:53, haraken wrote:
> On 2015/04/20 00:16:47, tkent wrote:
> > Does this have runtime performance impact?
> 
> Worth measuring.
> 
> In theory:
> 
> - Either way, this CL is needed to fix leaks.
> - The only vectors that are affected by this CL are the ones that contain
> ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure if
there
> is any performance-sensitive vector in those vectors. (As far as I know,
> HeapVector<RuleData> is the only performance-sensitive one but RuleData
doesn't
> require a destructor.)

I'm worry about clearUnusedSlot impact rather than desturctors.

[sof, 2015-04-20 05:12:16]

On 2015/04/20 00:22:50, tkent wrote:
> On 2015/04/20 00:19:53, haraken wrote:
> > On 2015/04/20 00:16:47, tkent wrote:
> > > Does this have runtime performance impact?
> > 
> > Worth measuring.
> > 
> > In theory:
> > 
> > - Either way, this CL is needed to fix leaks.
> > - The only vectors that are affected by this CL are the ones that contain
> > ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure if
> there
> > is any performance-sensitive vector in those vectors. (As far as I know,
> > HeapVector<RuleData> is the only performance-sensitive one but RuleData
> doesn't
> > require a destructor.)
> 
> I'm worry about clearUnusedSlot impact rather than desturctors.

Potentially, vec.resize(0) now performs an extra memset(.., 0, ...); in
particular.

Will run blink_perf and look for signs.

[sof, 2015-04-20 10:00:37]

On 2015/04/20 05:12:16, sof wrote:
> On 2015/04/20 00:22:50, tkent wrote:
> > On 2015/04/20 00:19:53, haraken wrote:
> > > On 2015/04/20 00:16:47, tkent wrote:
> > > > Does this have runtime performance impact?
> > > 
> > > Worth measuring.
> > > 
> > > In theory:
> > > 
> > > - Either way, this CL is needed to fix leaks.
> > > - The only vectors that are affected by this CL are the ones that contain
> > > ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure if
> > there
> > > is any performance-sensitive vector in those vectors. (As far as I know,
> > > HeapVector<RuleData> is the only performance-sensitive one but RuleData
> > doesn't
> > > require a destructor.)
> > 
> > I'm worry about clearUnusedSlot impact rather than desturctors.
> 
> Potentially, vec.resize(0) now performs an extra memset(.., 0, ...); in
> particular.
> 
> Will run blink_perf and look for signs.

No discernible regressions there (events and shadow_dom was the two I was most
interested in),

 
https://docs.google.com/spreadsheets/d/1zO-ZRG-XqFrzsNIEc7ri25GYpl47fbOSQYBLA...

[haraken, 2015-04-20 10:01:55]

On 2015/04/20 00:22:50, tkent wrote:
> On 2015/04/20 00:19:53, haraken wrote:
> > On 2015/04/20 00:16:47, tkent wrote:
> > > Does this have runtime performance impact?
> > 
> > Worth measuring.
> > 
> > In theory:
> > 
> > - Either way, this CL is needed to fix leaks.
> > - The only vectors that are affected by this CL are the ones that contain
> > ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure if
> there
> > is any performance-sensitive vector in those vectors. (As far as I know,
> > HeapVector<RuleData> is the only performance-sensitive one but RuleData
> doesn't
> > require a destructor.)
> 
> I'm worry about clearUnusedSlot impact rather than desturctors.

Note that clearUnusedSlot is no-op for "normal" vectors. Only vectors that
contain ALLOW_ONLY_INLINE_ALLOCATION items that need destructors will be
affected by this change.

[sof, 2015-04-20 10:29:40]

On 2015/04/20 10:01:55, haraken wrote:
> On 2015/04/20 00:22:50, tkent wrote:
> > On 2015/04/20 00:19:53, haraken wrote:
> > > On 2015/04/20 00:16:47, tkent wrote:
> > > > Does this have runtime performance impact?
> > > 
> > > Worth measuring.
> > > 
> > > In theory:
> > > 
> > > - Either way, this CL is needed to fix leaks.
> > > - The only vectors that are affected by this CL are the ones that contain
> > > ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure if
> > there
> > > is any performance-sensitive vector in those vectors. (As far as I know,
> > > HeapVector<RuleData> is the only performance-sensitive one but RuleData
> > doesn't
> > > require a destructor.)
> > 
> > I'm worry about clearUnusedSlot impact rather than desturctors.
> 
> Note that clearUnusedSlot is no-op for "normal" vectors. Only vectors that
> contain ALLOW_ONLY_INLINE_ALLOCATION items that need destructors will be
> affected by this change.

clearUnusedSlots() will be run for traceable references, right?

[haraken, 2015-04-20 11:51:23]

On 2015/04/20 10:29:40, sof wrote:
> On 2015/04/20 10:01:55, haraken wrote:
> > On 2015/04/20 00:22:50, tkent wrote:
> > > On 2015/04/20 00:19:53, haraken wrote:
> > > > On 2015/04/20 00:16:47, tkent wrote:
> > > > > Does this have runtime performance impact?
> > > > 
> > > > Worth measuring.
> > > > 
> > > > In theory:
> > > > 
> > > > - Either way, this CL is needed to fix leaks.
> > > > - The only vectors that are affected by this CL are the ones that
contain
> > > > ALLOW_ONLY_INLINE_ALLOCATION items that need destructors. I'm not sure
if
> > > there
> > > > is any performance-sensitive vector in those vectors. (As far as I know,
> > > > HeapVector<RuleData> is the only performance-sensitive one but RuleData
> > > doesn't
> > > > require a destructor.)
> > > 
> > > I'm worry about clearUnusedSlot impact rather than desturctors.
> > 
> > Note that clearUnusedSlot is no-op for "normal" vectors. Only vectors that
> > contain ALLOW_ONLY_INLINE_ALLOCATION items that need destructors will be
> > affected by this change.
> 
> clearUnusedSlots() will be run for traceable references, right?

ah, you're right.

[tkent, 2015-04-20 22:43:53]

> No discernible regressions there (events and shadow_dom was the two I was most
> interested in),
> 
>  
>
https://docs.google.com/spreadsheets/d/1zO-ZRG-XqFrzsNIEc7ri25GYpl47fbOSQYBLA...

Thanks you.  LGTM

[tkent, 2015-04-20 22:43:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2015-04-20 22:44:49]

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

[commit-bot: I haz the power, 2015-04-20 23:30:58]

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

[commit-bot: I haz the power, 2015-04-20 23:30:59]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52674)

[sof, 2015-04-21 06:12:23]

On 2015/04/20 22:43:53, tkent wrote:
> > No discernible regressions there (events and shadow_dom was the two I was
most
> > interested in),
> > 
> >  
> >
>
https://docs.google.com/spreadsheets/d/1zO-ZRG-XqFrzsNIEc7ri25GYpl47fbOSQYBLA...
> 
> Thanks you.  LGTM

Thanks; local testing with ASan didn't show up anything, so let's try this.

(Had to resolve trivial (re)naming conflicts, hence the ps#8 rebase.)

[sof, 2015-04-21 06:12:26]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-04-21 06:12:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1086413003/#ps140001
(title: "rebased past r194046 (upto r194085.)")

[commit-bot: I haz the power, 2015-04-21 06:12:54]

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

[commit-bot: I haz the power, 2015-04-21 10:09:35]

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