[oilpan]: Remove support for tracing off-heap hashmaps.

[oilpan]: Remove support for tracing off-heap hashmaps.

With this change we can no longer trace off-heap hashmaps. To make this work I changed the ExecutionContext to be WillBeHeapSupplementable and moved the IDBPendingTransactionMonitor to the heap. Finally I changed the way we trace through supplementables. Specifically we only trace our supplements for HeapSupplementable and PersistentHeapSupplementable.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org, sigbjornf@opera.com
BUG=

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

[sof, 2014-07-21 13:43:16]

lgtm. Would be good to get a 2nd pair of Oilpan-trained eyes to look it over.

https://codereview.chromium.org/403333002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.h (right):

https://codereview.chromium.org/403333002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBPendingTransactionMonitor.h:60: typedef
HeapVector<Member<IDBTransaction> > TransactionList;
nit: seems like this is only used for the field below; I'd be fine with seeing
it inlined (but don't think it is worth adding a transition type for it.)

https://codereview.chromium.org/403333002/diff/20001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/403333002/diff/20001/Source/platform/Suppleme...
Source/platform/Supplementable.h:255: // No tracing of off-heap supplements. We
should not have any Supplementable
I think it is unlikely that this slips through, but worth strengthening with an
assert of some kind?

[wibling-chromium, 2014-07-21 14:37:43]

Thanks for the review! I have updated the diff.

https://codereview.chromium.org/403333002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.h (right):

https://codereview.chromium.org/403333002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBPendingTransactionMonitor.h:60: typedef
HeapVector<Member<IDBTransaction> > TransactionList;
On 2014/07/21 13:43:15, sof wrote:
> nit: seems like this is only used for the field below; I'd be fine with seeing
> it inlined (but don't think it is worth adding a transition type for it.)

Done. I inlined it. I agree it is not worth adding a transition type for unless
we have more instances.

https://codereview.chromium.org/403333002/diff/20001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/403333002/diff/20001/Source/platform/Suppleme...
Source/platform/Supplementable.h:255: // No tracing of off-heap supplements. We
should not have any Supplementable
On 2014/07/21 13:43:15, sof wrote:
> I think it is unlikely that this slips through, but worth strengthening with
an
> assert of some kind?

Good idea. I added a compile assert checking that T is not garbage collected
when this trace method is instantiated.

[haraken, 2014-07-21 14:47:05]

LGTM

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp (right):

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp:73:
WillBeHeapSupplement<ExecutionContext>::trace(visitor); }

'}' should be written in the next line.

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.h (right):

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBPendingTransactionMonitor.h:60:
HeapVector<Member<IDBTransaction> > m_transactions;

Shall we introduce PersistentHeapVectorWillBeHeapVector? We already have
PersistentHeapHashSetWillBeHeapHashSet and PersistentHeapDequeWillBeHeapDeque.

[tkent, 2014-07-22 03:25:14]

platform/ lgtm.

[wibling-chromium, 2014-07-22 08:24:34]

Thanks for the reviews! I have updated the diffs.

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp (right):

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp:73:
WillBeHeapSupplement<ExecutionContext>::trace(visitor); }
On 2014/07/21 14:47:04, haraken wrote:
> 
> '}' should be written in the next line.

Done.

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.h (right):

https://codereview.chromium.org/403333002/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBPendingTransactionMonitor.h:60:
HeapVector<Member<IDBTransaction> > m_transactions;
On 2014/07/21 14:47:04, haraken wrote:
> 
> Shall we introduce PersistentHeapVectorWillBeHeapVector? We already have
> PersistentHeapHashSetWillBeHeapHashSet and PersistentHeapDequeWillBeHeapDeque.

Done.

[wibling-chromium, 2014-07-22 08:24:41]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-07-22 08:24:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/403333002/60001

[wibling-chromium, 2014-07-22 09:26:59]

The CQ bit was unchecked by wibling@chromium.org

[Erik Corry, 2014-07-22 09:37:51]

lgtm

https://codereview.chromium.org/403333002/diff/60001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/403333002/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:161: 
Could we add a comment here a la:
// Helper class used to implement HeapSupplement, Supplement, Supplementable and
PersistentHeapSupplementable.

And perhaps a 1-liner on each of them indicating when they are appropriate to
use.

https://codereview.chromium.org/403333002/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:189: }
I think this should have a virtual trace method to keep with our principle that
these are always present. It could contain a comment a la:
// m_supplements is actually traced in subclasses (where indicated by the
isGarbageCollected template parameter) because methods cannot be partially
specialized.

[wibling-chromium, 2014-07-22 11:03:42]

Thanks for the review. I have updated the diff.

https://codereview.chromium.org/403333002/diff/60001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/403333002/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:161: 
On 2014/07/22 09:37:51, Erik Corry wrote:
> Could we add a comment here a la:
> // Helper class used to implement HeapSupplement, Supplement, Supplementable
and
> PersistentHeapSupplementable.
> 
> And perhaps a 1-liner on each of them indicating when they are appropriate to
> use.

Done.

https://codereview.chromium.org/403333002/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:189: }
On 2014/07/22 09:37:51, Erik Corry wrote:
> I think this should have a virtual trace method to keep with our principle
that
> these are always present. It could contain a comment a la:
> // m_supplements is actually traced in subclasses (where indicated by the
> isGarbageCollected template parameter) because methods cannot be partially
> specialized.

Done. It does require me to add back the GC_PLUGIN_IGNORE, but I think it is
better to at least keep the pattern that the top most class has a vtable if a
subclass has it to ensure we can check whether it is initialized. If we didn't
have the virtual trace here the first word of the object would be the map.

[wibling-chromium, 2014-07-22 11:04:43]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-07-22 11:05:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/403333002/80001

[wibling-chromium, 2014-07-22 11:26:15]

The CQ bit was unchecked by wibling@chromium.org

[wibling-chromium, 2014-07-22 11:42:33]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-07-22 11:43:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/403333002/100001

[commit-bot: I haz the power, 2014-07-22 12:52:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[commit-bot: I haz the power, 2014-07-22 13:18:55]

Change committed as 178648

[johnme, 2014-07-22 14:19:35]

A revert of this CL has been created in
https://codereview.chromium.org/409113003/ by johnme@chromium.org.

The reason for reverting is: Seems to have broken compile across the tree..