Oilpan: Prepare to move AbstractSQLTransactionBackend, SQLTransactionBackend, and SQLTransactionCoo…

Oilpan: Prepare to move AbstractSQLTransactionBackend, SQLTransactionBackend, and SQLTransactionCoordinator::CoordinationInfo to Oilpan heap.

This CL changes the mapped type of m_coordinationInfoMap from copyable
object to OwnPtr<> or Member<>. So we need to update some code
in SQLTransactionCoordinator.

BUG=347902
R=ager@chromium.org, haraken@chromium.org

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

[tkent, 2014-03-17 06:23:17]

please review.

[haraken, 2014-03-18 13:45:20]

Sorry, this CL has slipped through my filter again. (I noticed that something
was wrong with my gmail filter.)

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseBackend.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseBackend.h:74:
Deque<RefPtrWillBeMember<SQLTransactionBackend> > m_transactionQueue;

It's OK in this CL but we should implement HeapDeque in the near future.

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/S...
File Source/modules/webdatabase/SQLTransactionCoordinator.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLTransactionCoordinator.h:69: typedef
WillBePersistentHeapHashMap<String, OwnPtrWillBeMember<CoordinationInfo> >
CoordinationInfoHeapMap;

Why did you change CoordinaitonInfo to OwnPtrWillBeMember<CoordinationInfo>?

Probably you can keep CoordinaitonInfo as is and add
ALLOW_ONLY_INLINE_ALLOCATION to CoordinaitonInfo.

[Mads Ager (chromium), 2014-03-18 13:57:10]

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
File Source/modules/webdatabase/AbstractSQLTransactionBackend.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
Source/modules/webdatabase/AbstractSQLTransactionBackend.h:41: class
AbstractSQLTransactionBackend : public
RefCountedWillBeGarbageCollectedFinalized<AbstractSQLTransactionBackend> {
Is it OK to drop ThreadSafe here?

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseBackend.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseBackend.h:74:
Deque<RefPtrWillBeMember<SQLTransactionBackend> > m_transactionQueue;
On 2014/03/18 13:45:20, haraken wrote:
> 
> It's OK in this CL but we should implement HeapDeque in the near future.

Right, Erik is getting close to having HeapLinkHashSet implemented. HeapDeque is
next. Once those are implemented we will probably remove the support for tracing
non-heap collections.

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/S...
File Source/modules/webdatabase/SQLTransactionCoordinator.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLTransactionCoordinator.h:69: typedef
WillBePersistentHeapHashMap<String, OwnPtrWillBeMember<CoordinationInfo> >
CoordinationInfoHeapMap;
On 2014/03/18 13:45:20, haraken wrote:
> 
> Why did you change CoordinaitonInfo to OwnPtrWillBeMember<CoordinationInfo>?
> 
> Probably you can keep CoordinaitonInfo as is and add
> ALLOW_ONLY_INLINE_ALLOCATION to CoordinaitonInfo.

Yes, value objects in heap collections are traced if they have a trace method.
So, as long as you implement the trace method in CoordinationInfo and mark it as
ALLOW_ONLY_INLINE_ALLOCATION that will work.

[tkent, 2014-03-19 04:44:56]

I uploaded new patch.

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
File Source/modules/webdatabase/AbstractSQLTransactionBackend.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
Source/modules/webdatabase/AbstractSQLTransactionBackend.h:41: class
AbstractSQLTransactionBackend : public
RefCountedWillBeGarbageCollectedFinalized<AbstractSQLTransactionBackend> {
On 2014/03/18 13:57:10, Mads Ager (chromium) wrote:
> Is it OK to drop ThreadSafe here?

Yes.  ThreadSafeRefCounted means RefCountedBase::m_refCount can be accessed from
multiple threads. In this case, a context thread and a database thread.
GarbageCollected(Finalized) doesn't have m_refCount.

CrossThreadPersistent<SQLTransactionBackend> in DatabaseTask.h represents
(Abstract)SQLTransactionBackend can be shared in multiple threads.

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/S...
File Source/modules/webdatabase/SQLTransactionCoordinator.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLTransactionCoordinator.h:69: typedef
WillBePersistentHeapHashMap<String, OwnPtrWillBeMember<CoordinationInfo> >
CoordinationInfoHeapMap;
On 2014/03/18 13:57:10, Mads Ager (chromium) wrote:
> On 2014/03/18 13:45:20, haraken wrote:
> > 
> > Why did you change CoordinaitonInfo to OwnPtrWillBeMember<CoordinationInfo>?
> > 
> > Probably you can keep CoordinaitonInfo as is and add
> > ALLOW_ONLY_INLINE_ALLOCATION to CoordinaitonInfo.
> 
> Yes, value objects in heap collections are traced if they have a trace method.
> So, as long as you implement the trace method in CoordinationInfo and mark it
as
> ALLOW_ONLY_INLINE_ALLOCATION that will work.

Done.

We need a documentation of ALLOW_ONLY_INLINE_ALLOCATION.

[tkent, 2014-03-19 04:46:50]

https://codereview.chromium.org/198213005/diff/20001/Source/heap/Handle.h
File Source/heap/Handle.h (left):

https://codereview.chromium.org/198213005/diff/20001/Source/heap/Handle.h#old...
Source/heap/Handle.h:433: ASSERT(!raw ||
ThreadStateFor<ThreadingTrait<T>::Affinity>::state()->contains(raw));
This assertion is too strict.
A SQLTransaction object is created in main/worker thread, and can be stored in a
Member<> in a database thread.

[haraken, 2014-03-19 04:59:20]

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionBackend.h (right):

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackend.h:112:
RefPtrWillBeMember<AbstractSQLTransaction> m_frontend; // Has a reference cycle,
and will break in doCleanup().

Who guarantees that the thread that created the SQLTransaction is alive while
the database thread is holding the SQLTransaction?

I think it's safe to store "an on-heap object created from one thread" into "a
member of another thread" only if it's guaranteed that the creation thread
outlives the thread that holds the member. Is there any way to assert the
guarantee?

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionCoordinator.h (right):

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionCoordinator.h:66:
ALLOW_ONLY_INLINE_ALLOCATION();

Nit: We normally put this at the beginning of the struct:

struct CoordinationInfo {
  ALLOW_ONLY_INLINE_ALLOCATION();
public:
  ...;
};

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionCoordinator.h:69: typedef
WillBePersistentHeapHashMap<String, CoordinationInfo> CoordinationInfoHeapMap;

Mads, Erik: We don't need to add any Traits for CoordinationInfo when it's used
in a hash map, right?

c.f.: We need to define a VectorTrait if CoordinationInfo is used in a vector:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[tkent, 2014-03-19 05:15:12]

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionBackend.h (right):

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackend.h:112:
RefPtrWillBeMember<AbstractSQLTransaction> m_frontend; // Has a reference cycle,
and will break in doCleanup().
On 2014/03/19 04:59:20, haraken wrote:
> 
> Who guarantees that the thread that created the SQLTransaction is alive while
> the database thread is holding the SQLTransaction?
> 
> I think it's safe to store "an on-heap object created from one thread" into "a
> member of another thread" only if it's guaranteed that the creation thread
> outlives the thread that holds the member. Is there any way to assert the
> guarantee?

It's guaranteed.  A main/worker thread creates a dedicated database thread, and
it waits for  the database thread termination at [1].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Mads Ager (chromium), 2014-03-19 06:30:53]

LGTM with the ThreadSafeRefCounted comment addressed.

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
File Source/modules/webdatabase/AbstractSQLTransactionBackend.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
Source/modules/webdatabase/AbstractSQLTransactionBackend.h:41: class
AbstractSQLTransactionBackend : public
RefCountedWillBeGarbageCollectedFinalized<AbstractSQLTransactionBackend> {
On 2014/03/19 04:44:56, tkent wrote:
> On 2014/03/18 13:57:10, Mads Ager (chromium) wrote:
> > Is it OK to drop ThreadSafe here?
> 
> Yes.  ThreadSafeRefCounted means RefCountedBase::m_refCount can be accessed
from
> multiple threads. In this case, a context thread and a database thread.
> GarbageCollected(Finalized) doesn't have m_refCount.
> 
> CrossThreadPersistent<SQLTransactionBackend> in DatabaseTask.h represents
> (Abstract)SQLTransactionBackend can be shared in multiple threads.

Yes, I know. However, you are using RefCountedWillBeGarbageCollectedFinalized so
this is RefCounted and not ThreadSafeRefCounted in the non-oilpan build. It
should remain ThreadSafeRefCounted in the non-oilpan build because it is
accessed from both context and database thread, right?

https://codereview.chromium.org/198213005/diff/20001/Source/heap/Handle.h
File Source/heap/Handle.h (left):

https://codereview.chromium.org/198213005/diff/20001/Source/heap/Handle.h#old...
Source/heap/Handle.h:433: ASSERT(!raw ||
ThreadStateFor<ThreadingTrait<T>::Affinity>::state()->contains(raw));
On 2014/03/19 04:46:50, tkent wrote:
> This assertion is too strict.
> A SQLTransaction object is created in main/worker thread, and can be stored in
a
> Member<> in a database thread.

Yes, that is true. I needed to relax the assert in the Persistent case for the
CrossThreadPersistents. We cannot do a similar relaxation here. What we would
have to check here is that the pointer is in one of the thread heaps. However,
we cannot do that unless we stop all threads to do it. :(

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionCoordinator.h (right):

https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionCoordinator.h:69: typedef
WillBePersistentHeapHashMap<String, CoordinationInfo> CoordinationInfoHeapMap;
On 2014/03/19 04:59:20, haraken wrote:
> 
> Mads, Erik: We don't need to add any Traits for CoordinationInfo when it's
used
> in a hash map, right?
> 
> c.f.: We need to define a VectorTrait if CoordinationInfo is used in a vector:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That is correct, you don't need the traits for HashMaps. They are needed for
Vectors of value objects to ensure that the value objects are not destructed
twice and we have made it a compilation error if the value object does not allow
initialization memset and copying with memcpy. For HashMaps we do not have that
issue because a HashMap knows if entries are deleted or empty values.

[tkent, 2014-03-19 06:33:46]

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
File Source/modules/webdatabase/AbstractSQLTransactionBackend.h (right):

https://codereview.chromium.org/198213005/diff/1/Source/modules/webdatabase/A...
Source/modules/webdatabase/AbstractSQLTransactionBackend.h:41: class
AbstractSQLTransactionBackend : public
RefCountedWillBeGarbageCollectedFinalized<AbstractSQLTransactionBackend> {
On 2014/03/19 06:30:54, Mads Ager (chromium) wrote:
> On 2014/03/19 04:44:56, tkent wrote:
> > On 2014/03/18 13:57:10, Mads Ager (chromium) wrote:
> > > Is it OK to drop ThreadSafe here?
> > 
> > Yes.  ThreadSafeRefCounted means RefCountedBase::m_refCount can be accessed
> from
> > multiple threads. In this case, a context thread and a database thread.
> > GarbageCollected(Finalized) doesn't have m_refCount.
> > 
> > CrossThreadPersistent<SQLTransactionBackend> in DatabaseTask.h represents
> > (Abstract)SQLTransactionBackend can be shared in multiple threads.
> 
> Yes, I know. However, you are using RefCountedWillBeGarbageCollectedFinalized
so
> this is RefCounted and not ThreadSafeRefCounted in the non-oilpan build. It
> should remain ThreadSafeRefCounted in the non-oilpan build because it is
> accessed from both context and database thread, right?

ah, got it.  This is my bad.  It should be
*ThreadSafe*RefCountedWillBeGarbageCollectedFinalized.

[haraken, 2014-03-19 06:39:31]

LGTM

On 2014/03/19 05:15:12, tkent wrote:
>
https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
> File Source/modules/webdatabase/SQLTransactionBackend.h (right):
> 
>
https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
> Source/modules/webdatabase/SQLTransactionBackend.h:112:
> RefPtrWillBeMember<AbstractSQLTransaction> m_frontend; // Has a reference
cycle,
> and will break in doCleanup().
> On 2014/03/19 04:59:20, haraken wrote:
> > 
> > Who guarantees that the thread that created the SQLTransaction is alive
while
> > the database thread is holding the SQLTransaction?
> > 
> > I think it's safe to store "an on-heap object created from one thread" into
"a
> > member of another thread" only if it's guaranteed that the creation thread
> > outlives the thread that holds the member. Is there any way to assert the
> > guarantee?
> 
> It's guaranteed.  A main/worker thread creates a dedicated database thread,
and
> it waits for  the database thread termination at [1].
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I understand that it's guaranteed in this particular case. My point is that it
would be nicer if we could have an ASSERT in oilpan's infrastructure to verify
that a member is not retaining a pointer to on-heap objects created from a
thread that is already destroyed. (I have no idea of how to implement such
ASSERT though.)

[Mads Ager (chromium), 2014-03-19 06:42:24]

On 2014/03/19 06:39:31, haraken wrote:
> LGTM
> 
> On 2014/03/19 05:15:12, tkent wrote:
> >
>
https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
> > File Source/modules/webdatabase/SQLTransactionBackend.h (right):
> > 
> >
>
https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
> > Source/modules/webdatabase/SQLTransactionBackend.h:112:
> > RefPtrWillBeMember<AbstractSQLTransaction> m_frontend; // Has a reference
> cycle,
> > and will break in doCleanup().
> > On 2014/03/19 04:59:20, haraken wrote:
> > > 
> > > Who guarantees that the thread that created the SQLTransaction is alive
> while
> > > the database thread is holding the SQLTransaction?
> > > 
> > > I think it's safe to store "an on-heap object created from one thread"
into
> "a
> > > member of another thread" only if it's guaranteed that the creation thread
> > > outlives the thread that holds the member. Is there any way to assert the
> > > guarantee?
> > 
> > It's guaranteed.  A main/worker thread creates a dedicated database thread,
> and
> > it waits for  the database thread termination at [1].
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I understand that it's guaranteed in this particular case. My point is that it
> would be nicer if we could have an ASSERT in oilpan's infrastructure to verify
> that a member is not retaining a pointer to on-heap objects created from a
> thread that is already destroyed. (I have no idea of how to implement such
> ASSERT though.)

If you have a member to an object on a dead worker thread you would hit an
ASSERT when that worker thread terminates. ThreadState::detach() has an assert
that after a number of garbage collections the worker threads heap is empty. If
another thread is holding a Member to an object in that worker thread heap, the
heap will not become empty because that object is alive.

[tkent, 2014-03-19 06:49:31]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 06:49:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/198213005/40001

[commit-bot: I haz the power, 2014-03-19 06:50:53]

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

[commit-bot: I haz the power, 2014-03-19 06:50:55]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[tkent, 2014-03-19 07:08:32]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 07:08:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/198213005/40001

[commit-bot: I haz the power, 2014-03-19 07:33:43]

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

[commit-bot: I haz the power, 2014-03-19 07:33:44]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[tkent, 2014-03-19 07:45:40]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 07:45:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/198213005/40001

[commit-bot: I haz the power, 2014-03-19 07:57:57]

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

[commit-bot: I haz the power, 2014-03-19 07:57:58]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-19 08:01:18]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/198213005/40001

[commit-bot: I haz the power, 2014-03-19 08:25:27]

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

[commit-bot: I haz the power, 2014-03-19 08:25:28]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[haraken, 2014-03-19 08:29:26]

On 2014/03/19 06:42:24, Mads Ager (chromium) wrote:
> On 2014/03/19 06:39:31, haraken wrote:
> > LGTM
> > 
> > On 2014/03/19 05:15:12, tkent wrote:
> > >
> >
>
https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
> > > File Source/modules/webdatabase/SQLTransactionBackend.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/198213005/diff/20001/Source/modules/webdataba...
> > > Source/modules/webdatabase/SQLTransactionBackend.h:112:
> > > RefPtrWillBeMember<AbstractSQLTransaction> m_frontend; // Has a reference
> > cycle,
> > > and will break in doCleanup().
> > > On 2014/03/19 04:59:20, haraken wrote:
> > > > 
> > > > Who guarantees that the thread that created the SQLTransaction is alive
> > while
> > > > the database thread is holding the SQLTransaction?
> > > > 
> > > > I think it's safe to store "an on-heap object created from one thread"
> into
> > "a
> > > > member of another thread" only if it's guaranteed that the creation
thread
> > > > outlives the thread that holds the member. Is there any way to assert
the
> > > > guarantee?
> > > 
> > > It's guaranteed.  A main/worker thread creates a dedicated database
thread,
> > and
> > > it waits for  the database thread termination at [1].
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > I understand that it's guaranteed in this particular case. My point is that
it
> > would be nicer if we could have an ASSERT in oilpan's infrastructure to
verify
> > that a member is not retaining a pointer to on-heap objects created from a
> > thread that is already destroyed. (I have no idea of how to implement such
> > ASSERT though.)
> 
> If you have a member to an object on a dead worker thread you would hit an
> ASSERT when that worker thread terminates. ThreadState::detach() has an assert
> that after a number of garbage collections the worker threads heap is empty.
If
> another thread is holding a Member to an object in that worker thread heap,
the
> heap will not become empty because that object is alive.

ah, this makes sense. Thanks for the clarification!

[tkent, 2014-03-19 08:34:24]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 08:34:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/198213005/40001

[commit-bot: I haz the power, 2014-03-19 09:11:06]

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

[commit-bot: I haz the power, 2014-03-19 09:11:08]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-19 09:33:22]

Committed patchset #3 manually as r169535 (presubmit successful).