Make sure all destructors are called in HeapHashMap and HeapHashSet

Make sure all destructors are called in HeapHashMap and HeapHashSet

BUG=357113
R=ager@chromium.org, haraken@chromium.org, wibling@chromium.org

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

[wibling-chromium, 2014-03-28 10:35:22]

lgtm

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:2281: if (m_x != emptyValue)
Does the constructor get called more than once?

[haraken, 2014-03-28 10:43:36]

LGTM

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

https://codereview.chromium.org/216723002/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:910: // again when the GC finds the backing store. With
the default

This comment was a bit confusing to me at first. Maybe slightly better: "A
deleted bucket is marked as deleted in the weak processing phase and finalized
in the sweeping phase."

[Mads Ager (chromium), 2014-03-28 11:12:38]

LGTM

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:3189: class Fluff {
Ehm, move meaningful names, please?

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:3202: TEST(HeapTest, Flaf)
DestructorsCalledOnMapClear?

[Erik Corry, 2014-03-30 20:11:36]

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:2281: if (m_x != emptyValue)
On 2014/03/28 10:35:23, wibling-chromium wrote:
> Does the constructor get called more than once?

Thanks for pointing this out.  There was a bug in the destructor in HashTable.h
that was skipping deleted buckets, but calling the destructor for empty objects.
 Now fixed, and the test has been changed to keep track of both births and
deaths of objects.

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:3189: class Fluff {
On 2014/03/28 11:12:39, Mads Ager (chromium) wrote:
> Ehm, move meaningful names, please?

Done.

https://codereview.chromium.org/216723002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:3202: TEST(HeapTest, Flaf)
On 2014/03/28 11:12:39, Mads Ager (chromium) wrote:
> DestructorsCalledOnMapClear?

Done.

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

https://codereview.chromium.org/216723002/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:910: // again when the GC finds the backing store. With
the default
On 2014/03/28 10:43:36, haraken wrote:
> 
> This comment was a bit confusing to me at first. Maybe slightly better: "A
> deleted bucket is marked as deleted in the weak processing phase and finalized
> in the sweeping phase."

This code is not related to weakness.  It is called when the hashtable is
cleared or rehashed due to size changes.  At this point the backing store is
freed.  In the GC heap, freeing it just done by no longer pointing at this
backing store, which means it will eventually be collected.  We want to run the
destructors of the entries eagerly on clear() and rehash, but we have to make
sure they are not run again when hte backing is eventually collected.

I'll try to make this clear in the comment.

[haraken, 2014-03-31 00:34:54]

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

https://codereview.chromium.org/216723002/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:910: // again when the GC finds the backing store. With
the default
On 2014/03/30 20:11:36, Erik Corry wrote:
> On 2014/03/28 10:43:36, haraken wrote:
> > 
> > This comment was a bit confusing to me at first. Maybe slightly better: "A
> > deleted bucket is marked as deleted in the weak processing phase and
finalized
> > in the sweeping phase."
> 
> This code is not related to weakness.  It is called when the hashtable is
> cleared or rehashed due to size changes.  At this point the backing store is
> freed.  In the GC heap, freeing it just done by no longer pointing at this
> backing store, which means it will eventually be collected.  We want to run
the
> destructors of the entries eagerly on clear() and rehash, but we have to make
> sure they are not run again when hte backing is eventually collected.
> 
> I'll try to make this clear in the comment.

Thanks, now I understand.

BTW, currently we don't allow rehashing in a destructor of on-heap hash
maps/sets, because allocation was not allowed in the destructor. However, now
that allocation is allowed in the destructor, shall we enable the rehashing
again?

[Erik Corry, 2014-03-31 06:38:30]

If we make that change it should be based on a concrete case where we need it.

[wibling-chromium, 2014-03-31 09:15:39]

lgtm

[haraken, 2014-03-31 09:22:00]

On 2014/03/31 06:38:30, Erik Corry wrote:
> If we make that change it should be based on a concrete case where we need it.

I think we've removed the rehashing code from destructors of on-heap hash
sets/maps, because having the rehashing code caused actual crashes in layout
tests. If you enable the rehashing and run layout tests, I think you can get
actual code that rehashes in destructors.

I don't think that fixing the behavior is a P1 task, but it sounds weird to
delay rehashing until a next insertion/removal happens on the hash sets/maps.

[Erik Corry, 2014-03-31 09:29:17]

On 2014/03/31 09:22:00, haraken wrote:
> On 2014/03/31 06:38:30, Erik Corry wrote:
> > If we make that change it should be based on a concrete case where we need
it.
> 
> I think we've removed the rehashing code from destructors of on-heap hash
> sets/maps, because having the rehashing code caused actual crashes in layout
> tests. If you enable the rehashing and run layout tests, I think you can get
> actual code that rehashes in destructors.
> 
> I don't think that fixing the behavior is a P1 task, but it sounds weird to
> delay rehashing until a next insertion/removal happens on the hash sets/maps.

We still can't allocate during weak processing, so we have to have the delayed
rehashing for that case.

During destructor running you can't touch other heap objects, so there is no
issue of causing rehashing of on-heap hash tables.  The hash table backing
itself can delete entries during destruction, and does so if the keys or values
have destructors (currently buggy, but fix in review now), but it has no need to
rehash since it is dead after the destructor runs.

[haraken, 2014-03-31 09:36:24]

On 2014/03/31 09:29:17, Erik Corry wrote:
> On 2014/03/31 09:22:00, haraken wrote:
> > On 2014/03/31 06:38:30, Erik Corry wrote:
> > > If we make that change it should be based on a concrete case where we need
> it.
> > 
> > I think we've removed the rehashing code from destructors of on-heap hash
> > sets/maps, because having the rehashing code caused actual crashes in layout
> > tests. If you enable the rehashing and run layout tests, I think you can get
> > actual code that rehashes in destructors.
> > 
> > I don't think that fixing the behavior is a P1 task, but it sounds weird to
> > delay rehashing until a next insertion/removal happens on the hash
sets/maps.
> 
> We still can't allocate during weak processing, so we have to have the delayed
> rehashing for that case.
> 
> During destructor running you can't touch other heap objects, so there is no
> issue of causing rehashing of on-heap hash tables.  The hash table backing
> itself can delete entries during destruction, and does so if the keys or
values
> have destructors (currently buggy, but fix in review now), but it has no need
to
> rehash since it is dead after the destructor runs.

ah, understand. The issue we observed before was rehashing during weak
processing, not rehashing during destructors. Sorry about the noise.

[jochen (gone - plz use gerrit), 2014-03-31 09:56:19]

lgtm

[Erik Corry, 2014-03-31 11:43:36]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-03-31 11:43:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/216723002/20001

[commit-bot: I haz the power, 2014-03-31 12:55:07]

Change committed as 170443