Issue 1158443008: compositor-worker: Share a thread and an isolate for compositor workers.

sadrul

sadrul@chromium.org changed reviewers: + kinuko@chromium.org

5 years, 7 months ago (2015-05-25 04:13:22 UTC) #1

sadrul

Hi! This is still a little WIP, in the sense that there is a TODO, ...

5 years, 7 months ago (2015-05-25 04:13:23 UTC) #2

kinuko

(Sorry for belated review, thanks for your patience) https://codereview.chromium.org/1158443008/diff/20001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1158443008/diff/20001/Source/core/workers/WorkerThread.cpp#newcode433 Source/core/workers/WorkerThread.cpp:433: m_isolate ...

5 years, 7 months ago (2015-05-26 14:41:25 UTC) #3

sadrul

Thanks for the feedback! I have responded to your comments below. I still haven't completed ...

5 years, 7 months ago (2015-05-27 01:28:01 UTC) #4

sadrul

Hi! I have added a few unit-tests to test the edge-cases I could think of. ...

5 years, 7 months ago (2015-05-28 00:32:19 UTC) #6

kinuko

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h#newcode77 Source/core/workers/WorkerThread.h:77: virtual void shutdownBackingThread(); Are they called outside WorkerThread or ...

5 years, 6 months ago (2015-05-28 16:21:59 UTC) #7

sadrul

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h#newcode77 Source/core/workers/WorkerThread.h:77: virtual void shutdownBackingThread(); On 2015/05/28 16:21:59, kinuko wrote: > ...

5 years, 6 months ago (2015-05-28 16:51:56 UTC) #8

sadrul

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h#newcode166 Source/core/workers/WorkerThread.h:166: OwnPtr<WorkerV8Isolate> m_isolate; On 2015/05/28 16:51:56, sadrul wrote: > On ...

5 years, 6 months ago (2015-05-28 18:00:47 UTC) #10

sadrul

To note: https://codereview.chromium.org/1134523002/ has landed. So the trybots should now turn green.

5 years, 6 months ago (2015-05-29 15:35:20 UTC) #11

kinuko

Sorry, let me take another look tomorrow. It's a bit puzzling me how they should ...

5 years, 6 months ago (2015-05-29 17:45:18 UTC) #12

Sorry, let me take another look tomorrow. It's a bit puzzling me how they should
be organized.  Sending incomplete comments for now, but not for asking immediate
action. Sorry if you're in a hurry.

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:166: OwnPtr<WorkerV8Isolate> m_isolate;
On 2015/05/28 18:00:47, sadrul wrote:
> On 2015/05/28 16:51:56, sadrul wrote:
> > On 2015/05/28 16:21:59, kinuko wrote:
> > > thread and isolate are used in a very similar way, but how they are tied
to
> > > WorkerThread are very different.  Why are they?
> > 
> > They don't actually have to be. It needs a little more code duplication for
> the
> > various subclasses, and I wanted to avoid that. But I can go back to having
> each
> > subclass be responsible for managing the isolate instead.
> 
> I have gone ahead and made this change. PTAL.

Well, was just asking because I vaguely remember you were planning to bundle
thread and isolate before (weren't you?).

I don't think each subclass should be necessarily responsible for managing it
either.

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:124: virtual OwnPtr<WorkerV8Isolate>&
workerIsolate() = 0;
Returning OwnPtr but not for the caller to take ownership looks a bit uncommon
to me... hmm.

I think the functionality wise this change (or previous change) looks good to
me, but in general I'm feeling that the WorkerThread's interface is getting less
and less organized.  I can't come up with a good plan for now though.

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:127: virtual void didStartRunLoop();
nit: I think this can be public as far as didStartRunLoop() is public, mainly
for symmetricity and readability.

sadrul

Thanks for the feedback! https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h#newcode166 Source/core/workers/WorkerThread.h:166: OwnPtr<WorkerV8Isolate> m_isolate; On 2015/05/29 17:45:18, ...

5 years, 6 months ago (2015-05-29 18:26:32 UTC) #13

Thanks for the feedback!

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:166: OwnPtr<WorkerV8Isolate> m_isolate;
On 2015/05/29 17:45:18, kinuko wrote:
> On 2015/05/28 18:00:47, sadrul wrote:
> > On 2015/05/28 16:51:56, sadrul wrote:
> > > On 2015/05/28 16:21:59, kinuko wrote:
> > > > thread and isolate are used in a very similar way, but how they are tied
> to
> > > > WorkerThread are very different.  Why are they?
> > > 
> > > They don't actually have to be. It needs a little more code duplication
for
> > the
> > > various subclasses, and I wanted to avoid that. But I can go back to
having
> > each
> > > subclass be responsible for managing the isolate instead.
> > 
> > I have gone ahead and made this change. PTAL.
> 
> Well, was just asking because I vaguely remember you were planning to bundle
> thread and isolate before (weren't you?).

I was, yes! (I was going to put it in WebThreadSupportingGC, which the consumer
would optionally initialize) But since there was discussion about allowing
multiple isolates in the same thread at some point, I didn't want to make a
change here that would make experimenting with that harder.

> I don't think each subclass should be necessarily responsible for managing it
> either.

I tend to agree. That was the approach I had in patchset6, which allows the
subclass to optionally override the behaviour, ridding all the other subclasses
to have to similar code. (although in terms of API, I think patchset7 is better,
and I can move the ownership of the WorkerV8Isolate to WorkerThread, have a
default implementation of workerIsolate(), and override it from
CompositorWorkerThread).

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:124: virtual OwnPtr<WorkerV8Isolate>&
workerIsolate() = 0;
On 2015/05/29 17:45:18, kinuko wrote:
> Returning OwnPtr but not for the caller to take ownership looks a bit uncommon
> to me... hmm.
> 
> I think the functionality wise this change (or previous change) looks good to
> me, but in general I'm feeling that the WorkerThread's interface is getting
less
> and less organized.  I can't come up with a good plan for now though.

With this change:

. For managing the WebThread: WorkerThread now has initializeBackingThread(),
shutdownBackingThread(), and backingThread(). (I am not counting
didStartRunLoop() and didStopRunLoop(), since these are more about setting
things up in chrome-land than managing the thread)

. For managing the v8::Isolate: WorkerThread now has workerIsolate(),
terminateV8Execution(), and isolate() (since this CL is also removing some
now-unnecessary isolate-managing functions).

Both of the sets are fairly small, and pretty self-explanatory. Is there
anything specific you think looks ugly/untidy that I could address?

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:127: virtual void didStartRunLoop();
On 2015/05/29 17:45:18, kinuko wrote:
> nit: I think this can be public as far as didStartRunLoop() is public, mainly
> for symmetricity and readability.

Acknowledged. Will make this change.

kinuko

Some more comments. https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/WorkerThread.h#newcode166 Source/core/workers/WorkerThread.h:166: OwnPtr<WorkerV8Isolate> m_isolate; On 2015/05/29 18:26:32, sadrul ...

5 years, 6 months ago (2015-05-31 15:18:24 UTC) #14

Some more comments.

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/120001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:166: OwnPtr<WorkerV8Isolate> m_isolate;
On 2015/05/29 18:26:32, sadrul wrote:
> On 2015/05/29 17:45:18, kinuko wrote:
> > On 2015/05/28 18:00:47, sadrul wrote:
> > > On 2015/05/28 16:51:56, sadrul wrote:
> > > > On 2015/05/28 16:21:59, kinuko wrote:
> > > > > thread and isolate are used in a very similar way, but how they are
tied
> > to
> > > > > WorkerThread are very different.  Why are they?
> > > > 
> > > > They don't actually have to be. It needs a little more code duplication
> for
> > > the
> > > > various subclasses, and I wanted to avoid that. But I can go back to
> having
> > > each
> > > > subclass be responsible for managing the isolate instead.
> > > 
> > > I have gone ahead and made this change. PTAL.
> > 
> > Well, was just asking because I vaguely remember you were planning to bundle
> > thread and isolate before (weren't you?).
> 
> I was, yes! (I was going to put it in WebThreadSupportingGC, which the
consumer
> would optionally initialize) But since there was discussion about allowing
> multiple isolates in the same thread at some point, I didn't want to make a
> change here that would make experimenting with that harder.

WebThreadSupportingGC is used not only for JS-controlled threads but also for
background threads that don't require v8 isolate, so bundling isolate in it
would have been probably a bit undesirable anyway.

> > I don't think each subclass should be necessarily responsible for managing
it
> > either.
> 
> I tend to agree. That was the approach I had in patchset6, which allows the
> subclass to optionally override the behaviour, ridding all the other
subclasses
> to have to similar code. (although in terms of API, I think patchset7 is
better,
> and I can move the ownership of the WorkerV8Isolate to WorkerThread, have a
> default implementation of workerIsolate(), and override it from
> CompositorWorkerThread).

Ok. Can we rename workerIsolate() to createWorkerIsolate() if that's the case?
'initialize' (as in PS6) doesn't really mean creation, and 'workerIsolate'
accessor (as in PS7) makes the ownership and creation semantics unclear.

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:320: ASSERT(isolate());
This part is a bit awkward, if we use the WorkerV8Isolate object to encapsulate
V8Isolate's lifetime I'd like to make it explicit.

  m_isolate = createWorkerIsolate();

The name 'm_isolate' or WorkerV8Isolate are not very helpful to know that they
may not be tied to one v8 isolate, I think probably it'd be better to rename
these, say... 'WorkerIsolateWrapper'?  (I'm not great at naming)

Then WorkerThread's interface would look like:
  v8::Isolate* isolate() const { m_isolateWrapper->isolate(); }
  PassOwnPtr<WorkerIsolateWrapper> createIsolateWrapper();
  OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerV8Isolate.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerV8Isolate.h:15: // A common interface for a worker to
manage the v8::Isolate used for the worker.
Please also note about its lifetime (when it's created and when it's destroyed)
and also that it doesn't necessarily tied to a single v8::Isolate.

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:43: static
CompositorWorkerManager* s_instance = nullptr;
The same question I asked before... this means that we'll be using the same
isolate and worker thread for multiple ExecutionContext, therefore things that
shouldn't be shared could be leaked to another context?  (Have you asked v8 and
security people about this, if not could we do so now?)

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.h:49: int m_isolateCount
= 0;
Using manual counters is not great especially if we have two of them... in the
current interface it's not straightforward but could we somehow bundle them at
least in this class? (We may need to break them later if we break the tie
between thread and isolate, but changing it wouldn't be that hard if it's only
for CWManager)

sadrul

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/compositorworker/CompositorWorkerManager.cpp File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): https://codereview.chromium.org/1158443008/diff/160001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode43 Source/modules/compositorworker/CompositorWorkerManager.cpp:43: static CompositorWorkerManager* s_instance = nullptr; On 2015/05/31 15:18:24, kinuko ...

5 years, 6 months ago (2015-05-31 16:23:43 UTC) #15

kinuko

On 2015/05/31 16:23:43, sadrul wrote: > https://codereview.chromium.org/1158443008/diff/160001/Source/modules/compositorworker/CompositorWorkerManager.cpp > File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): > > https://codereview.chromium.org/1158443008/diff/160001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode43 > ...

5 years, 6 months ago (2015-06-01 01:57:17 UTC) #16

sadrul

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/WorkerThread.cpp#newcode320 Source/core/workers/WorkerThread.cpp:320: ASSERT(isolate()); On 2015/05/31 15:18:24, kinuko wrote: > This part ...

5 years, 6 months ago (2015-06-01 05:24:11 UTC) #17

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:320: ASSERT(isolate());
On 2015/05/31 15:18:24, kinuko wrote:
> This part is a bit awkward, if we use the WorkerV8Isolate object to
encapsulate
> V8Isolate's lifetime I'd like to make it explicit.
> 
>   m_isolate = createWorkerIsolate();
> 
> The name 'm_isolate' or WorkerV8Isolate are not very helpful to know that they
> may not be tied to one v8 isolate, I think probably it'd be better to rename
> these, say... 'WorkerIsolateWrapper'?  (I'm not great at naming)
> 
> Then WorkerThread's interface would look like:
>   v8::Isolate* isolate() const { m_isolateWrapper->isolate(); }
>   PassOwnPtr<WorkerIsolateWrapper> createIsolateWrapper();
>   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;

WorkerIsolateWrapper sounds fairly clear. Done.

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.h:127: virtual void didStartRunLoop();
On 2015/05/29 18:26:32, sadrul wrote:
> On 2015/05/29 17:45:18, kinuko wrote:
> > nit: I think this can be public as far as didStartRunLoop() is public,
mainly
> > for symmetricity and readability.
> 
> Acknowledged. Will make this change.

Done.

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
File Source/core/workers/WorkerV8Isolate.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/core/workers/Wo...
Source/core/workers/WorkerV8Isolate.h:15: // A common interface for a worker to
manage the v8::Isolate used for the worker.
On 2015/05/31 15:18:24, kinuko wrote:
> Please also note about its lifetime (when it's created and when it's
destroyed)
> and also that it doesn't necessarily tied to a single v8::Isolate.

Done.

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.h (right):

https://codereview.chromium.org/1158443008/diff/160001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.h:49: int m_isolateCount
= 0;
On 2015/05/31 15:18:24, kinuko wrote:
> Using manual counters is not great especially if we have two of them... in the
> current interface it's not straightforward but could we somehow bundle them at
> least in this class? (We may need to break them later if we break the tie
> between thread and isolate, but changing it wouldn't be that hard if it's only
> for CWManager)

Done.

kinuko

Haven't looked into tests yet, but sending some more comments now. https://codereview.chromium.org/1158443008/diff/180001/Source/core/workers/DedicatedWorkerThread.cpp File Source/core/workers/DedicatedWorkerThread.cpp (right): ...

5 years, 6 months ago (2015-06-01 13:15:42 UTC) #18

sadrul

https://codereview.chromium.org/1158443008/diff/180001/Source/core/workers/DedicatedWorkerThread.cpp File Source/core/workers/DedicatedWorkerThread.cpp (right): https://codereview.chromium.org/1158443008/diff/180001/Source/core/workers/DedicatedWorkerThread.cpp#newcode71 Source/core/workers/DedicatedWorkerThread.cpp:71: return WorkerIsolateWrapper::createDefault(); On 2015/06/01 13:15:41, kinuko wrote: > I'm ...

5 years, 6 months ago (2015-06-01 14:21:16 UTC) #20

kinuko

ok, lgtm mod nits https://codereview.chromium.org/1158443008/diff/220001/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp File Source/modules/compositorworker/CompositorWorkerManagerTest.cpp (right): https://codereview.chromium.org/1158443008/diff/220001/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp#newcode34 Source/modules/compositorworker/CompositorWorkerManagerTest.cpp:34: void executeCallbackAfterV8Termination(PassOwnPtr<Function<void()>> callback) The verb ...

5 years, 6 months ago (2015-06-01 16:08:00 UTC) #21

kinuko

By the way once you feel comfortable you should probably make modules/compositorworker/OWNERS and put your ...

5 years, 6 months ago (2015-06-01 16:09:34 UTC) #22

sadrul

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

5 years, 6 months ago (2015-06-01 17:12:38 UTC) #23

sadrul

+haraken@ for Source/modules/ owner. On 2015/06/01 16:09:34, kinuko wrote: > By the way once you ...

5 years, 6 months ago (2015-06-01 17:12:39 UTC) #24

haraken

not LGTM, unfortunately. This CL looks more complicated than necessary. Also can we split this ...

5 years, 6 months ago (2015-06-02 02:14:49 UTC) #25

sadrul

Hi! Thanks for the feedback! I replied to your question about SharedV8Isolate below. I will ...

5 years, 6 months ago (2015-06-02 02:37:54 UTC) #26

Hi! Thanks for the feedback! I replied to your question about SharedV8Isolate
below. I will address the rest of the comments in the follow-up CL.

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
SharedV8Isolate final : public WorkerIsolateWrapper {
On 2015/06/02 02:14:49, haraken wrote:
> 
> I think this is mis-designed and complicated more than needed. You now have
> WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
> CompositorWorkerManager (which is doing a reference counting for worker
> threads).
> 
> In particular, I don't quite understand why you need SharedV8Isolate. Why
can't
> we simply create an backing Isolate when CompositorWorkerManager creates the
> first worker and destroy the Isolate when CompositorWorkerManager destroys the
> last worker? CompositorWorkerManager should know the timing when the backing
> Isolate should be created or destroyed without getting any help of
> SharedV8Isolate etc.
> 
> I think WorkerIsolateWrapper is a good abstraction, but I'm not sure if we
need
> WorkerIsolateWrapperDefault and SharedV8Isolate.

We need SharedV8Isolate because each WorkerThread owns its own
WorkerIsolateWrapper instance. For compositor workers, we cannot pass ownership
of the isolate to the first worker, since we want the manager to retain
ownership. So we need a version of WorkerIsolateWrapper that doesn't take
ownership of the isolate. SharedV8Isolate+CompositorWorkerManager essentially
does as you suggest (by creating an isolate for the first worker, and destroying
when the last worker is terminated), but I suppose through some indirection.
Some amount of code-rearrangement can make this better by moving the ownership
out of WorkerThread (into the subclass implementations, for example), and have
WorkerThread work with a raw-pointer that it receives from the subclass instead.
This way, we won't need SharedV8Isolate. Would that me more palatable?

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:51: static void
destroyThread(WebThreadSupportingGC* thread)
On 2015/06/02 02:14:49, haraken wrote:
> 
> Where is this called?

In a posted task (below in line 116).

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:53: delete thread;
On 2015/06/02 02:14:49, haraken wrote:
> 
> We should avoid calling manual delete...

I tried using something like DeleteSoon() in chrome, but couldn't find something
similar for blink. Is there anything that I missed?

haraken

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode17 Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class SharedV8Isolate final : public WorkerIsolateWrapper { On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 02:42:23 UTC) #27

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
SharedV8Isolate final : public WorkerIsolateWrapper {
On 2015/06/02 02:37:53, sadrul wrote:
> On 2015/06/02 02:14:49, haraken wrote:
> > 
> > I think this is mis-designed and complicated more than needed. You now have
> > WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
> > CompositorWorkerManager (which is doing a reference counting for worker
> > threads).
> > 
> > In particular, I don't quite understand why you need SharedV8Isolate. Why
> can't
> > we simply create an backing Isolate when CompositorWorkerManager creates the
> > first worker and destroy the Isolate when CompositorWorkerManager destroys
the
> > last worker? CompositorWorkerManager should know the timing when the backing
> > Isolate should be created or destroyed without getting any help of
> > SharedV8Isolate etc.
> > 
> > I think WorkerIsolateWrapper is a good abstraction, but I'm not sure if we
> need
> > WorkerIsolateWrapperDefault and SharedV8Isolate.
> 
> We need SharedV8Isolate because each WorkerThread owns its own
> WorkerIsolateWrapper instance. For compositor workers, we cannot pass
ownership
> of the isolate to the first worker, since we want the manager to retain
> ownership. So we need a version of WorkerIsolateWrapper that doesn't take
> ownership of the isolate. SharedV8Isolate+CompositorWorkerManager essentially
> does as you suggest (by creating an isolate for the first worker, and
destroying
> when the last worker is terminated), but I suppose through some indirection.
> Some amount of code-rearrangement can make this better by moving the ownership
> out of WorkerThread (into the subclass implementations, for example), and have
> WorkerThread work with a raw-pointer that it receives from the subclass
instead.
> This way, we won't need SharedV8Isolate. Would that me more palatable?

Just to confirm:

Compositor workers : CompositorWorkerManager : WorkerThread : Isolate = N : 1 :
1 : 1

Maybe am I misunderstanding?

sadrul

[ some comments still left to address about renaming and moving WorkerIsolateWrapper. But responded to ...

5 years, 6 months ago (2015-06-02 02:48:28 UTC) #28

[ some comments still left to address about renaming and moving
WorkerIsolateWrapper. But responded to the clarification question. ]

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
File Source/core/workers/WorkerIsolateWrapper.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
Source/core/workers/WorkerIsolateWrapper.cpp:15: namespace {
On 2015/06/02 02:14:49, haraken wrote:
> 
> Nit: I'd drop this namespace.

Done.

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
File Source/core/workers/WorkerIsolateWrapper.h (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
Source/core/workers/WorkerIsolateWrapper.h:27: virtual void willDestroy() = 0;
On 2015/06/02 02:14:49, haraken wrote:
> 
> Nit: willBeDestroyed()?

Done.

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:460: return m_isolateWrapper ?
m_isolateWrapper->isolate() : nullptr;
On 2015/06/02 02:14:49, haraken wrote:
> 
> v8::Isolate* must not be null. Can we change this to ASSERT(m_isolateWrapper)?

Done.

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:464: {
On 2015/06/02 02:14:49, haraken wrote:
> 
> Add ASSERT(!isMainThread()).

Done. (added ASSERT(isCurrentThread()); instead)

https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:470: backingThread().shutdown();
On 2015/06/02 02:14:49, haraken wrote:
> 
> Add ASSERT(!isMainThread()).

Ditto.

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
SharedV8Isolate final : public WorkerIsolateWrapper {
On 2015/06/02 02:42:23, haraken wrote:
> On 2015/06/02 02:37:53, sadrul wrote:
> > On 2015/06/02 02:14:49, haraken wrote:
> > > 
> > > I think this is mis-designed and complicated more than needed. You now
have
> > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
> > > CompositorWorkerManager (which is doing a reference counting for worker
> > > threads).
> > > 
> > > In particular, I don't quite understand why you need SharedV8Isolate. Why
> > can't
> > > we simply create an backing Isolate when CompositorWorkerManager creates
the
> > > first worker and destroy the Isolate when CompositorWorkerManager destroys
> the
> > > last worker? CompositorWorkerManager should know the timing when the
backing
> > > Isolate should be created or destroyed without getting any help of
> > > SharedV8Isolate etc.
> > > 
> > > I think WorkerIsolateWrapper is a good abstraction, but I'm not sure if we
> > need
> > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > 
> > We need SharedV8Isolate because each WorkerThread owns its own
> > WorkerIsolateWrapper instance. For compositor workers, we cannot pass
> ownership
> > of the isolate to the first worker, since we want the manager to retain
> > ownership. So we need a version of WorkerIsolateWrapper that doesn't take
> > ownership of the isolate. SharedV8Isolate+CompositorWorkerManager
essentially
> > does as you suggest (by creating an isolate for the first worker, and
> destroying
> > when the last worker is terminated), but I suppose through some indirection.
> > Some amount of code-rearrangement can make this better by moving the
ownership
> > out of WorkerThread (into the subclass implementations, for example), and
have
> > WorkerThread work with a raw-pointer that it receives from the subclass
> instead.
> > This way, we won't need SharedV8Isolate. Would that me more palatable?
> 
> Just to confirm:
> 
> Compositor workers : CompositorWorkerManager : WorkerThread : Isolate = N : 1
:
> 1 : 1
> 
> Maybe am I misunderstanding?


There can be multiple WorkerThread instances. So:

CompositorWorkerManager : Isolate : Compositor workers : WorkerThread = 1 : 1 :
N : N

To clarify a bit more:

CompositorWorkerManager : Isolate = 1 : 1
CompositorWorkerManager : Compositor workers = 1 : N
Compositor worker : WorkerThread = 1 : 1

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.h (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.h:35:
WebThreadSupportingGC* m_thread = nullptr;
On 2015/06/02 02:14:49, haraken wrote:
> 
> It looks nasty to keep a raw pointer to the WebThreadSupportingGC. Can we
remove
> this? (i.e., we can always use
> CompositorWorkerManager::instance()->compositorWorkerThread() to get the Web
> thread.)

Done.

haraken

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode17 Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class SharedV8Isolate final : public WorkerIsolateWrapper { On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 03:34:26 UTC) #29

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
SharedV8Isolate final : public WorkerIsolateWrapper {
On 2015/06/02 02:48:28, sadrul wrote:
> On 2015/06/02 02:42:23, haraken wrote:
> > On 2015/06/02 02:37:53, sadrul wrote:
> > > On 2015/06/02 02:14:49, haraken wrote:
> > > > 
> > > > I think this is mis-designed and complicated more than needed. You now
> have
> > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
> > > > CompositorWorkerManager (which is doing a reference counting for worker
> > > > threads).
> > > > 
> > > > In particular, I don't quite understand why you need SharedV8Isolate.
Why
> > > can't
> > > > we simply create an backing Isolate when CompositorWorkerManager creates
> the
> > > > first worker and destroy the Isolate when CompositorWorkerManager
destroys
> > the
> > > > last worker? CompositorWorkerManager should know the timing when the
> backing
> > > > Isolate should be created or destroyed without getting any help of
> > > > SharedV8Isolate etc.
> > > > 
> > > > I think WorkerIsolateWrapper is a good abstraction, but I'm not sure if
we
> > > need
> > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > 
> > > We need SharedV8Isolate because each WorkerThread owns its own
> > > WorkerIsolateWrapper instance. For compositor workers, we cannot pass
> > ownership
> > > of the isolate to the first worker, since we want the manager to retain
> > > ownership. So we need a version of WorkerIsolateWrapper that doesn't take
> > > ownership of the isolate. SharedV8Isolate+CompositorWorkerManager
> essentially
> > > does as you suggest (by creating an isolate for the first worker, and
> > destroying
> > > when the last worker is terminated), but I suppose through some
indirection.
> > > Some amount of code-rearrangement can make this better by moving the
> ownership
> > > out of WorkerThread (into the subclass implementations, for example), and
> have
> > > WorkerThread work with a raw-pointer that it receives from the subclass
> > instead.
> > > This way, we won't need SharedV8Isolate. Would that me more palatable?
> > 
> > Just to confirm:
> > 
> > Compositor workers : CompositorWorkerManager : WorkerThread : Isolate = N :
1
> :
> > 1 : 1
> > 
> > Maybe am I misunderstanding?
> 
> 
> There can be multiple WorkerThread instances. So:
> 
> CompositorWorkerManager : Isolate : Compositor workers : WorkerThread = 1 : 1
:
> N : N
> 
> To clarify a bit more:
> 
> CompositorWorkerManager : Isolate = 1 : 1
> CompositorWorkerManager : Compositor workers = 1 : N
> Compositor worker : WorkerThread = 1 : 1

Thanks for the clarification!

It seems that the current class design doesn't reflect the concept. For example,
in this CL WorkerThread owns an Isolate, but conceptually there is no ownership
relationship from WorkerThread to Isolate. Would it be possible to reconsider
the class design that reflects the concept more straightforwardly?

sadrul

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode17 Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class SharedV8Isolate final : public WorkerIsolateWrapper { On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 04:23:56 UTC) #30

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
SharedV8Isolate final : public WorkerIsolateWrapper {
On 2015/06/02 03:34:26, haraken wrote:
> On 2015/06/02 02:48:28, sadrul wrote:
> > On 2015/06/02 02:42:23, haraken wrote:
> > > On 2015/06/02 02:37:53, sadrul wrote:
> > > > On 2015/06/02 02:14:49, haraken wrote:
> > > > > 
> > > > > I think this is mis-designed and complicated more than needed. You now
> > have
> > > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
> > > > > CompositorWorkerManager (which is doing a reference counting for
worker
> > > > > threads).
> > > > > 
> > > > > In particular, I don't quite understand why you need SharedV8Isolate.
> Why
> > > > can't
> > > > > we simply create an backing Isolate when CompositorWorkerManager
creates
> > the
> > > > > first worker and destroy the Isolate when CompositorWorkerManager
> destroys
> > > the
> > > > > last worker? CompositorWorkerManager should know the timing when the
> > backing
> > > > > Isolate should be created or destroyed without getting any help of
> > > > > SharedV8Isolate etc.
> > > > > 
> > > > > I think WorkerIsolateWrapper is a good abstraction, but I'm not sure
if
> we
> > > > need
> > > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > > 
> > > > We need SharedV8Isolate because each WorkerThread owns its own
> > > > WorkerIsolateWrapper instance. For compositor workers, we cannot pass
> > > ownership
> > > > of the isolate to the first worker, since we want the manager to retain
> > > > ownership. So we need a version of WorkerIsolateWrapper that doesn't
take
> > > > ownership of the isolate. SharedV8Isolate+CompositorWorkerManager
> > essentially
> > > > does as you suggest (by creating an isolate for the first worker, and
> > > destroying
> > > > when the last worker is terminated), but I suppose through some
> indirection.
> > > > Some amount of code-rearrangement can make this better by moving the
> > ownership
> > > > out of WorkerThread (into the subclass implementations, for example),
and
> > have
> > > > WorkerThread work with a raw-pointer that it receives from the subclass
> > > instead.
> > > > This way, we won't need SharedV8Isolate. Would that me more palatable?
> > > 
> > > Just to confirm:
> > > 
> > > Compositor workers : CompositorWorkerManager : WorkerThread : Isolate = N
:
> 1
> > :
> > > 1 : 1
> > > 
> > > Maybe am I misunderstanding?
> > 
> > 
> > There can be multiple WorkerThread instances. So:
> > 
> > CompositorWorkerManager : Isolate : Compositor workers : WorkerThread = 1 :
1
> :
> > N : N
> > 
> > To clarify a bit more:
> > 
> > CompositorWorkerManager : Isolate = 1 : 1
> > CompositorWorkerManager : Compositor workers = 1 : N
> > Compositor worker : WorkerThread = 1 : 1
> 
> Thanks for the clarification!
> 
> It seems that the current class design doesn't reflect the concept. For
example,
> in this CL WorkerThread owns an Isolate, but conceptually there is no
ownership
> relationship from WorkerThread to Isolate.

The WorkerThread doesn't necessarily own the Isolate. That is why we have this
IsolateWrapper, which may or may not own the isolate. The IsolateWrapper for the
CompositorWorkerThread does not own the isolate, since the isolate is shared
with other CompositorWorkerThread instances. For other workers (e.g.
DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
IsolateWrapper used (the default one) does own the Isolate. So essentially, each
WorkerThread owns an IsolateWrapper to communicate with the isolate, but the
ownership of the v8::Isolate is left up to the implementation (i.e. the
subclass) to decide. (I have attempted to clarify this for WorkerIsolateWrapper
doc here:
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)

To briefly explain: each worker is represented by a WorkerThread (which is a bit
of a misnomer; we still intend to rename it to WorkerScript:
https://codereview.chromium.org/1115923002/), which has a WorkerGlobalScope,
which owns a WorkerScriptController. We do need a WorkerThread for each
compositor worker (because we want each worker to have its own global-scope),
and because all of them live in the same thread, it is also necessary for these
workers to share the v8::Isolate. So the ownership of the v8::Isolate does have
to be different for compositor-workers from the other workers, and this seems
fairly unavoidable. Does this make a reasonable amount of sense to explain the
isolate ownership?

(Note: we do have plans on experimenting with multiple threads/isolates, and
this design does allow for that to happen fairly easily, but we want to start
with the simpler approach)

> Would it be possible to reconsider
> the class design that reflects the concept more straightforwardly?

haraken

On 2015/06/02 04:23:56, sadrul wrote: > https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp > File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): > > https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode17 > ...

5 years, 6 months ago (2015-06-02 04:36:14 UTC) #31

On 2015/06/02 04:23:56, sadrul wrote:
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):
> 
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
> SharedV8Isolate final : public WorkerIsolateWrapper {
> On 2015/06/02 03:34:26, haraken wrote:
> > On 2015/06/02 02:48:28, sadrul wrote:
> > > On 2015/06/02 02:42:23, haraken wrote:
> > > > On 2015/06/02 02:37:53, sadrul wrote:
> > > > > On 2015/06/02 02:14:49, haraken wrote:
> > > > > > 
> > > > > > I think this is mis-designed and complicated more than needed. You
now
> > > have
> > > > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
> > > > > > CompositorWorkerManager (which is doing a reference counting for
> worker
> > > > > > threads).
> > > > > > 
> > > > > > In particular, I don't quite understand why you need
SharedV8Isolate.
> > Why
> > > > > can't
> > > > > > we simply create an backing Isolate when CompositorWorkerManager
> creates
> > > the
> > > > > > first worker and destroy the Isolate when CompositorWorkerManager
> > destroys
> > > > the
> > > > > > last worker? CompositorWorkerManager should know the timing when the
> > > backing
> > > > > > Isolate should be created or destroyed without getting any help of
> > > > > > SharedV8Isolate etc.
> > > > > > 
> > > > > > I think WorkerIsolateWrapper is a good abstraction, but I'm not sure
> if
> > we
> > > > > need
> > > > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > > > 
> > > > > We need SharedV8Isolate because each WorkerThread owns its own
> > > > > WorkerIsolateWrapper instance. For compositor workers, we cannot pass
> > > > ownership
> > > > > of the isolate to the first worker, since we want the manager to
retain
> > > > > ownership. So we need a version of WorkerIsolateWrapper that doesn't
> take
> > > > > ownership of the isolate. SharedV8Isolate+CompositorWorkerManager
> > > essentially
> > > > > does as you suggest (by creating an isolate for the first worker, and
> > > > destroying
> > > > > when the last worker is terminated), but I suppose through some
> > indirection.
> > > > > Some amount of code-rearrangement can make this better by moving the
> > > ownership
> > > > > out of WorkerThread (into the subclass implementations, for example),
> and
> > > have
> > > > > WorkerThread work with a raw-pointer that it receives from the
subclass
> > > > instead.
> > > > > This way, we won't need SharedV8Isolate. Would that me more palatable?
> > > > 
> > > > Just to confirm:
> > > > 
> > > > Compositor workers : CompositorWorkerManager : WorkerThread : Isolate =
N
> :
> > 1
> > > :
> > > > 1 : 1
> > > > 
> > > > Maybe am I misunderstanding?
> > > 
> > > 
> > > There can be multiple WorkerThread instances. So:
> > > 
> > > CompositorWorkerManager : Isolate : Compositor workers : WorkerThread = 1
:
> 1
> > :
> > > N : N
> > > 
> > > To clarify a bit more:
> > > 
> > > CompositorWorkerManager : Isolate = 1 : 1
> > > CompositorWorkerManager : Compositor workers = 1 : N
> > > Compositor worker : WorkerThread = 1 : 1
> > 
> > Thanks for the clarification!
> > 
> > It seems that the current class design doesn't reflect the concept. For
> example,
> > in this CL WorkerThread owns an Isolate, but conceptually there is no
> ownership
> > relationship from WorkerThread to Isolate.
> 
> The WorkerThread doesn't necessarily own the Isolate. That is why we have this
> IsolateWrapper, which may or may not own the isolate. The IsolateWrapper for
the
> CompositorWorkerThread does not own the isolate, since the isolate is shared
> with other CompositorWorkerThread instances. For other workers (e.g.
> DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
> IsolateWrapper used (the default one) does own the Isolate. So essentially,
each
> WorkerThread owns an IsolateWrapper to communicate with the isolate, but the
> ownership of the v8::Isolate is left up to the implementation (i.e. the
> subclass) to decide. (I have attempted to clarify this for
WorkerIsolateWrapper
> doc here:
>
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)
> 
> To briefly explain: each worker is represented by a WorkerThread (which is a
bit
> of a misnomer; we still intend to rename it to WorkerScript:
> https://codereview.chromium.org/1115923002/), which has a WorkerGlobalScope,
> which owns a WorkerScriptController. We do need a WorkerThread for each
> compositor worker (because we want each worker to have its own global-scope),
> and because all of them live in the same thread, it is also necessary for
these
> workers to share the v8::Isolate. So the ownership of the v8::Isolate does
have
> to be different for compositor-workers from the other workers, and this seems
> fairly unavoidable. Does this make a reasonable amount of sense to explain the
> isolate ownership?
> 
> (Note: we do have plans on experimenting with multiple threads/isolates, and
> this design does allow for that to happen fairly easily, but we want to start
> with the simpler approach)

Thanks for the details! Then would it make more sense to restructure the classes
like this?

class CompositorWorkerManager {
  v8::Isolate* m_isolate; // Owns.
  WorkerThread* m_workerThreads; // Owns.
};

class WorkerManager {  // For normal workers
  v8::Isolate* m_isolate; // Owns.
  WorkerThread* m_workerThread; // Owns.
};

class WorkerThread { };

sadrul

On 2015/06/02 04:36:14, haraken wrote: > On 2015/06/02 04:23:56, sadrul wrote: > > > https://codereview.chromium.org/1158443008/diff/240001/Source/modules/compositorworker/CompositorWorkerManager.cpp ...

5 years, 6 months ago (2015-06-02 04:45:41 UTC) #32

On 2015/06/02 04:36:14, haraken wrote:
> On 2015/06/02 04:23:56, sadrul wrote:
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
> > SharedV8Isolate final : public WorkerIsolateWrapper {
> > On 2015/06/02 03:34:26, haraken wrote:
> > > On 2015/06/02 02:48:28, sadrul wrote:
> > > > On 2015/06/02 02:42:23, haraken wrote:
> > > > > On 2015/06/02 02:37:53, sadrul wrote:
> > > > > > On 2015/06/02 02:14:49, haraken wrote:
> > > > > > > 
> > > > > > > I think this is mis-designed and complicated more than needed. You
> now
> > > > have
> > > > > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault,
SharedV8Isolate,
> > > > > > > CompositorWorkerManager (which is doing a reference counting for
> > worker
> > > > > > > threads).
> > > > > > > 
> > > > > > > In particular, I don't quite understand why you need
> SharedV8Isolate.
> > > Why
> > > > > > can't
> > > > > > > we simply create an backing Isolate when CompositorWorkerManager
> > creates
> > > > the
> > > > > > > first worker and destroy the Isolate when CompositorWorkerManager
> > > destroys
> > > > > the
> > > > > > > last worker? CompositorWorkerManager should know the timing when
the
> > > > backing
> > > > > > > Isolate should be created or destroyed without getting any help of
> > > > > > > SharedV8Isolate etc.
> > > > > > > 
> > > > > > > I think WorkerIsolateWrapper is a good abstraction, but I'm not
sure
> > if
> > > we
> > > > > > need
> > > > > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > > > > 
> > > > > > We need SharedV8Isolate because each WorkerThread owns its own
> > > > > > WorkerIsolateWrapper instance. For compositor workers, we cannot
pass
> > > > > ownership
> > > > > > of the isolate to the first worker, since we want the manager to
> retain
> > > > > > ownership. So we need a version of WorkerIsolateWrapper that doesn't
> > take
> > > > > > ownership of the isolate. SharedV8Isolate+CompositorWorkerManager
> > > > essentially
> > > > > > does as you suggest (by creating an isolate for the first worker,
and
> > > > > destroying
> > > > > > when the last worker is terminated), but I suppose through some
> > > indirection.
> > > > > > Some amount of code-rearrangement can make this better by moving the
> > > > ownership
> > > > > > out of WorkerThread (into the subclass implementations, for
example),
> > and
> > > > have
> > > > > > WorkerThread work with a raw-pointer that it receives from the
> subclass
> > > > > instead.
> > > > > > This way, we won't need SharedV8Isolate. Would that me more
palatable?
> > > > > 
> > > > > Just to confirm:
> > > > > 
> > > > > Compositor workers : CompositorWorkerManager : WorkerThread : Isolate
=
> N
> > :
> > > 1
> > > > :
> > > > > 1 : 1
> > > > > 
> > > > > Maybe am I misunderstanding?
> > > > 
> > > > 
> > > > There can be multiple WorkerThread instances. So:
> > > > 
> > > > CompositorWorkerManager : Isolate : Compositor workers : WorkerThread =
1
> :
> > 1
> > > :
> > > > N : N
> > > > 
> > > > To clarify a bit more:
> > > > 
> > > > CompositorWorkerManager : Isolate = 1 : 1
> > > > CompositorWorkerManager : Compositor workers = 1 : N
> > > > Compositor worker : WorkerThread = 1 : 1
> > > 
> > > Thanks for the clarification!
> > > 
> > > It seems that the current class design doesn't reflect the concept. For
> > example,
> > > in this CL WorkerThread owns an Isolate, but conceptually there is no
> > ownership
> > > relationship from WorkerThread to Isolate.
> > 
> > The WorkerThread doesn't necessarily own the Isolate. That is why we have
this
> > IsolateWrapper, which may or may not own the isolate. The IsolateWrapper for
> the
> > CompositorWorkerThread does not own the isolate, since the isolate is shared
> > with other CompositorWorkerThread instances. For other workers (e.g.
> > DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
> > IsolateWrapper used (the default one) does own the Isolate. So essentially,
> each
> > WorkerThread owns an IsolateWrapper to communicate with the isolate, but the
> > ownership of the v8::Isolate is left up to the implementation (i.e. the
> > subclass) to decide. (I have attempted to clarify this for
> WorkerIsolateWrapper
> > doc here:
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)
> > 
> > To briefly explain: each worker is represented by a WorkerThread (which is a
> bit
> > of a misnomer; we still intend to rename it to WorkerScript:
> > https://codereview.chromium.org/1115923002/), which has a WorkerGlobalScope,
> > which owns a WorkerScriptController. We do need a WorkerThread for each
> > compositor worker (because we want each worker to have its own
global-scope),
> > and because all of them live in the same thread, it is also necessary for
> these
> > workers to share the v8::Isolate. So the ownership of the v8::Isolate does
> have
> > to be different for compositor-workers from the other workers, and this
seems
> > fairly unavoidable. Does this make a reasonable amount of sense to explain
the
> > isolate ownership?
> > 
> > (Note: we do have plans on experimenting with multiple threads/isolates, and
> > this design does allow for that to happen fairly easily, but we want to
start
> > with the simpler approach)
> 
> Thanks for the details! Then would it make more sense to restructure the
classes
> like this?
> 
> class CompositorWorkerManager {
>   v8::Isolate* m_isolate; // Owns.
>   WorkerThread* m_workerThreads; // Owns.
> };
> 
> class WorkerManager {  // For normal workers
>   v8::Isolate* m_isolate; // Owns.
>   WorkerThread* m_workerThread; // Owns.
> };
> 
> class WorkerThread { };

Interesting. CompositorWorkerManager is a singleton, which is explicitly
initialized/shutdown. This makes it easy for each CompositorWorkerThread to
communicate with it directly. For the case of normal workers, it's a bit unclear
what the ownership of WorkerManager would be like (e.g., who would own a
WorkerManager?). Do you mean WorkerManager would be something like a
WorkerThreadDelegate, and the WorkerThread gets to the isolate through the
delegate interface? Is that significantly different from what we have here,
where the subclasses are providing this to WorkerThread instead?

haraken

On 2015/06/02 04:45:41, sadrul wrote: > On 2015/06/02 04:36:14, haraken wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 04:49:56 UTC) #33

On 2015/06/02 04:45:41, sadrul wrote:
> On 2015/06/02 04:36:14, haraken wrote:
> > On 2015/06/02 04:23:56, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > > File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > > Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
> > > SharedV8Isolate final : public WorkerIsolateWrapper {
> > > On 2015/06/02 03:34:26, haraken wrote:
> > > > On 2015/06/02 02:48:28, sadrul wrote:
> > > > > On 2015/06/02 02:42:23, haraken wrote:
> > > > > > On 2015/06/02 02:37:53, sadrul wrote:
> > > > > > > On 2015/06/02 02:14:49, haraken wrote:
> > > > > > > > 
> > > > > > > > I think this is mis-designed and complicated more than needed.
You
> > now
> > > > > have
> > > > > > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault,
> SharedV8Isolate,
> > > > > > > > CompositorWorkerManager (which is doing a reference counting for
> > > worker
> > > > > > > > threads).
> > > > > > > > 
> > > > > > > > In particular, I don't quite understand why you need
> > SharedV8Isolate.
> > > > Why
> > > > > > > can't
> > > > > > > > we simply create an backing Isolate when CompositorWorkerManager
> > > creates
> > > > > the
> > > > > > > > first worker and destroy the Isolate when
CompositorWorkerManager
> > > > destroys
> > > > > > the
> > > > > > > > last worker? CompositorWorkerManager should know the timing when
> the
> > > > > backing
> > > > > > > > Isolate should be created or destroyed without getting any help
of
> > > > > > > > SharedV8Isolate etc.
> > > > > > > > 
> > > > > > > > I think WorkerIsolateWrapper is a good abstraction, but I'm not
> sure
> > > if
> > > > we
> > > > > > > need
> > > > > > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > > > > > 
> > > > > > > We need SharedV8Isolate because each WorkerThread owns its own
> > > > > > > WorkerIsolateWrapper instance. For compositor workers, we cannot
> pass
> > > > > > ownership
> > > > > > > of the isolate to the first worker, since we want the manager to
> > retain
> > > > > > > ownership. So we need a version of WorkerIsolateWrapper that
doesn't
> > > take
> > > > > > > ownership of the isolate. SharedV8Isolate+CompositorWorkerManager
> > > > > essentially
> > > > > > > does as you suggest (by creating an isolate for the first worker,
> and
> > > > > > destroying
> > > > > > > when the last worker is terminated), but I suppose through some
> > > > indirection.
> > > > > > > Some amount of code-rearrangement can make this better by moving
the
> > > > > ownership
> > > > > > > out of WorkerThread (into the subclass implementations, for
> example),
> > > and
> > > > > have
> > > > > > > WorkerThread work with a raw-pointer that it receives from the
> > subclass
> > > > > > instead.
> > > > > > > This way, we won't need SharedV8Isolate. Would that me more
> palatable?
> > > > > > 
> > > > > > Just to confirm:
> > > > > > 
> > > > > > Compositor workers : CompositorWorkerManager : WorkerThread :
Isolate
> =
> > N
> > > :
> > > > 1
> > > > > :
> > > > > > 1 : 1
> > > > > > 
> > > > > > Maybe am I misunderstanding?
> > > > > 
> > > > > 
> > > > > There can be multiple WorkerThread instances. So:
> > > > > 
> > > > > CompositorWorkerManager : Isolate : Compositor workers : WorkerThread
=
> 1
> > :
> > > 1
> > > > :
> > > > > N : N
> > > > > 
> > > > > To clarify a bit more:
> > > > > 
> > > > > CompositorWorkerManager : Isolate = 1 : 1
> > > > > CompositorWorkerManager : Compositor workers = 1 : N
> > > > > Compositor worker : WorkerThread = 1 : 1
> > > > 
> > > > Thanks for the clarification!
> > > > 
> > > > It seems that the current class design doesn't reflect the concept. For
> > > example,
> > > > in this CL WorkerThread owns an Isolate, but conceptually there is no
> > > ownership
> > > > relationship from WorkerThread to Isolate.
> > > 
> > > The WorkerThread doesn't necessarily own the Isolate. That is why we have
> this
> > > IsolateWrapper, which may or may not own the isolate. The IsolateWrapper
for
> > the
> > > CompositorWorkerThread does not own the isolate, since the isolate is
shared
> > > with other CompositorWorkerThread instances. For other workers (e.g.
> > > DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
> > > IsolateWrapper used (the default one) does own the Isolate. So
essentially,
> > each
> > > WorkerThread owns an IsolateWrapper to communicate with the isolate, but
the
> > > ownership of the v8::Isolate is left up to the implementation (i.e. the
> > > subclass) to decide. (I have attempted to clarify this for
> > WorkerIsolateWrapper
> > > doc here:
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)
> > > 
> > > To briefly explain: each worker is represented by a WorkerThread (which is
a
> > bit
> > > of a misnomer; we still intend to rename it to WorkerScript:
> > > https://codereview.chromium.org/1115923002/), which has a
WorkerGlobalScope,
> > > which owns a WorkerScriptController. We do need a WorkerThread for each
> > > compositor worker (because we want each worker to have its own
> global-scope),
> > > and because all of them live in the same thread, it is also necessary for
> > these
> > > workers to share the v8::Isolate. So the ownership of the v8::Isolate does
> > have
> > > to be different for compositor-workers from the other workers, and this
> seems
> > > fairly unavoidable. Does this make a reasonable amount of sense to explain
> the
> > > isolate ownership?
> > > 
> > > (Note: we do have plans on experimenting with multiple threads/isolates,
and
> > > this design does allow for that to happen fairly easily, but we want to
> start
> > > with the simpler approach)
> > 
> > Thanks for the details! Then would it make more sense to restructure the
> classes
> > like this?
> > 
> > class CompositorWorkerManager {
> >   v8::Isolate* m_isolate; // Owns.
> >   WorkerThread* m_workerThreads; // Owns.
> > };
> > 
> > class WorkerManager {  // For normal workers
> >   v8::Isolate* m_isolate; // Owns.
> >   WorkerThread* m_workerThread; // Owns.
> > };
> > 
> > class WorkerThread { };
> 
> Interesting. CompositorWorkerManager is a singleton, which is explicitly
> initialized/shutdown. This makes it easy for each CompositorWorkerThread to
> communicate with it directly. For the case of normal workers, it's a bit
unclear
> what the ownership of WorkerManager would be like (e.g., who would own a
> WorkerManager?). Do you mean WorkerManager would be something like a
> WorkerThreadDelegate, and the WorkerThread gets to the isolate through the
> delegate interface? Is that significantly different from what we have here,
> where the subclasses are providing this to WorkerThread instead?

I'm expecting that WorkerManager (for normal workers) is a very thin wrapper of
a WorkerThread. An object that currently owns a WorkerThread can (just) own the
WorkerManager in the new model instead. Will it work?

sadrul

On 2015/06/02 04:49:56, haraken wrote: > On 2015/06/02 04:45:41, sadrul wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 04:57:08 UTC) #34

On 2015/06/02 04:49:56, haraken wrote:
> On 2015/06/02 04:45:41, sadrul wrote:
> > On 2015/06/02 04:36:14, haraken wrote:
> > > On 2015/06/02 04:23:56, sadrul wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > > > File Source/modules/compositorworker/CompositorWorkerManager.cpp
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > > > Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
> > > > SharedV8Isolate final : public WorkerIsolateWrapper {
> > > > On 2015/06/02 03:34:26, haraken wrote:
> > > > > On 2015/06/02 02:48:28, sadrul wrote:
> > > > > > On 2015/06/02 02:42:23, haraken wrote:
> > > > > > > On 2015/06/02 02:37:53, sadrul wrote:
> > > > > > > > On 2015/06/02 02:14:49, haraken wrote:
> > > > > > > > > 
> > > > > > > > > I think this is mis-designed and complicated more than needed.
> You
> > > now
> > > > > > have
> > > > > > > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault,
> > SharedV8Isolate,
> > > > > > > > > CompositorWorkerManager (which is doing a reference counting
for
> > > > worker
> > > > > > > > > threads).
> > > > > > > > > 
> > > > > > > > > In particular, I don't quite understand why you need
> > > SharedV8Isolate.
> > > > > Why
> > > > > > > > can't
> > > > > > > > > we simply create an backing Isolate when
CompositorWorkerManager
> > > > creates
> > > > > > the
> > > > > > > > > first worker and destroy the Isolate when
> CompositorWorkerManager
> > > > > destroys
> > > > > > > the
> > > > > > > > > last worker? CompositorWorkerManager should know the timing
when
> > the
> > > > > > backing
> > > > > > > > > Isolate should be created or destroyed without getting any
help
> of
> > > > > > > > > SharedV8Isolate etc.
> > > > > > > > > 
> > > > > > > > > I think WorkerIsolateWrapper is a good abstraction, but I'm
not
> > sure
> > > > if
> > > > > we
> > > > > > > > need
> > > > > > > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > > > > > > 
> > > > > > > > We need SharedV8Isolate because each WorkerThread owns its own
> > > > > > > > WorkerIsolateWrapper instance. For compositor workers, we cannot
> > pass
> > > > > > > ownership
> > > > > > > > of the isolate to the first worker, since we want the manager to
> > > retain
> > > > > > > > ownership. So we need a version of WorkerIsolateWrapper that
> doesn't
> > > > take
> > > > > > > > ownership of the isolate.
SharedV8Isolate+CompositorWorkerManager
> > > > > > essentially
> > > > > > > > does as you suggest (by creating an isolate for the first
worker,
> > and
> > > > > > > destroying
> > > > > > > > when the last worker is terminated), but I suppose through some
> > > > > indirection.
> > > > > > > > Some amount of code-rearrangement can make this better by moving
> the
> > > > > > ownership
> > > > > > > > out of WorkerThread (into the subclass implementations, for
> > example),
> > > > and
> > > > > > have
> > > > > > > > WorkerThread work with a raw-pointer that it receives from the
> > > subclass
> > > > > > > instead.
> > > > > > > > This way, we won't need SharedV8Isolate. Would that me more
> > palatable?
> > > > > > > 
> > > > > > > Just to confirm:
> > > > > > > 
> > > > > > > Compositor workers : CompositorWorkerManager : WorkerThread :
> Isolate
> > =
> > > N
> > > > :
> > > > > 1
> > > > > > :
> > > > > > > 1 : 1
> > > > > > > 
> > > > > > > Maybe am I misunderstanding?
> > > > > > 
> > > > > > 
> > > > > > There can be multiple WorkerThread instances. So:
> > > > > > 
> > > > > > CompositorWorkerManager : Isolate : Compositor workers :
WorkerThread
> =
> > 1
> > > :
> > > > 1
> > > > > :
> > > > > > N : N
> > > > > > 
> > > > > > To clarify a bit more:
> > > > > > 
> > > > > > CompositorWorkerManager : Isolate = 1 : 1
> > > > > > CompositorWorkerManager : Compositor workers = 1 : N
> > > > > > Compositor worker : WorkerThread = 1 : 1
> > > > > 
> > > > > Thanks for the clarification!
> > > > > 
> > > > > It seems that the current class design doesn't reflect the concept.
For
> > > > example,
> > > > > in this CL WorkerThread owns an Isolate, but conceptually there is no
> > > > ownership
> > > > > relationship from WorkerThread to Isolate.
> > > > 
> > > > The WorkerThread doesn't necessarily own the Isolate. That is why we
have
> > this
> > > > IsolateWrapper, which may or may not own the isolate. The IsolateWrapper
> for
> > > the
> > > > CompositorWorkerThread does not own the isolate, since the isolate is
> shared
> > > > with other CompositorWorkerThread instances. For other workers (e.g.
> > > > DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
> > > > IsolateWrapper used (the default one) does own the Isolate. So
> essentially,
> > > each
> > > > WorkerThread owns an IsolateWrapper to communicate with the isolate, but
> the
> > > > ownership of the v8::Isolate is left up to the implementation (i.e. the
> > > > subclass) to decide. (I have attempted to clarify this for
> > > WorkerIsolateWrapper
> > > > doc here:
> > > >
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)
> > > > 
> > > > To briefly explain: each worker is represented by a WorkerThread (which
is
> a
> > > bit
> > > > of a misnomer; we still intend to rename it to WorkerScript:
> > > > https://codereview.chromium.org/1115923002/), which has a
> WorkerGlobalScope,
> > > > which owns a WorkerScriptController. We do need a WorkerThread for each
> > > > compositor worker (because we want each worker to have its own
> > global-scope),
> > > > and because all of them live in the same thread, it is also necessary
for
> > > these
> > > > workers to share the v8::Isolate. So the ownership of the v8::Isolate
does
> > > have
> > > > to be different for compositor-workers from the other workers, and this
> > seems
> > > > fairly unavoidable. Does this make a reasonable amount of sense to
explain
> > the
> > > > isolate ownership?
> > > > 
> > > > (Note: we do have plans on experimenting with multiple threads/isolates,
> and
> > > > this design does allow for that to happen fairly easily, but we want to
> > start
> > > > with the simpler approach)
> > > 
> > > Thanks for the details! Then would it make more sense to restructure the
> > classes
> > > like this?
> > > 
> > > class CompositorWorkerManager {
> > >   v8::Isolate* m_isolate; // Owns.
> > >   WorkerThread* m_workerThreads; // Owns.
> > > };
> > > 
> > > class WorkerManager {  // For normal workers
> > >   v8::Isolate* m_isolate; // Owns.
> > >   WorkerThread* m_workerThread; // Owns.
> > > };
> > > 
> > > class WorkerThread { };
> > 
> > Interesting. CompositorWorkerManager is a singleton, which is explicitly
> > initialized/shutdown. This makes it easy for each CompositorWorkerThread to
> > communicate with it directly. For the case of normal workers, it's a bit
> unclear
> > what the ownership of WorkerManager would be like (e.g., who would own a
> > WorkerManager?). Do you mean WorkerManager would be something like a
> > WorkerThreadDelegate, and the WorkerThread gets to the isolate through the
> > delegate interface? Is that significantly different from what we have here,
> > where the subclasses are providing this to WorkerThread instead?
> 
> I'm expecting that WorkerManager (for normal workers) is a very thin wrapper
of
> a WorkerThread. An object that currently owns a WorkerThread can (just) own
the
> WorkerManager in the new model instead. Will it work?

I think so, yes. But ... and sorry if I am missing something obvious, but if we
really wanted something like that, wouldn't it be simpler if we make the
subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead (for
example, in patchset7 in this CL)? Adding a new class for normal workers doesn't
seem like a gain in this case.

kinuko@: maybe I am missing something. What do you think?

haraken

On 2015/06/02 04:57:08, sadrul wrote: > On 2015/06/02 04:49:56, haraken wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 05:07:42 UTC) #35

On 2015/06/02 04:57:08, sadrul wrote:
> On 2015/06/02 04:49:56, haraken wrote:
> > On 2015/06/02 04:45:41, sadrul wrote:
> > > On 2015/06/02 04:36:14, haraken wrote:
> > > > On 2015/06/02 04:23:56, sadrul wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > > > > File Source/modules/compositorworker/CompositorWorkerManager.cpp
> (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/modules/composi...
> > > > > Source/modules/compositorworker/CompositorWorkerManager.cpp:17: class
> > > > > SharedV8Isolate final : public WorkerIsolateWrapper {
> > > > > On 2015/06/02 03:34:26, haraken wrote:
> > > > > > On 2015/06/02 02:48:28, sadrul wrote:
> > > > > > > On 2015/06/02 02:42:23, haraken wrote:
> > > > > > > > On 2015/06/02 02:37:53, sadrul wrote:
> > > > > > > > > On 2015/06/02 02:14:49, haraken wrote:
> > > > > > > > > > 
> > > > > > > > > > I think this is mis-designed and complicated more than
needed.
> > You
> > > > now
> > > > > > > have
> > > > > > > > > > WorkerIsolateWrapper, WorkerIsolateWrapperDefault,
> > > SharedV8Isolate,
> > > > > > > > > > CompositorWorkerManager (which is doing a reference counting
> for
> > > > > worker
> > > > > > > > > > threads).
> > > > > > > > > > 
> > > > > > > > > > In particular, I don't quite understand why you need
> > > > SharedV8Isolate.
> > > > > > Why
> > > > > > > > > can't
> > > > > > > > > > we simply create an backing Isolate when
> CompositorWorkerManager
> > > > > creates
> > > > > > > the
> > > > > > > > > > first worker and destroy the Isolate when
> > CompositorWorkerManager
> > > > > > destroys
> > > > > > > > the
> > > > > > > > > > last worker? CompositorWorkerManager should know the timing
> when
> > > the
> > > > > > > backing
> > > > > > > > > > Isolate should be created or destroyed without getting any
> help
> > of
> > > > > > > > > > SharedV8Isolate etc.
> > > > > > > > > > 
> > > > > > > > > > I think WorkerIsolateWrapper is a good abstraction, but I'm
> not
> > > sure
> > > > > if
> > > > > > we
> > > > > > > > > need
> > > > > > > > > > WorkerIsolateWrapperDefault and SharedV8Isolate.
> > > > > > > > > 
> > > > > > > > > We need SharedV8Isolate because each WorkerThread owns its own
> > > > > > > > > WorkerIsolateWrapper instance. For compositor workers, we
cannot
> > > pass
> > > > > > > > ownership
> > > > > > > > > of the isolate to the first worker, since we want the manager
to
> > > > retain
> > > > > > > > > ownership. So we need a version of WorkerIsolateWrapper that
> > doesn't
> > > > > take
> > > > > > > > > ownership of the isolate.
> SharedV8Isolate+CompositorWorkerManager
> > > > > > > essentially
> > > > > > > > > does as you suggest (by creating an isolate for the first
> worker,
> > > and
> > > > > > > > destroying
> > > > > > > > > when the last worker is terminated), but I suppose through
some
> > > > > > indirection.
> > > > > > > > > Some amount of code-rearrangement can make this better by
moving
> > the
> > > > > > > ownership
> > > > > > > > > out of WorkerThread (into the subclass implementations, for
> > > example),
> > > > > and
> > > > > > > have
> > > > > > > > > WorkerThread work with a raw-pointer that it receives from the
> > > > subclass
> > > > > > > > instead.
> > > > > > > > > This way, we won't need SharedV8Isolate. Would that me more
> > > palatable?
> > > > > > > > 
> > > > > > > > Just to confirm:
> > > > > > > > 
> > > > > > > > Compositor workers : CompositorWorkerManager : WorkerThread :
> > Isolate
> > > =
> > > > N
> > > > > :
> > > > > > 1
> > > > > > > :
> > > > > > > > 1 : 1
> > > > > > > > 
> > > > > > > > Maybe am I misunderstanding?
> > > > > > > 
> > > > > > > 
> > > > > > > There can be multiple WorkerThread instances. So:
> > > > > > > 
> > > > > > > CompositorWorkerManager : Isolate : Compositor workers :
> WorkerThread
> > =
> > > 1
> > > > :
> > > > > 1
> > > > > > :
> > > > > > > N : N
> > > > > > > 
> > > > > > > To clarify a bit more:
> > > > > > > 
> > > > > > > CompositorWorkerManager : Isolate = 1 : 1
> > > > > > > CompositorWorkerManager : Compositor workers = 1 : N
> > > > > > > Compositor worker : WorkerThread = 1 : 1
> > > > > > 
> > > > > > Thanks for the clarification!
> > > > > > 
> > > > > > It seems that the current class design doesn't reflect the concept.
> For
> > > > > example,
> > > > > > in this CL WorkerThread owns an Isolate, but conceptually there is
no
> > > > > ownership
> > > > > > relationship from WorkerThread to Isolate.
> > > > > 
> > > > > The WorkerThread doesn't necessarily own the Isolate. That is why we
> have
> > > this
> > > > > IsolateWrapper, which may or may not own the isolate. The
IsolateWrapper
> > for
> > > > the
> > > > > CompositorWorkerThread does not own the isolate, since the isolate is
> > shared
> > > > > with other CompositorWorkerThread instances. For other workers (e.g.
> > > > > DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
> > > > > IsolateWrapper used (the default one) does own the Isolate. So
> > essentially,
> > > > each
> > > > > WorkerThread owns an IsolateWrapper to communicate with the isolate,
but
> > the
> > > > > ownership of the v8::Isolate is left up to the implementation (i.e.
the
> > > > > subclass) to decide. (I have attempted to clarify this for
> > > > WorkerIsolateWrapper
> > > > > doc here:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)
> > > > > 
> > > > > To briefly explain: each worker is represented by a WorkerThread
(which
> is
> > a
> > > > bit
> > > > > of a misnomer; we still intend to rename it to WorkerScript:
> > > > > https://codereview.chromium.org/1115923002/), which has a
> > WorkerGlobalScope,
> > > > > which owns a WorkerScriptController. We do need a WorkerThread for
each
> > > > > compositor worker (because we want each worker to have its own
> > > global-scope),
> > > > > and because all of them live in the same thread, it is also necessary
> for
> > > > these
> > > > > workers to share the v8::Isolate. So the ownership of the v8::Isolate
> does
> > > > have
> > > > > to be different for compositor-workers from the other workers, and
this
> > > seems
> > > > > fairly unavoidable. Does this make a reasonable amount of sense to
> explain
> > > the
> > > > > isolate ownership?
> > > > > 
> > > > > (Note: we do have plans on experimenting with multiple
threads/isolates,
> > and
> > > > > this design does allow for that to happen fairly easily, but we want
to
> > > start
> > > > > with the simpler approach)
> > > > 
> > > > Thanks for the details! Then would it make more sense to restructure the
> > > classes
> > > > like this?
> > > > 
> > > > class CompositorWorkerManager {
> > > >   v8::Isolate* m_isolate; // Owns.
> > > >   WorkerThread* m_workerThreads; // Owns.
> > > > };
> > > > 
> > > > class WorkerManager {  // For normal workers
> > > >   v8::Isolate* m_isolate; // Owns.
> > > >   WorkerThread* m_workerThread; // Owns.
> > > > };
> > > > 
> > > > class WorkerThread { };
> > > 
> > > Interesting. CompositorWorkerManager is a singleton, which is explicitly
> > > initialized/shutdown. This makes it easy for each CompositorWorkerThread
to
> > > communicate with it directly. For the case of normal workers, it's a bit
> > unclear
> > > what the ownership of WorkerManager would be like (e.g., who would own a
> > > WorkerManager?). Do you mean WorkerManager would be something like a
> > > WorkerThreadDelegate, and the WorkerThread gets to the isolate through the
> > > delegate interface? Is that significantly different from what we have
here,
> > > where the subclasses are providing this to WorkerThread instead?
> > 
> > I'm expecting that WorkerManager (for normal workers) is a very thin wrapper
> of
> > a WorkerThread. An object that currently owns a WorkerThread can (just) own
> the
> > WorkerManager in the new model instead. Will it work?
> 
> I think so, yes. But ... and sorry if I am missing something obvious, but if
we
> really wanted something like that, wouldn't it be simpler if we make the
> subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead (for
> example, in patchset7 in this CL)? Adding a new class for normal workers
doesn't
> seem like a gain in this case.

Yeah, now I'm getting to understand what you want to do :)

Then a better idea would be:

class WorkerThread {  // Doesn't own an isolate.
  OwnPtr<WebThreadSupportingGC> m_thread;
};

class DedicatedWorkerThread : public WorkerThread {
  OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
};

class SharedWorkerThread : public WorkerThread {
  OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
};

class CompositorWorkerThread : public WorkerThread { // Don't hold a raw pointer
to WebThreadSupportingGC.
  v8::Isolate* isolate() { return
CompositorWorkerManager::instance()->isolate(); }
};

class CompositorWorkerManager {  // Singleton.
  v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
  OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
};

Then I guess we won't need WorkerIsolateWrapperDefault or SharedV8Isolate.

What do you think?

sadrul

[snipping earlier snippets since we seem to have hit rietveld comment limit of 10K characters] ...

5 years, 6 months ago (2015-06-02 05:37:36 UTC) #36

[snipping earlier snippets since we seem to have hit rietveld comment limit of
10K characters]

> > > 
> > > I'm expecting that WorkerManager (for normal workers) is a very thin
wrapper
> > of
> > > a WorkerThread. An object that currently owns a WorkerThread can (just)
own
> > the
> > > WorkerManager in the new model instead. Will it work?
> > 
> > I think so, yes. But ... and sorry if I am missing something obvious, but if
> we
> > really wanted something like that, wouldn't it be simpler if we make the
> > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead (for
> > example, in patchset7 in this CL)? Adding a new class for normal workers
> doesn't
> > seem like a gain in this case.
> 
> Yeah, now I'm getting to understand what you want to do :)
> 
> Then a better idea would be:
> 
> class WorkerThread {  // Doesn't own an isolate.
>   OwnPtr<WebThreadSupportingGC> m_thread;
> };
> 
> class DedicatedWorkerThread : public WorkerThread {
>   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> };
> 
> class SharedWorkerThread : public WorkerThread {
>   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> };
> 
> class CompositorWorkerThread : public WorkerThread { // Don't hold a raw
pointer
> to WebThreadSupportingGC.
>   v8::Isolate* isolate() { return
> CompositorWorkerManager::instance()->isolate(); }
> };
> 
> class CompositorWorkerManager {  // Singleton.
>   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
>   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> };
> 
> Then I guess we won't need WorkerIsolateWrapperDefault or SharedV8Isolate.
> 
> What do you think?

Yep. I think that would work too. Although we would need to bring back the
virtual functions in WorkerThread for managing the isolate (i.e.
willDestroyIsolate(), destroyIsolate()), and provide implementations in the
subclasses (or maybe have default implementation in WorkerThread?), so that for
normal workers, these functions do directly destroy the isolate, but for
compositor workers, the implementation (i.e. CompositorWorkerThread)
communicates with CompositorWorkerManager.

sadrul

On 2015/06/02 05:37:36, sadrul wrote: > [snipping earlier snippets since we seem to have hit ...

5 years, 6 months ago (2015-06-02 06:03:44 UTC) #37

On 2015/06/02 05:37:36, sadrul wrote:
> [snipping earlier snippets since we seem to have hit rietveld comment limit of
> 10K characters]
> 
> > > > 
> > > > I'm expecting that WorkerManager (for normal workers) is a very thin
> wrapper
> > > of
> > > > a WorkerThread. An object that currently owns a WorkerThread can (just)
> own
> > > the
> > > > WorkerManager in the new model instead. Will it work?
> > > 
> > > I think so, yes. But ... and sorry if I am missing something obvious, but
if
> > we
> > > really wanted something like that, wouldn't it be simpler if we make the
> > > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead (for
> > > example, in patchset7 in this CL)? Adding a new class for normal workers
> > doesn't
> > > seem like a gain in this case.
> > 
> > Yeah, now I'm getting to understand what you want to do :)
> > 
> > Then a better idea would be:
> > 
> > class WorkerThread {  // Doesn't own an isolate.
> >   OwnPtr<WebThreadSupportingGC> m_thread;
> > };
> > 
> > class DedicatedWorkerThread : public WorkerThread {
> >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > };
> > 
> > class SharedWorkerThread : public WorkerThread {
> >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > };
> > 
> > class CompositorWorkerThread : public WorkerThread { // Don't hold a raw
> pointer
> > to WebThreadSupportingGC.
> >   v8::Isolate* isolate() { return
> > CompositorWorkerManager::instance()->isolate(); }
> > };
> > 
> > class CompositorWorkerManager {  // Singleton.
> >   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
> >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> > };
> > 
> > Then I guess we won't need WorkerIsolateWrapperDefault or SharedV8Isolate.
> > 
> > What do you think?
> 
> Yep. I think that would work too. Although we would need to bring back the
> virtual functions in WorkerThread for managing the isolate (i.e.
> willDestroyIsolate(), destroyIsolate()), and provide implementations in the
> subclasses (or maybe have default implementation in WorkerThread?), so that
for
> normal workers, these functions do directly destroy the isolate, but for
> compositor workers, the implementation (i.e. CompositorWorkerThread)
> communicates with CompositorWorkerManager.

... and to add a bit more to this :) this is essentially what this CL does
through WorkerIsolateWrapper, where the various WorkerThread-subclasses dictate
the isolate ownership, and WorkerThread manages the isolate through the
WorkerIsolateWrapper (which I feel is slightly cleaner), instead of through
virtual interfaces in WorkerThread. I can make the change as you suggest though
and remove WorkerIsolateWrapper (and its subclasses) if you think that's
cleaner.

kinuko

On 2015/06/02 06:03:44, sadrul wrote: > On 2015/06/02 05:37:36, sadrul wrote: > > [snipping earlier ...

5 years, 6 months ago (2015-06-02 10:20:54 UTC) #38

On 2015/06/02 06:03:44, sadrul wrote:
> On 2015/06/02 05:37:36, sadrul wrote:
> > [snipping earlier snippets since we seem to have hit rietveld comment limit
of
> > 10K characters]
> > 
> > > > > 
> > > > > I'm expecting that WorkerManager (for normal workers) is a very thin
> > wrapper
> > > > of
> > > > > a WorkerThread. An object that currently owns a WorkerThread can
(just)
> > own
> > > > the
> > > > > WorkerManager in the new model instead. Will it work?
> > > > 
> > > > I think so, yes. But ... and sorry if I am missing something obvious,
but
> if
> > > we
> > > > really wanted something like that, wouldn't it be simpler if we make the
> > > > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead
(for
> > > > example, in patchset7 in this CL)? Adding a new class for normal workers
> > > doesn't
> > > > seem like a gain in this case.
> > > 
> > > Yeah, now I'm getting to understand what you want to do :)
> > > 
> > > Then a better idea would be:
> > > 
> > > class WorkerThread {  // Doesn't own an isolate.
> > >   OwnPtr<WebThreadSupportingGC> m_thread;
> > > };
> > > 
> > > class DedicatedWorkerThread : public WorkerThread {
> > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > };
> > > 
> > > class SharedWorkerThread : public WorkerThread {
> > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > };
> > > 
> > > class CompositorWorkerThread : public WorkerThread { // Don't hold a raw
> > pointer
> > > to WebThreadSupportingGC.
> > >   v8::Isolate* isolate() { return
> > > CompositorWorkerManager::instance()->isolate(); }
> > > };
> > > 
> > > class CompositorWorkerManager {  // Singleton.
> > >   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
> > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> > > };
> > > 
> > > Then I guess we won't need WorkerIsolateWrapperDefault or SharedV8Isolate.
> > > 
> > > What do you think?
> > 
> > Yep. I think that would work too. Although we would need to bring back the
> > virtual functions in WorkerThread for managing the isolate (i.e.
> > willDestroyIsolate(), destroyIsolate()), and provide implementations in the
> > subclasses (or maybe have default implementation in WorkerThread?), so that
> for
> > normal workers, these functions do directly destroy the isolate, but for
> > compositor workers, the implementation (i.e. CompositorWorkerThread)
> > communicates with CompositorWorkerManager.
> 
> ... and to add a bit more to this :) this is essentially what this CL does
> through WorkerIsolateWrapper, where the various WorkerThread-subclasses
dictate
> the isolate ownership, and WorkerThread manages the isolate through the
> WorkerIsolateWrapper (which I feel is slightly cleaner), instead of through
> virtual interfaces in WorkerThread. I can make the change as you suggest
though
> and remove WorkerIsolateWrapper (and its subclasses) if you think that's
> cleaner.

I think at this point it'd be probably better to fall back to the most basic
design (i.e. normal workers own their isolates while compositor worker doesn't,
without the indirection of IsolateWrapper) even if we end up a little more code
in WorkerThread and normal workers. I had a bit hard time convincing myself with
the current design, and seeing similar concerns discussed between you and
haraken@, it's probably showing signals that the current design is not
representing the background ownership model in an understandable way.

sadrul

On 2015/06/02 10:20:54, kinuko wrote: > On 2015/06/02 06:03:44, sadrul wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 12:23:13 UTC) #39

On 2015/06/02 10:20:54, kinuko wrote:
> On 2015/06/02 06:03:44, sadrul wrote:
> > On 2015/06/02 05:37:36, sadrul wrote:
> > > [snipping earlier snippets since we seem to have hit rietveld comment
limit
> of
> > > 10K characters]
> > > 
> > > > > > 
> > > > > > I'm expecting that WorkerManager (for normal workers) is a very thin
> > > wrapper
> > > > > of
> > > > > > a WorkerThread. An object that currently owns a WorkerThread can
> (just)
> > > own
> > > > > the
> > > > > > WorkerManager in the new model instead. Will it work?
> > > > > 
> > > > > I think so, yes. But ... and sorry if I am missing something obvious,
> but
> > if
> > > > we
> > > > > really wanted something like that, wouldn't it be simpler if we make
the
> > > > > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead
> (for
> > > > > example, in patchset7 in this CL)? Adding a new class for normal
workers
> > > > doesn't
> > > > > seem like a gain in this case.
> > > > 
> > > > Yeah, now I'm getting to understand what you want to do :)
> > > > 
> > > > Then a better idea would be:
> > > > 
> > > > class WorkerThread {  // Doesn't own an isolate.
> > > >   OwnPtr<WebThreadSupportingGC> m_thread;
> > > > };
> > > > 
> > > > class DedicatedWorkerThread : public WorkerThread {
> > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > };
> > > > 
> > > > class SharedWorkerThread : public WorkerThread {
> > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > };
> > > > 
> > > > class CompositorWorkerThread : public WorkerThread { // Don't hold a raw
> > > pointer
> > > > to WebThreadSupportingGC.
> > > >   v8::Isolate* isolate() { return
> > > > CompositorWorkerManager::instance()->isolate(); }
> > > > };
> > > > 
> > > > class CompositorWorkerManager {  // Singleton.
> > > >   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
> > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> > > > };
> > > > 
> > > > Then I guess we won't need WorkerIsolateWrapperDefault or
SharedV8Isolate.
> > > > 
> > > > What do you think?
> > > 
> > > Yep. I think that would work too. Although we would need to bring back the
> > > virtual functions in WorkerThread for managing the isolate (i.e.
> > > willDestroyIsolate(), destroyIsolate()), and provide implementations in
the
> > > subclasses (or maybe have default implementation in WorkerThread?), so
that
> > for
> > > normal workers, these functions do directly destroy the isolate, but for
> > > compositor workers, the implementation (i.e. CompositorWorkerThread)
> > > communicates with CompositorWorkerManager.
> > 
> > ... and to add a bit more to this :) this is essentially what this CL does
> > through WorkerIsolateWrapper, where the various WorkerThread-subclasses
> dictate
> > the isolate ownership, and WorkerThread manages the isolate through the
> > WorkerIsolateWrapper (which I feel is slightly cleaner), instead of through
> > virtual interfaces in WorkerThread. I can make the change as you suggest
> though
> > and remove WorkerIsolateWrapper (and its subclasses) if you think that's
> > cleaner.
> 
> I think at this point it'd be probably better to fall back to the most basic
> design (i.e. normal workers own their isolates while compositor worker
doesn't,
> without the indirection of IsolateWrapper) even if we end up a little more
code
> in WorkerThread and normal workers. I had a bit hard time convincing myself
with
> the current design, and seeing similar concerns discussed between you and
> haraken@, it's probably showing signals that the current design is not
> representing the background ownership model in an understandable way.

So, just to clarify, you are suggesting making initializeIsolate(),
willDestroyIsolate(), destroyIsolate(), and terminateV8EXecution() in
WorkerThread all pure virtual, and implement them in the subclasses?

haraken

On 2015/06/02 12:23:13, sadrul wrote: > On 2015/06/02 10:20:54, kinuko wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 14:22:03 UTC) #40

On 2015/06/02 12:23:13, sadrul wrote:
> On 2015/06/02 10:20:54, kinuko wrote:
> > On 2015/06/02 06:03:44, sadrul wrote:
> > > On 2015/06/02 05:37:36, sadrul wrote:
> > > > [snipping earlier snippets since we seem to have hit rietveld comment
> limit
> > of
> > > > 10K characters]
> > > > 
> > > > > > > 
> > > > > > > I'm expecting that WorkerManager (for normal workers) is a very
thin
> > > > wrapper
> > > > > > of
> > > > > > > a WorkerThread. An object that currently owns a WorkerThread can
> > (just)
> > > > own
> > > > > > the
> > > > > > > WorkerManager in the new model instead. Will it work?
> > > > > > 
> > > > > > I think so, yes. But ... and sorry if I am missing something
obvious,
> > but
> > > if
> > > > > we
> > > > > > really wanted something like that, wouldn't it be simpler if we make
> the
> > > > > > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate instead
> > (for
> > > > > > example, in patchset7 in this CL)? Adding a new class for normal
> workers
> > > > > doesn't
> > > > > > seem like a gain in this case.
> > > > > 
> > > > > Yeah, now I'm getting to understand what you want to do :)
> > > > > 
> > > > > Then a better idea would be:
> > > > > 
> > > > > class WorkerThread {  // Doesn't own an isolate.
> > > > >   OwnPtr<WebThreadSupportingGC> m_thread;
> > > > > };
> > > > > 
> > > > > class DedicatedWorkerThread : public WorkerThread {
> > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > > };
> > > > > 
> > > > > class SharedWorkerThread : public WorkerThread {
> > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > > };
> > > > > 
> > > > > class CompositorWorkerThread : public WorkerThread { // Don't hold a
raw
> > > > pointer
> > > > > to WebThreadSupportingGC.
> > > > >   v8::Isolate* isolate() { return
> > > > > CompositorWorkerManager::instance()->isolate(); }
> > > > > };
> > > > > 
> > > > > class CompositorWorkerManager {  // Singleton.
> > > > >   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
> > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> > > > > };
> > > > > 
> > > > > Then I guess we won't need WorkerIsolateWrapperDefault or
> SharedV8Isolate.
> > > > > 
> > > > > What do you think?
> > > > 
> > > > Yep. I think that would work too. Although we would need to bring back
the
> > > > virtual functions in WorkerThread for managing the isolate (i.e.
> > > > willDestroyIsolate(), destroyIsolate()), and provide implementations in
> the
> > > > subclasses (or maybe have default implementation in WorkerThread?), so
> that
> > > for
> > > > normal workers, these functions do directly destroy the isolate, but for
> > > > compositor workers, the implementation (i.e. CompositorWorkerThread)
> > > > communicates with CompositorWorkerManager.
> > > 
> > > ... and to add a bit more to this :) this is essentially what this CL does
> > > through WorkerIsolateWrapper, where the various WorkerThread-subclasses
> > dictate
> > > the isolate ownership, and WorkerThread manages the isolate through the
> > > WorkerIsolateWrapper (which I feel is slightly cleaner), instead of
through
> > > virtual interfaces in WorkerThread. I can make the change as you suggest
> > though
> > > and remove WorkerIsolateWrapper (and its subclasses) if you think that's
> > > cleaner.
> > 
> > I think at this point it'd be probably better to fall back to the most basic
> > design (i.e. normal workers own their isolates while compositor worker
> doesn't,
> > without the indirection of IsolateWrapper) even if we end up a little more
> code
> > in WorkerThread and normal workers. I had a bit hard time convincing myself
> with
> > the current design, and seeing similar concerns discussed between you and
> > haraken@, it's probably showing signals that the current design is not
> > representing the background ownership model in an understandable way.
> 
> So, just to clarify, you are suggesting making initializeIsolate(),
> willDestroyIsolate(), destroyIsolate(), and terminateV8EXecution() in
> WorkerThread all pure virtual, and implement them in the subclasses?

Or you can add a default implementation to WorkerThread (instead of making them
pure virtual) and override the implementation in CompositorWorkerThread.

kinuko

On 2015/06/02 14:22:03, haraken wrote: > On 2015/06/02 12:23:13, sadrul wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 15:46:34 UTC) #41

On 2015/06/02 14:22:03, haraken wrote:
> On 2015/06/02 12:23:13, sadrul wrote:
> > On 2015/06/02 10:20:54, kinuko wrote:
> > > On 2015/06/02 06:03:44, sadrul wrote:
> > > > On 2015/06/02 05:37:36, sadrul wrote:
> > > > > [snipping earlier snippets since we seem to have hit rietveld comment
> > limit
> > > of
> > > > > 10K characters]
> > > > > 
> > > > > > > > 
> > > > > > > > I'm expecting that WorkerManager (for normal workers) is a very
> thin
> > > > > wrapper
> > > > > > > of
> > > > > > > > a WorkerThread. An object that currently owns a WorkerThread can
> > > (just)
> > > > > own
> > > > > > > the
> > > > > > > > WorkerManager in the new model instead. Will it work?
> > > > > > > 
> > > > > > > I think so, yes. But ... and sorry if I am missing something
> obvious,
> > > but
> > > > if
> > > > > > we
> > > > > > > really wanted something like that, wouldn't it be simpler if we
make
> > the
> > > > > > > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate
instead
> > > (for
> > > > > > > example, in patchset7 in this CL)? Adding a new class for normal
> > workers
> > > > > > doesn't
> > > > > > > seem like a gain in this case.
> > > > > > 
> > > > > > Yeah, now I'm getting to understand what you want to do :)
> > > > > > 
> > > > > > Then a better idea would be:
> > > > > > 
> > > > > > class WorkerThread {  // Doesn't own an isolate.
> > > > > >   OwnPtr<WebThreadSupportingGC> m_thread;
> > > > > > };
> > > > > > 
> > > > > > class DedicatedWorkerThread : public WorkerThread {
> > > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > > > };
> > > > > > 
> > > > > > class SharedWorkerThread : public WorkerThread {
> > > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > > > };
> > > > > > 
> > > > > > class CompositorWorkerThread : public WorkerThread { // Don't hold a
> raw
> > > > > pointer
> > > > > > to WebThreadSupportingGC.
> > > > > >   v8::Isolate* isolate() { return
> > > > > > CompositorWorkerManager::instance()->isolate(); }
> > > > > > };
> > > > > > 
> > > > > > class CompositorWorkerManager {  // Singleton.
> > > > > >   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
> > > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> > > > > > };
> > > > > > 
> > > > > > Then I guess we won't need WorkerIsolateWrapperDefault or
> > SharedV8Isolate.
> > > > > > 
> > > > > > What do you think?
> > > > > 
> > > > > Yep. I think that would work too. Although we would need to bring back
> the
> > > > > virtual functions in WorkerThread for managing the isolate (i.e.
> > > > > willDestroyIsolate(), destroyIsolate()), and provide implementations
in
> > the
> > > > > subclasses (or maybe have default implementation in WorkerThread?), so
> > that
> > > > for
> > > > > normal workers, these functions do directly destroy the isolate, but
for
> > > > > compositor workers, the implementation (i.e. CompositorWorkerThread)
> > > > > communicates with CompositorWorkerManager.
> > > > 
> > > > ... and to add a bit more to this :) this is essentially what this CL
does
> > > > through WorkerIsolateWrapper, where the various WorkerThread-subclasses
> > > dictate
> > > > the isolate ownership, and WorkerThread manages the isolate through the
> > > > WorkerIsolateWrapper (which I feel is slightly cleaner), instead of
> through
> > > > virtual interfaces in WorkerThread. I can make the change as you suggest
> > > though
> > > > and remove WorkerIsolateWrapper (and its subclasses) if you think that's
> > > > cleaner.
> > > 
> > > I think at this point it'd be probably better to fall back to the most
basic
> > > design (i.e. normal workers own their isolates while compositor worker
> > doesn't,
> > > without the indirection of IsolateWrapper) even if we end up a little more
> > code
> > > in WorkerThread and normal workers. I had a bit hard time convincing
myself
> > with
> > > the current design, and seeing similar concerns discussed between you and
> > > haraken@, it's probably showing signals that the current design is not
> > > representing the background ownership model in an understandable way.
> > 
> > So, just to clarify, you are suggesting making initializeIsolate(),
> > willDestroyIsolate(), destroyIsolate(), and terminateV8EXecution() in
> > WorkerThread all pure virtual, and implement them in the subclasses?
> 
> Or you can add a default implementation to WorkerThread (instead of making
them
> pure virtual) and override the implementation in CompositorWorkerThread.

Yes, as far as the ownership part is made clear having default impl is fine.

sadrul

On 2015/06/02 15:46:34, kinuko wrote: > On 2015/06/02 14:22:03, haraken wrote: > > On 2015/06/02 ...

5 years, 6 months ago (2015-06-02 20:47:07 UTC) #42

On 2015/06/02 15:46:34, kinuko wrote:
> On 2015/06/02 14:22:03, haraken wrote:
> > On 2015/06/02 12:23:13, sadrul wrote:
> > > On 2015/06/02 10:20:54, kinuko wrote:
> > > > On 2015/06/02 06:03:44, sadrul wrote:
> > > > > On 2015/06/02 05:37:36, sadrul wrote:
> > > > > > [snipping earlier snippets since we seem to have hit rietveld
comment
> > > limit
> > > > of
> > > > > > 10K characters]
> > > > > > 
> > > > > > > > > 
> > > > > > > > > I'm expecting that WorkerManager (for normal workers) is a
very
> > thin
> > > > > > wrapper
> > > > > > > > of
> > > > > > > > > a WorkerThread. An object that currently owns a WorkerThread
can
> > > > (just)
> > > > > > own
> > > > > > > > the
> > > > > > > > > WorkerManager in the new model instead. Will it work?
> > > > > > > > 
> > > > > > > > I think so, yes. But ... and sorry if I am missing something
> > obvious,
> > > > but
> > > > > if
> > > > > > > we
> > > > > > > > really wanted something like that, wouldn't it be simpler if we
> make
> > > the
> > > > > > > > subclasses (e.g. DedicatedWorkerThread etc.) own the isolate
> instead
> > > > (for
> > > > > > > > example, in patchset7 in this CL)? Adding a new class for normal
> > > workers
> > > > > > > doesn't
> > > > > > > > seem like a gain in this case.
> > > > > > > 
> > > > > > > Yeah, now I'm getting to understand what you want to do :)
> > > > > > > 
> > > > > > > Then a better idea would be:
> > > > > > > 
> > > > > > > class WorkerThread {  // Doesn't own an isolate.
> > > > > > >   OwnPtr<WebThreadSupportingGC> m_thread;
> > > > > > > };
> > > > > > > 
> > > > > > > class DedicatedWorkerThread : public WorkerThread {
> > > > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > > > > };
> > > > > > > 
> > > > > > > class SharedWorkerThread : public WorkerThread {
> > > > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;
> > > > > > > };
> > > > > > > 
> > > > > > > class CompositorWorkerThread : public WorkerThread { // Don't hold
a
> > raw
> > > > > > pointer
> > > > > > > to WebThreadSupportingGC.
> > > > > > >   v8::Isolate* isolate() { return
> > > > > > > CompositorWorkerManager::instance()->isolate(); }
> > > > > > > };
> > > > > > > 
> > > > > > > class CompositorWorkerManager {  // Singleton.
> > > > > > >   v8::Isolate* isolate() { return m_isolateWrapper->isolate(); }
> > > > > > >   OwnPtr<WorkerIsolateWrapper> m_isolateWrapper;  
> > > > > > > };
> > > > > > > 
> > > > > > > Then I guess we won't need WorkerIsolateWrapperDefault or
> > > SharedV8Isolate.
> > > > > > > 
> > > > > > > What do you think?
> > > > > > 
> > > > > > Yep. I think that would work too. Although we would need to bring
back
> > the
> > > > > > virtual functions in WorkerThread for managing the isolate (i.e.
> > > > > > willDestroyIsolate(), destroyIsolate()), and provide implementations
> in
> > > the
> > > > > > subclasses (or maybe have default implementation in WorkerThread?),
so
> > > that
> > > > > for
> > > > > > normal workers, these functions do directly destroy the isolate, but
> for
> > > > > > compositor workers, the implementation (i.e. CompositorWorkerThread)
> > > > > > communicates with CompositorWorkerManager.
> > > > > 
> > > > > ... and to add a bit more to this :) this is essentially what this CL
> does
> > > > > through WorkerIsolateWrapper, where the various
WorkerThread-subclasses
> > > > dictate
> > > > > the isolate ownership, and WorkerThread manages the isolate through
the
> > > > > WorkerIsolateWrapper (which I feel is slightly cleaner), instead of
> > through
> > > > > virtual interfaces in WorkerThread. I can make the change as you
suggest
> > > > though
> > > > > and remove WorkerIsolateWrapper (and its subclasses) if you think
that's
> > > > > cleaner.
> > > > 
> > > > I think at this point it'd be probably better to fall back to the most
> basic
> > > > design (i.e. normal workers own their isolates while compositor worker
> > > doesn't,
> > > > without the indirection of IsolateWrapper) even if we end up a little
more
> > > code
> > > > in WorkerThread and normal workers. I had a bit hard time convincing
> myself
> > > with
> > > > the current design, and seeing similar concerns discussed between you
and
> > > > haraken@, it's probably showing signals that the current design is not
> > > > representing the background ownership model in an understandable way.
> > > 
> > > So, just to clarify, you are suggesting making initializeIsolate(),
> > > willDestroyIsolate(), destroyIsolate(), and terminateV8EXecution() in
> > > WorkerThread all pure virtual, and implement them in the subclasses?
> > 
> > Or you can add a default implementation to WorkerThread (instead of making
> them
> > pure virtual) and override the implementation in CompositorWorkerThread.
> 
> Yes, as far as the ownership part is made clear having default impl is fine.

Makes sense. Thanks. Done. PTAL.

haraken

Thanks for the update! Mostly looks good to me. https://codereview.chromium.org/1158443008/diff/260001/Source/modules/compositorworker/CompositorWorkerManager.cpp File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): https://codereview.chromium.org/1158443008/diff/260001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode103 Source/modules/compositorworker/CompositorWorkerManager.cpp:103: ...

5 years, 6 months ago (2015-06-03 02:44:11 UTC) #43

sadrul

Thanks for the quick reviews! https://codereview.chromium.org/1158443008/diff/260001/Source/modules/compositorworker/CompositorWorkerManager.cpp File Source/modules/compositorworker/CompositorWorkerManager.cpp (right): https://codereview.chromium.org/1158443008/diff/260001/Source/modules/compositorworker/CompositorWorkerManager.cpp#newcode103 Source/modules/compositorworker/CompositorWorkerManager.cpp:103: m_isolate = V8PerIsolateData::initialize(); On ...

5 years, 6 months ago (2015-06-03 03:16:00 UTC) #44

Thanks for the quick reviews!

https://codereview.chromium.org/1158443008/diff/260001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1158443008/diff/260001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:103: m_isolate =
V8PerIsolateData::initialize();
On 2015/06/03 02:44:11, haraken wrote:
> 
> Can we do the isolate initialization in initializeBackingThread()?

Done.

https://codereview.chromium.org/1158443008/diff/260001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerManager.cpp:128: if (!m_thread)
{
On 2015/06/03 02:44:11, haraken wrote:
> 
> What is this check for? Is it possible that m_thread becomes nullptr (after
> we've initialized it)?
> 
> I guess you need to either of:
> 
> - Do m_thread=nullptr after calling m_thread->shutdown().
> - Change the check to if(m_workerCount == 0).

After calling m_thread->shutdown(), we schedule the thread to be destroyed in a
posted task in the main-thread. We pass the thread ownership to the callback by
calling leakPtr() on it (like 94), which does reset it back to nullptr. But I
have added code in there to do this explicitly too.

https://codereview.chromium.org/1158443008/diff/260001/Source/modules/composi...
File Source/modules/compositorworker/CompositorWorkerThread.cpp (right):

https://codereview.chromium.org/1158443008/diff/260001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.cpp:56: return
CompositorWorkerManager::instance()->initializedIsolate();
On 2015/06/03 02:44:11, haraken wrote:
> 
> initializedIsolate => initializeIsolate

Done.

https://codereview.chromium.org/1158443008/diff/260001/Source/modules/composi...
Source/modules/compositorworker/CompositorWorkerThread.cpp:71:
CompositorWorkerManager::instance()->terminateIsolate();
On 2015/06/03 02:44:11, haraken wrote:
> 
> terminateIsolate => terminateV8Execution

Done.

kinuko

Looking good, much simpler now! https://codereview.chromium.org/1158443008/diff/280001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1158443008/diff/280001/Source/core/workers/WorkerThread.cpp#newcode378 Source/core/workers/WorkerThread.cpp:378: I feel we should ...

5 years, 6 months ago (2015-06-03 05:05:03 UTC) #46

sadrul

https://codereview.chromium.org/1158443008/diff/280001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1158443008/diff/280001/Source/core/workers/WorkerThread.cpp#newcode378 Source/core/workers/WorkerThread.cpp:378: On 2015/06/03 05:05:03, kinuko wrote: > I feel we ...

5 years, 6 months ago (2015-06-03 14:31:38 UTC) #47

kinuko

On 2015/06/03 15:27:51, kinuko wrote: > lgtm Would be nice to update the CL description.

5 years, 6 months ago (2015-06-03 15:28:21 UTC) #49

sadrul

On 2015/06/03 15:28:21, kinuko wrote: > On 2015/06/03 15:27:51, kinuko wrote: > > lgtm > ...

5 years, 6 months ago (2015-06-03 16:28:04 UTC) #50

sadrul

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the ...

5 years, 6 months ago (2015-06-06 07:09:19 UTC) #52

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158443008/300001

5 years, 6 months ago (2015-06-06 07:09:28 UTC) #53

commit-bot: I haz the power

Committed patchset #13 (id:300001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196625

5 years, 6 months ago (2015-06-06 08:41:23 UTC) #54

sof

A revert of this CL (patchset #13 id:300001) has been created in https://codereview.chromium.org/1166923003/ by sigbjornf@opera.com. ...

5 years, 6 months ago (2015-06-06 08:55:43 UTC) #55

sadrul

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, haraken@chromium.org Link to ...

5 years, 6 months ago (2015-06-06 09:02:55 UTC) #57

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158443008/320001

5 years, 6 months ago (2015-06-06 09:03:00 UTC) #58

commit-bot: I haz the power

Committed patchset #14 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196627

5 years, 6 months ago (2015-06-06 10:15:00 UTC) #59

sof

sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com

5 years, 6 months ago (2015-06-07 06:17:10 UTC) #60

sof

The unit tests added here run into problems Oilpan-wise, http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%20Leak/builds/10313 but I'm a bit surprised ...

5 years, 6 months ago (2015-06-07 06:17:11 UTC) #61

sof

https://codereview.chromium.org/1158443008/diff/300001/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp File Source/modules/compositorworker/CompositorWorkerManagerTest.cpp (right): https://codereview.chromium.org/1158443008/diff/300001/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp#newcode85 Source/modules/compositorworker/CompositorWorkerManagerTest.cpp:85: WorkerClients::create(), On 2015/06/07 06:17:11, sof wrote: > This is ...

5 years, 6 months ago (2015-06-07 06:28:20 UTC) #62

sof

On 2015/06/07 06:28:20, sof wrote: > https://codereview.chromium.org/1158443008/diff/300001/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp > File Source/modules/compositorworker/CompositorWorkerManagerTest.cpp (right): > > https://codereview.chromium.org/1158443008/diff/300001/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp#newcode85 > ...

5 years, 6 months ago (2015-06-07 20:17:19 UTC) #63

sadrul

This was reverted again in https://codereview.chromium.org/1168073002. I am fixing the failures in content_browsertests with https://codereview.chromium.org/1171853003/. ...

5 years, 6 months ago (2015-06-09 17:20:04 UTC) #64

sof

On 2015/06/09 17:20:04, sadrul wrote: > This was reverted again in https://codereview.chromium.org/1168073002. I am > ...

5 years, 6 months ago (2015-06-09 18:50:16 UTC) #65

sadrul

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, haraken@chromium.org Link to ...

5 years, 6 months ago (2015-06-11 11:17:03 UTC) #67

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158443008/340001

5 years, 6 months ago (2015-06-11 11:17:18 UTC) #68

commit-bot: I haz the power

5 years, 6 months ago (2015-06-11 12:29:36 UTC) #69

Message was sent while issue was closed.

Committed patchset #15 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196944

Issue 1158443008: compositor-worker: Share a thread and an isolate for compositor workers. (Closed)

Description

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : . #

Patch Set 6 : harden-test #

Patch Set 7 : . #

Patch Set 8 : . #

Patch Set 9 : . #

Patch Set 10 : . #

Patch Set 11 : . #

Patch Set 12 : . #

Patch Set 13 : . #

Patch Set 14 : fix-build #

Patch Set 15 : tot-merge #

Messages