Make workers inherit the large heap limit of the main isolate.

Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs with
an increased heap limit. Workers spawned by DevTools should also run
with increased heap limits, otherwise they will OOM while inspecting
large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
Review-Url: https://codereview.chromium.org/2627953005
Cr-Commit-Position: refs/heads/master@{#443619}
Committed: https://chromium.googlesource.com/chromium/src/+/843960f748b26fbd707e1bf6e3a3f51bc201db6a

[ulan, 2017-01-12 22:42:50]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

DevTools isolate runs with increased heap limit to avoid OOM while
inspecting large heaps. Workers spawned by DevTools should also
run with increased heap limits.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

The main isolate of DevTools runs with an increased heap limit.
Workers spawned by DevTools should also run with increased heap limits,
otherwise they will OOM while inspecting large heaps.

BUG=675911
==========

[ulan, 2017-01-12 22:44:12]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

The main isolate of DevTools runs with an increased heap limit.
Workers spawned by DevTools should also run with increased heap limits,
otherwise they will OOM while inspecting large heaps.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate
runs with an increased heap limit. Workers spawned by DevTools
should also run with increased heap limits, otherwise they will
OOM while inspecting large heaps.

BUG=675911
==========

[ulan, 2017-01-12 22:45:58]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate
runs with an increased heap limit. Workers spawned by DevTools
should also run with increased heap limits, otherwise they will
OOM while inspecting large heaps.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate
runs with an increased heap limit. Workers spawned by DevTools
should also run with increased heap limits, otherwise they will
OOM while inspecting large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

[ulan, 2017-01-12 22:46:17]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate
runs with an increased heap limit. Workers spawned by DevTools
should also run with increased heap limits, otherwise they will
OOM while inspecting large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs
with an increased heap limit. Workers spawned by DevTools should
also run with increased heap limits, otherwise they will OOM while inspecting
large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

[ulan, 2017-01-12 22:48:24]

ulan@chromium.org changed reviewers:
+ alph@chromium.org, jochen@chromium.org, kozyatinskiy@chromium.org

[ulan, 2017-01-12 22:48:25]

PTAL. This indirectly depends on https://codereview.chromium.org/2630453004/

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:54:
exceptionState.isolate()->IsHeapLimitIncreasedForDebugging();
I don't like getting isolate from the exceptionState, but don't see a better
way.

[ulan, 2017-01-12 22:48:56]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs
with an increased heap limit. Workers spawned by DevTools should
also run with increased heap limits, otherwise they will OOM while inspecting
large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs
with an increased heap limit. Workers spawned by DevTools should
also run with increased heap limits, otherwise they will OOM while
inspecting large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

[ulan, 2017-01-12 22:49:22]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs
with an increased heap limit. Workers spawned by DevTools should
also run with increased heap limits, otherwise they will OOM while
inspecting large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs with
an increased heap limit. Workers spawned by DevTools should also run
with increased heap limits, otherwise they will OOM while inspecting
large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

[haraken, 2017-01-13 03:54:06]

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

[haraken, 2017-01-13 03:54:07]

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:54:
exceptionState.isolate()->IsHeapLimitIncreasedForDebugging();
On 2017/01/12 22:48:25, ulan wrote:
> I don't like getting isolate from the exceptionState, but don't see a better
> way.

You can use toIsolate(executionContext). ExceptionState's isolate can be null.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:73:
WorkerV8Settings m_workerV8Settings;

I'm not sure if it's a good idea to make InProcessWorkerBase own
m_workerV8Settings. I'd prefer encapsulating such configuration in WorkerThread
and/or WorkerGlobalScope.

nhiroki@: Any thoughts?

[nhiroki, 2017-01-13 04:21:36]

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:73:
WorkerV8Settings m_workerV8Settings;
On 2017/01/13 03:54:06, haraken wrote:
> 
> I'm not sure if it's a good idea to make InProcessWorkerBase own
> m_workerV8Settings. I'd prefer encapsulating such configuration in
WorkerThread
> and/or WorkerGlobalScope.
> 
> nhiroki@: Any thoughts?

See my comment in InProcessWorkerMessagingProxy.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
(right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:115:
Document* document = toDocument(getExecutionContext());
Instead of passing the setting as an argument, you could get the flag here by
toIsolate(document)->IsHeapLimitIncreasedForDebugging().

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h
(right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:30:
#include "bindings/core/v8/WorkerV8Settings.h"
This CL doesn't include this header file.

[alph, 2017-01-13 04:42:26]

On 2017/01/13 04:21:36, nhiroki wrote:
>
https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/workers/InProcessWorkerBase.h (right):
> 
>
https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:73:
> WorkerV8Settings m_workerV8Settings;
> On 2017/01/13 03:54:06, haraken wrote:
> > 
> > I'm not sure if it's a good idea to make InProcessWorkerBase own
> > m_workerV8Settings. I'd prefer encapsulating such configuration in
> WorkerThread
> > and/or WorkerGlobalScope.
> > 
> > nhiroki@: Any thoughts?
> 
> See my comment in InProcessWorkerMessagingProxy.
> 
>
https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
> (right):
> 
>
https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:115:
> Document* document = toDocument(getExecutionContext());
> Instead of passing the setting as an argument, you could get the flag here by
> toIsolate(document)->IsHeapLimitIncreasedForDebugging().
> 
>
https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h
> (right):
> 
>
https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:30:
> #include "bindings/core/v8/WorkerV8Settings.h"
> This CL doesn't include this header file.

Hmm, this becomes more complicated for a workaround than I thought. Should we
just fix DevTools memory hog bug rather than raise the limit? Or is it needed
for something else as well?

[jochen (gone - plz use gerrit), 2017-01-13 08:45:11]

i agree that we should put the config on WorkerGlobalScope or WorkerThread

while devtools only uses in process workers, I think it would be nice if we had
to plumbing for any kind of worker

[ulan, 2017-01-13 13:51:20]

Thanks all for the comments!

I went with nhiroki's suggestion. Now WorkerV8Settings is initialized at the
same place as the WorkerSettings.

> Hmm, this becomes more complicated for a workaround than I thought. Should we
just fix DevTools memory hog bug rather than raise the limit? Or is it needed
for something else as well?
We can probably do both? Fix the memory hog and increase the limit.
There is no downside for increasing the limit of trusted V8 instances. Below the
limit GC will behave almost the same in both cases. Near the limit the
performance of the increased instance will be much better because there will be
no "last-resort" GC storm.

Besides, once kozyatinskiy@ lands his outstanding CL, inspected tab will run
with increased limit if it is nearing OOM with DevTools open. It would be good
to have DevTools limit not smaller than the inspected tab limit.

> i agree that we should put the config on WorkerGlobalScope or WorkerThread
> while devtools only uses in process workers, I think it would be nice if we
had
> to plumbing for any kind of worker
Acknowledged. I went with nhiroki's suggestion.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:54:
exceptionState.isolate()->IsHeapLimitIncreasedForDebugging();
On 2017/01/13 03:54:06, haraken wrote:
> On 2017/01/12 22:48:25, ulan wrote:
> > I don't like getting isolate from the exceptionState, but don't see a better
> > way.
> 
> You can use toIsolate(executionContext). ExceptionState's isolate can be null.
> 

Acknowledged. Did as nhiroki@ suggested.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:73:
WorkerV8Settings m_workerV8Settings;
On 2017/01/13 04:21:35, nhiroki (OOO until Jan 17) wrote:
> On 2017/01/13 03:54:06, haraken wrote:
> > 
> > I'm not sure if it's a good idea to make InProcessWorkerBase own
> > m_workerV8Settings. I'd prefer encapsulating such configuration in
> WorkerThread
> > and/or WorkerGlobalScope.
> > 
> > nhiroki@: Any thoughts?
> 
> See my comment in InProcessWorkerMessagingProxy.

Done.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:73:
WorkerV8Settings m_workerV8Settings;
On 2017/01/13 03:54:06, haraken wrote:
> 
> I'm not sure if it's a good idea to make InProcessWorkerBase own
> m_workerV8Settings. I'd prefer encapsulating such configuration in
WorkerThread
> and/or WorkerGlobalScope.
> 
> nhiroki@: Any thoughts?

Acknowledged.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
(right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:115:
Document* document = toDocument(getExecutionContext());
On 2017/01/13 04:21:35, nhiroki (OOO until Jan 17) wrote:
> Instead of passing the setting as an argument, you could get the flag here by
> toIsolate(document)->IsHeapLimitIncreasedForDebugging().

Done.

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h
(right):

https://codereview.chromium.org/2627953005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:30:
#include "bindings/core/v8/WorkerV8Settings.h"
On 2017/01/13 04:21:35, nhiroki (OOO until Jan 17) wrote:
> This CL doesn't include this header file.

Done.

[jochen (gone - plz use gerrit), 2017-01-13 14:11:44]

https://codereview.chromium.org/2627953005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h (right):

https://codereview.chromium.org/2627953005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:15: public:
WTF_MAKE_NONCOPIABLE()

https://codereview.chromium.org/2627953005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:17: bool
m_heapLimitIncreasedForDebugging;
can you move those to the private part, and add getters and a ctor to set them?

[ulan, 2017-01-13 14:33:59]

https://codereview.chromium.org/2627953005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h (right):

https://codereview.chromium.org/2627953005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:15: public:
On 2017/01/13 14:11:44, jochen wrote:
> WTF_MAKE_NONCOPIABLE()

Made it a struct as discussed offline.

https://codereview.chromium.org/2627953005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:17: bool
m_heapLimitIncreasedForDebugging;
On 2017/01/13 14:11:44, jochen wrote:
> can you move those to the private part, and add getters and a ctor to set
them?

Acknowledged.

[haraken, 2017-01-13 14:43:32]

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h (right):

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:14: struct
CORE_EXPORT WorkerV8Settings {

Maybe would it be cleaner to merge these settings into WorkerSettings?

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:15:
WorkerV8Settings(bool heapLimitIncreasedForDebugging,

We prefer an enum over a boolean.

[ulan, 2017-01-13 15:02:56]

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h (right):

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/01/13 14:43:32, haraken wrote:
> 
> 2017

good catch :)

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:14: struct
CORE_EXPORT WorkerV8Settings {
On 2017/01/13 14:43:32, haraken wrote:
> 
> Maybe would it be cleaner to merge these settings into WorkerSettings?

I don't have strong opinion on this. If you like, I can unify them in a separate
CL.

https://codereview.chromium.org/2627953005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h:15:
WorkerV8Settings(bool heapLimitIncreasedForDebugging,
On 2017/01/13 14:43:32, haraken wrote:
> 
> We prefer an enum over a boolean.

Done.

[haraken, 2017-01-13 15:40:11]

LGTM % merging WorkerSettings and WorkerV8Settings in a follow-up.

[ulan, 2017-01-13 15:43:33]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2017-01-13 15:44:00]

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, 2017-01-13 16:02:22]

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

[commit-bot: I haz the power, 2017-01-13 16:02:24]

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

[ulan, 2017-01-13 16:23:44]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-01-13 16:23:45]

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/2627953005/#ps120001
(title: "fix tests")

[commit-bot: I haz the power, 2017-01-13 16:24:29]

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, 2017-01-13 18:29:26]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484324624645080,
"parent_rev": "d814290cfc839f8e7d02b632e33026c8878a1b3d", "commit_rev":
"843960f748b26fbd707e1bf6e3a3f51bc201db6a"}

[commit-bot: I haz the power, 2017-01-13 18:29:58]

Description was changed from

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs with
an increased heap limit. Workers spawned by DevTools should also run
with increased heap limits, otherwise they will OOM while inspecting
large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911
==========

to

==========
Make workers inherit the large heap limit of the main isolate.

This is mainly needed for DevTools, where the main isolate runs with
an increased heap limit. Workers spawned by DevTools should also run
with increased heap limits, otherwise they will OOM while inspecting
large heaps.

This patch replaces the V8CacheOptions field of WorkerThreadStartupData
with more generic WorkerV8Settings and propagates the heap limit state
from the main isolate to worker isolates.

BUG=675911

Review-Url: https://codereview.chromium.org/2627953005
Cr-Commit-Position: refs/heads/master@{#443619}
Committed:
https://chromium.googlesource.com/chromium/src/+/843960f748b26fbd707e1bf6e3a3...
==========

[commit-bot: I haz the power, 2017-01-13 18:30:00]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/843960f748b26fbd707e1bf6e3a3...

[nhiroki, 2017-01-18 07:00:39]

lgtm