Move ThreadableLoader to Oilpan heap (3/3)

Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663
Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Committed: https://crrev.com/3a06b506b18754545e5b50bf66d839c8b152f695
Cr-Original-Commit-Position: refs/heads/master@{#409961}
Cr-Commit-Position: refs/heads/master@{#411919}

[yhirano, 2016-07-29 09:35:54]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 09:36:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-29 10:08:49]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 10:08:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-29 10:09:00]

Description was changed from

==========
Move ThreadableLoader to Oilpan heap (2/2)

This CL is a refactoring and does not change the behavior.

BUG=587663
==========

to

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663
==========

[yhirano, 2016-07-29 10:10:32]

yhirano@chromium.org changed reviewers:
+ haraken@chromium.org

[yhirano, 2016-07-29 11:11:28]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 11:11:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 12:35:37]

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

[commit-bot: I haz the power, 2016-07-29 12:35:38]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-07-29 15:00:44]

LGTM

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (left):

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:57: class
CORE_EXPORT DocumentThreadableLoader final : public ThreadableLoader, private
RawResourceClient {

I'm just curious but is there any reason for the private inheritance?

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:60: //
ResourceLoader work only in the main thread, A WorkerThreadableLoader holds

A => a

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:76: //
+-----------------------+   CTWP  +----------------------+-+

I asked this question before but let me confirm again: What happens if we use
CTP for the WorkerThreadableLoader => MainThreadLoaderHolder? It will create a
cycle temporarily but the cycle is explicitly broken when the worker thread
dies. So it won't cause memory leaks, right?

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:81: //           
                     +------------------+

This model looks amazing! It looks much simpler than before.

[yhirano, 2016-08-01 05:52:54]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 05:53:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 07:37:48]

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

[commit-bot: I haz the power, 2016-08-01 07:37:49]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-01 08:36:31]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 08:36:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-01 08:52:38]

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (left):

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:57: class
CORE_EXPORT DocumentThreadableLoader final : public ThreadableLoader, private
RawResourceClient {
On 2016/07/29 15:00:44, haraken wrote:
> 
> I'm just curious but is there any reason for the private inheritance?

I don't know, and I don't like private inheritance, but I imagine that the
original author didn't want to allow an external user to handle
DocumentThreadableLoader as RawResourceClient / ResourceOwner.

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:60: //
ResourceLoader work only in the main thread, A WorkerThreadableLoader holds
On 2016/07/29 15:00:44, haraken wrote:
> 
> A => a

Done.

https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:76: //
+-----------------------+   CTWP  +----------------------+-+
On 2016/07/29 15:00:44, haraken wrote:
> 
> I asked this question before but let me confirm again: What happens if we use
> CTP for the WorkerThreadableLoader => MainThreadLoaderHolder? It will create a
> cycle temporarily but the cycle is explicitly broken when the worker thread
> dies. So it won't cause memory leaks, right?
> 
> 

It's a bit more complicated.

1. As WorkerThreadableLoader requires users to cancel it appropriately (see
ThreadableLoader's comments), WorkerThreadableLoader <=>
MainThreadableLoaderHelper persistent loop should be no harm (though I don't
feel good with the loop).

2. It is not planned now, but I think it's good to move ThreadableLoaderClient
to the heap. With on-heap ThreadableLoaderClient, WorkerThreadableLoader =>
ThreadableLoaderClient "raw ptr" will be replaced with a Member ptr. Also, it's
very common that a ThreadableLoaderClient has a  ThreadableLoader as Member.
That means there will be a persistent loop from ThreadableLoaderClient to
ThreadableLoaderClient itself. That's problematic. Assume that a
ThreadableLoader has a pre-finalizer that cancels its ThreadableLoader. That
pre-finalizer will not be called because there is a persistent loop.

3. As you say that loop will be broken when the thread is terminated but I think
that's not enough. We still see unbound memory consumption increase in a
long-running worker thread.

[haraken, 2016-08-01 09:07:24]

On 2016/08/01 08:52:38, yhirano wrote:
>
https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (left):
> 
>
https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:57: class
> CORE_EXPORT DocumentThreadableLoader final : public ThreadableLoader, private
> RawResourceClient {
> On 2016/07/29 15:00:44, haraken wrote:
> > 
> > I'm just curious but is there any reason for the private inheritance?
> 
> I don't know, and I don't like private inheritance, but I imagine that the
> original author didn't want to allow an external user to handle
> DocumentThreadableLoader as RawResourceClient / ResourceOwner.
> 
>
https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):
> 
>
https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:60: //
> ResourceLoader work only in the main thread, A WorkerThreadableLoader holds
> On 2016/07/29 15:00:44, haraken wrote:
> > 
> > A => a
> 
> Done.
> 
>
https://codereview.chromium.org/2193683004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:76: //
> +-----------------------+   CTWP  +----------------------+-+
> On 2016/07/29 15:00:44, haraken wrote:
> > 
> > I asked this question before but let me confirm again: What happens if we
use
> > CTP for the WorkerThreadableLoader => MainThreadLoaderHolder? It will create
a
> > cycle temporarily but the cycle is explicitly broken when the worker thread
> > dies. So it won't cause memory leaks, right?
> > 
> > 
> 
> It's a bit more complicated.
> 
> 1. As WorkerThreadableLoader requires users to cancel it appropriately (see
> ThreadableLoader's comments), WorkerThreadableLoader <=>
> MainThreadableLoaderHelper persistent loop should be no harm (though I don't
> feel good with the loop).
> 
> 2. It is not planned now, but I think it's good to move ThreadableLoaderClient
> to the heap. With on-heap ThreadableLoaderClient, WorkerThreadableLoader =>
> ThreadableLoaderClient "raw ptr" will be replaced with a Member ptr. Also,
it's
> very common that a ThreadableLoaderClient has a  ThreadableLoader as Member.
> That means there will be a persistent loop from ThreadableLoaderClient to
> ThreadableLoaderClient itself. That's problematic. Assume that a
> ThreadableLoader has a pre-finalizer that cancels its ThreadableLoader. That
> pre-finalizer will not be called because there is a persistent loop.
> 
> 3. As you say that loop will be broken when the thread is terminated but I
think
> that's not enough. We still see unbound memory consumption increase in a
> long-running worker thread.

Thanks, makes sense. LGTM.

[commit-bot: I haz the power, 2016-08-01 12:10:37]

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

[commit-bot: I haz the power, 2016-08-01 12:10:38]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-04 06:40:59]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 06:41:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 07:42:27]

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

[commit-bot: I haz the power, 2016-08-04 07:42:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-05 01:05:33]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-08-05 01:05:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-05 01:11:17]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2016-08-05 01:19:11]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-08-05 01:19:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 01:33:19]

Description was changed from

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663
==========

to

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663
==========

[commit-bot: I haz the power, 2016-08-05 01:33:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-05 01:34:33]

Description was changed from

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663
==========

to

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}
==========

[commit-bot: I haz the power, 2016-08-05 01:34:38]

Patchset 5 (id:??) landed as
https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}

[yhirano, 2016-08-08 04:59:28]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2225733002/ by yhirano@chromium.org.

The reason for reverting is: To investigate crashes reported at
https://crbug.com/634546..

[yhirano, 2016-08-09 06:55:20]

Description was changed from

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}
==========

to

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}
==========

[yhirano, 2016-08-09 06:55:32]

reopen

[yhirano, 2016-08-12 09:27:56]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 09:28:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-12 09:31:25]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 09:31:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-12 09:34:51]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 09:35:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-12 09:37:29]

cc: hiroshige@

I talked with Hiroshige and decided to revert DocumentThreadableLoader changes.

[commit-bot: I haz the power, 2016-08-12 10:33:54]

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

[commit-bot: I haz the power, 2016-08-12 10:33:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-08-14 12:09:14]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-08-14 12:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2193683004/#ps140001
(title: "revert DocumentThreadableLoader changes")

[commit-bot: I haz the power, 2016-08-14 12:09:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-14 12:09:29]

relanding...

[commit-bot: I haz the power, 2016-08-14 13:39:54]

Description was changed from

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}
==========

to

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}
==========

[commit-bot: I haz the power, 2016-08-14 13:39:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-14 13:41:40]

Description was changed from

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Cr-Commit-Position: refs/heads/master@{#409961}
==========

to

==========
Move ThreadableLoader to Oilpan heap (3/3)

This CL is a refactoring and does not change the behavior.

BUG=587663

Committed: https://crrev.com/2ec047e0061d6c07339e404f918f34459ab6a7b8
Committed: https://crrev.com/3a06b506b18754545e5b50bf66d839c8b152f695
Cr-Original-Commit-Position: refs/heads/master@{#409961}
Cr-Commit-Position: refs/heads/master@{#411919}
==========

[commit-bot: I haz the power, 2016-08-14 13:41:41]

Patchset 8 (id:??) landed as
https://crrev.com/3a06b506b18754545e5b50bf66d839c8b152f695
Cr-Commit-Position: refs/heads/master@{#411919}