Prepare DedicatedWorker for per thread heap

Prepare DedicatedWorker for per thread heap

Adds flag to turn on per thread heap on DedicatedWorkers. Add tests so both per thread haep enable and disabled are tested.

BUG=591606
Committed: https://crrev.com/5e3e05a33a5db4b1c7b836f172e0f9899c9965e3
Cr-Commit-Position: refs/heads/master@{#421118}

[keishi, 2016-09-14 07:26:51]

Description was changed from

==========
dw

BUG=
==========

to

==========
Adds flag to turn on per thread heap on DedicatedWorkers.
Also updates LocalFileSystem reference to use CrossThreadPersistent.

BUG=591606
==========

[keishi, 2016-09-14 07:32:18]

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

[commit-bot: I haz the power, 2016-09-14 07:32:29]

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

[keishi, 2016-09-14 07:32:39]

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

[keishi, 2016-09-14 07:32:39]

PTAL

[haraken, 2016-09-14 07:34:41]

Looks good.

But shall we enable the per-thread heap for CompositorWorker first?

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/filesystem/LocalFileSystem.cpp:90:
WTF::bind(&LocalFileSystem::fileSystemNotAllowedInternal,
wrapCrossThreadPersistent(this), wrapPersistent(context),
wrapPersistent(wrapper)));

Let's make this change in a separate CL.

[nhiroki, 2016-09-14 07:59:45]

nhiroki@chromium.org changed reviewers:
+ nhiroki@chromium.org

[nhiroki, 2016-09-14 07:59:46]

Drive-by comments

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp (right):

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp:49: ,
m_workerBackingThread(WorkerBackingThread::create("DedicatedWorker Thread",
true))
Can you add "/* enablePerThreadHeap */" or use enum instead?

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right):

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:30: static
std::unique_ptr<WorkerBackingThread> create(const char* name, bool
enablePerThreadHeap = false) { return wrapUnique(new WorkerBackingThread(name,
false, enablePerThreadHeap)); }
I'd prefer that each thread explicitly specifies whether to enable the
per-thread heap for readability. I guess there are only a few callsites of this
and it couldn't be so difficult to add them.

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:35: static
std::unique_ptr<WorkerBackingThread> createForTest(const char* name, bool
enablePerThreadHeap = false) { return wrapUnique(new WorkerBackingThread(name,
true, enablePerThreadHeap)); }
This is used by WorkerThreadTest that is a set of tests for common worker
infrastructure. We might want to modify the tests to run on both per-thread heap
and non-per-thread heap environments.

[commit-bot: I haz the power, 2016-09-14 11:11:06]

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

[commit-bot: I haz the power, 2016-09-14 11:11:07]

Dry run: This issue passed the CQ dry run.

[keishi, 2016-09-21 04:57:25]

PTAL

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp (right):

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp:49: ,
m_workerBackingThread(WorkerBackingThread::create("DedicatedWorker Thread",
true))
On 2016/09/14 07:59:46, nhiroki wrote:
> Can you add "/* enablePerThreadHeap */" or use enum instead?

Created CL to use enum.
https://codereview.chromium.org/2355193002/

New PS depends on this CL.

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right):

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:30: static
std::unique_ptr<WorkerBackingThread> create(const char* name, bool
enablePerThreadHeap = false) { return wrapUnique(new WorkerBackingThread(name,
false, enablePerThreadHeap)); }
On 2016/09/14 07:59:46, nhiroki wrote:
> I'd prefer that each thread explicitly specifies whether to enable the
> per-thread heap for readability. I guess there are only a few callsites of
this
> and it couldn't be so difficult to add them.

Done in https://codereview.chromium.org/2355193002/ .

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:35: static
std::unique_ptr<WorkerBackingThread> createForTest(const char* name, bool
enablePerThreadHeap = false) { return wrapUnique(new WorkerBackingThread(name,
true, enablePerThreadHeap)); }
On 2016/09/14 07:59:46, nhiroki wrote:
> This is used by WorkerThreadTest that is a set of tests for common worker
> infrastructure. We might want to modify the tests to run on both per-thread
heap
> and non-per-thread heap environments.

Done. Added INSTANTIATE_TEST_CASE_P for both cases.

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/2335063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/filesystem/LocalFileSystem.cpp:90:
WTF::bind(&LocalFileSystem::fileSystemNotAllowedInternal,
wrapCrossThreadPersistent(this), wrapPersistent(context),
wrapPersistent(wrapper)));
On 2016/09/14 07:34:41, haraken wrote:
> 
> Let's make this change in a separate CL.

Done.

[keishi, 2016-09-21 05:02:10]

Description was changed from

==========
Adds flag to turn on per thread heap on DedicatedWorkers.
Also updates LocalFileSystem reference to use CrossThreadPersistent.

BUG=591606
==========

to

==========
Prepare DedicatedWorker for per thread heap

Adds flag to turn on per thread heap on DedicatedWorkers. Add tests so both per
thread haep enable and disabled are tested.

BUG=591606
==========

[nhiroki, 2016-09-21 05:33:00]

Thank you for adding tests! Looks good overall. Added some comments.

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:345:
ThreadHeapMode m_threadHeapMode;
Can you add 'const' for clarifying that the heap mode is never changed during
operations.

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right):

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:31: static
std::unique_ptr<WorkerBackingThread> create(const char* name, ThreadHeapMode
threadHeapMode = MainThreadHeapMode) { return wrapUnique(new
WorkerBackingThread(name, false, threadHeapMode)); }
Can you specify MainThreadHeapMode in SharedWorkerThread.cpp and
ServiceWorkerThread.cpp, and then remove the default argument?

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:36: static
std::unique_ptr<WorkerBackingThread> createForTest(const char* name,
ThreadHeapMode threadHeapMode = MainThreadHeapMode) { return wrapUnique(new
WorkerBackingThread(name, true, threadHeapMode)); }
Apparently, all callers of this function explicitly specify |threadHeapMode|, so
this default argument wouldn't be necessary.

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:123: ThreadHeapMode
m_threadHeapMode;
const

[keishi, 2016-09-21 09:16:11]

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:345:
ThreadHeapMode m_threadHeapMode;
On 2016/09/21 05:32:59, nhiroki wrote:
> Can you add 'const' for clarifying that the heap mode is never changed during
> operations.

Done.

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right):

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:31: static
std::unique_ptr<WorkerBackingThread> create(const char* name, ThreadHeapMode
threadHeapMode = MainThreadHeapMode) { return wrapUnique(new
WorkerBackingThread(name, false, threadHeapMode)); }
On 2016/09/21 05:32:59, nhiroki wrote:
> Can you specify MainThreadHeapMode in SharedWorkerThread.cpp and
> ServiceWorkerThread.cpp, and then remove the default argument?

Done.

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:36: static
std::unique_ptr<WorkerBackingThread> createForTest(const char* name,
ThreadHeapMode threadHeapMode = MainThreadHeapMode) { return wrapUnique(new
WorkerBackingThread(name, true, threadHeapMode)); }
On 2016/09/21 05:32:59, nhiroki wrote:
> Apparently, all callers of this function explicitly specify |threadHeapMode|,
so
> this default argument wouldn't be necessary.

Done.

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/2335063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:123: ThreadHeapMode
m_threadHeapMode;
On 2016/09/21 05:32:59, nhiroki wrote:
> const

Done.

[nhiroki, 2016-09-21 09:36:01]

lgtm

https://codereview.chromium.org/2335063003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/SharedWorkerThread.cpp (right):

https://codereview.chromium.org/2335063003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/SharedWorkerThread.cpp:48: ,
m_workerBackingThread(WorkerBackingThread::create("SharedWorker Thread",
BlinkGC::PerThreadHeapMode))
MainThreadHeapMode?

[keishi, 2016-09-21 09:40:31]

https://codereview.chromium.org/2335063003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/SharedWorkerThread.cpp (right):

https://codereview.chromium.org/2335063003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/SharedWorkerThread.cpp:48: ,
m_workerBackingThread(WorkerBackingThread::create("SharedWorker Thread",
BlinkGC::PerThreadHeapMode))
On 2016/09/21 09:36:00, nhiroki wrote:
> MainThreadHeapMode?

Sorry. Done.

[haraken, 2016-09-21 12:31:23]

LGTM

[keishi, 2016-09-26 01:13:24]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-09-26 01:13:25]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2335063003/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2016-09-26 01:13:42]

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-09-26 03:49:13]

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

[commit-bot: I haz the power, 2016-09-26 03:49:14]

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...)

[keishi, 2016-09-26 03:58:34]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-26 03:58:48]

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-09-26 06:37:48]

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

[commit-bot: I haz the power, 2016-09-26 06:37:50]

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...)

[keishi, 2016-09-27 01:12:52]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-27 01:13:20]

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-09-27 02:47:42]

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

[commit-bot: I haz the power, 2016-09-27 02:47:44]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[keishi, 2016-09-27 03:00:12]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-27 03:00:48]

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-09-27 04:22:25]

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

[commit-bot: I haz the power, 2016-09-27 04:22:26]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[keishi, 2016-09-27 05:20:10]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-27 05:20:21]

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-09-27 05:50:01]

Description was changed from

==========
Prepare DedicatedWorker for per thread heap

Adds flag to turn on per thread heap on DedicatedWorkers. Add tests so both per
thread haep enable and disabled are tested.

BUG=591606
==========

to

==========
Prepare DedicatedWorker for per thread heap

Adds flag to turn on per thread heap on DedicatedWorkers. Add tests so both per
thread haep enable and disabled are tested.

BUG=591606
==========

[commit-bot: I haz the power, 2016-09-27 05:50:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-27 05:52:24]

Description was changed from

==========
Prepare DedicatedWorker for per thread heap

Adds flag to turn on per thread heap on DedicatedWorkers. Add tests so both per
thread haep enable and disabled are tested.

BUG=591606
==========

to

==========
Prepare DedicatedWorker for per thread heap

Adds flag to turn on per thread heap on DedicatedWorkers. Add tests so both per
thread haep enable and disabled are tested.

BUG=591606

Committed: https://crrev.com/5e3e05a33a5db4b1c7b836f172e0f9899c9965e3
Cr-Commit-Position: refs/heads/master@{#421118}
==========

[commit-bot: I haz the power, 2016-09-27 05:52:25]

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