[WebWorkers][Chromium] Use v8 Isolates for in-process implementation of WebWorkers

[Dmitry Lomov]

V8 now supports multiple instances of virtual machine in one process, dubbed isolates.

We should use them for the implementation of shared workers in chromium port

[Dmitry Lomov]

Created attachment 94291 [details]
First bit of work

This is a first bit of the work -  by no means complete.

The main challenge is moving static bits of WebCore V8 bindings to per-isolate storage. To this end, I have added a per-isolate embedded data to V8, and this patch utilizes that to cache FuntionTemplates for V8 bindings.
Moving remainder of static state over to isolates is in the works. 

I'd appreciate comments from interested parties.

[David Levin]

Comment on attachment 94291 [details]
First bit of work

View in context: https://bugs.webkit.org/attachment.cgi?id=94291&action=review

Loks good overall.
Mostly minor style nits - a few more substantive comments.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2165
> +    if (!data->rawTemplateMap().contains(&info)) {

Use "find" instead of contains to only do the lookup once. (Right now it does contains and get).

Sketch of code:

result = data->rawTemplateMap().find(&info);
if (result != end)
    return result...;

v8::Persistent<v8::FunctionTemplate> rawTemplate = createRawTemplate();
data->rawTemplateMap().add(&info, rawTemplate);
return rawTemplate;

> Source/WebCore/bindings/v8/V8Binding.cpp:51
> +V8BindingPerIsolateData::V8BindingPerIsolateData(v8::Isolate* isolate) {

{ on next line (in many places).

> Source/WebCore/bindings/v8/V8Binding.cpp:57
> +// static

Chromium does these "static" comments but WebKit doesn't.

> Source/WebCore/bindings/v8/V8Binding.cpp:60
> +    if (embedderData != 0) {

Do use {} for single line clauses and avoid comparisons to 0.

> Source/WebCore/bindings/v8/V8Binding.cpp:71
> +    if (data != 0) {

Ditto.

> Source/WebCore/bindings/v8/V8Binding.h:59
> +        static void dispose(v8::Isolate* isolate);

isolate -- param name adds no information so don't include it.

> Source/WebCore/bindings/v8/V8Binding.h:67
> +        V8BindingPerIsolateData(v8::Isolate* isolate);

Ditto.

> Source/WebKit/chromium/src/PlatformBridge.cpp:1048
> +    return new WorkerMessagingProxy(worker);

Is there a different code path for shared workers?

[Dmitry Lomov]

(In reply to comment #2)
> (From update of attachment 94291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94291&action=review
> 
> Loks good overall.
> Mostly minor style nits - a few more substantive comments.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2165
> > +    if (!data->rawTemplateMap().contains(&info)) {
> 
> Use "find" instead of contains to only do the lookup once. (Right now it does contains and get).
> 
> Sketch of code:
> 
> result = data->rawTemplateMap().find(&info);
> if (result != end)
>     return result...;
> 
> v8::Persistent<v8::FunctionTemplate> rawTemplate = createRawTemplate();
> data->rawTemplateMap().add(&info, rawTemplate);
> return rawTemplate;
> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:51
> > +V8BindingPerIsolateData::V8BindingPerIsolateData(v8::Isolate* isolate) {
> 
> { on next line (in many places).
> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:57
> > +// static
> 
> Chromium does these "static" comments but WebKit doesn't.
> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:60
> > +    if (embedderData != 0) {
> 
> Do use {} for single line clauses and avoid comparisons to 0.
> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:71
> > +    if (data != 0) {
> 
> Ditto.
> 
> > Source/WebCore/bindings/v8/V8Binding.h:59
> > +        static void dispose(v8::Isolate* isolate);
> 
> isolate -- param name adds no information so don't include it.
> 
> > Source/WebCore/bindings/v8/V8Binding.h:67
> > +        V8BindingPerIsolateData(v8::Isolate* isolate);
> 
> Ditto.

Thanks for taking a look - fixed all of the above
> 
> > Source/WebKit/chromium/src/PlatformBridge.cpp:1048
> > +    return new WorkerMessagingProxy(worker);
> 
> Is there a different code path for shared workers?

Yes there is.

[Dmitry Lomov]

Created attachment 95694 [details]
This patch makes simple worker actually work in-process in Chrome

[Dmitry Lomov]

(In reply to comment #4)
> Created an attachment (id=95694) [details]
> This patch makes simple worker actually work in-process in Chrome

This is not a landable patch again, but gives a good overview of a minimal set of changes to be done

[Dmitry Titov]

Comment on attachment 95694 [details]
This patch makes simple worker actually work in-process in Chrome

View in context: https://bugs.webkit.org/attachment.cgi?id=95694&action=review

> Source/WebCore/bindings/v8/V8Binding.cpp:62
> +    if (embedderData) { return static_cast<V8BindingPerIsolateData*>(embedderData); }

It's possible to avoid this 'if' - by assuming the data is always there. It's possible to initialize it in the ScriptController. The 'if' will consume few clocks every time...

Also, we could have V8 expose Isolate::GetCurrentIsolateData() to avoid one extra non-inlinable call. Of course, only if it actually saves part of the expected perf problem :-)

[Dmitry Lomov]

Created attachment 103883 [details]
This adds an implementation of in-process dedicated workers to chromium port. 

The crux of the matter is the reimplementation of WebWorkerClientImpl. WebWorkerClientImpl now 
implements all three of Worker{Loader,Context,Object}Proxies and delegates the implementation to 
WebKit's standard WorkerMessagingProxy.
For now, we have 3 implementations of workers in chromium WebKit:
          - In-process dedicated workers (this checkin)
          - Inter-process shared workers
          - Inter-process dedicated workers (defunct after this checkin)
This patch extracts some new common interfaces (WebCommonWorkerBase and WebCommonWorkerClient) for these 
three implementations. 
Removing the remainings of inter-process dedicated workers -related classes is left for a separate patch 
(it will require coordinated changes on chromuium side).

Chromium trybots are happy.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103883 [details]
This adds an implementation of in-process dedicated workers to chromium port. 

View in context: https://bugs.webkit.org/attachment.cgi?id=103883&action=review

I think we can do better than adding WebCommonFooBase interfaces.  Let's try to just make use of WebCommonFoo, or switch to WebFooBase.

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:53
> +class WebCommonWorkerClient : public WebCommonWorkerClientBase {

WebCommonWorkerClientBase is a funny name.  Isn't WebCommonWorkerClient already
supposed to be the interface that represents the common functions needed by all
types of worker clients?

I see this comment above WebCommonWorkerClient:

// This interface contains common APIs used by both shared and dedicated
// workers.

> Source/WebKit/chromium/public/WebCommonWorkerClientBase.h:56
> +}

WebKit API headers normally put a comment here to indicate the close of the namespace.

You can see this in part 3 of the coding style guide: http://www.webkit.org/coding/coding-style.html

> Source/WebKit/chromium/src/WebCommonWorkerBase.h:45
> +    virtual WebView* webView() const = 0;

we normally drop the "web" prefix on method names that return a WebFoo type.

> Source/WebKit/chromium/src/WebCommonWorkerBase.h:47
> +

nit: only one new line here please

[Dmitry Lomov]

Comment on attachment 103883 [details]
This adds an implementation of in-process dedicated workers to chromium port. 

View in context: https://bugs.webkit.org/attachment.cgi?id=103883&action=review

Re Web*Base classes - as I said, after this patch, we will be in a bit of  temporary situation where we will have 2 implementations of dedicated workers, one defunct. I think complete removal of inter-process workers and subsequent clean-up should be a separate patch. Awkward naming reflect that.

>> Source/WebKit/chromium/public/WebCommonWorkerClient.h:53
>> +class WebCommonWorkerClient : public WebCommonWorkerClientBase {
> 
> WebCommonWorkerClientBase is a funny name.  Isn't WebCommonWorkerClient already
> supposed to be the interface that represents the common functions needed by all
> types of worker clients?
> 
> I see this comment above WebCommonWorkerClient:
> 
> // This interface contains common APIs used by both shared and dedicated
> // workers.

WebCommonWorkerClient unfortunately has a bunch of methods that are unnecessary and cumbersome to implement in a new world of in-proc dedicated workers (postMessage* and others that are supposed to work on _main_ thread) - hence the need for a base interface shared between all 3 worker implementation. The idea is that after the dedicated worker implementation cleanup these methods will go away and WebCommonWorkerClientBase will merge back into WebCommonWorkerClient.

>> Source/WebKit/chromium/public/WebCommonWorkerClientBase.h:56
>> +}
> 
> WebKit API headers normally put a comment here to indicate the close of the namespace.
> 
> You can see this in part 3 of the coding style guide: http://www.webkit.org/coding/coding-style.html

Done.

>> Source/WebKit/chromium/src/WebCommonWorkerBase.h:45
>> +    virtual WebView* webView() const = 0;
> 
> we normally drop the "web" prefix on method names that return a WebFoo type.

Done.

>> Source/WebKit/chromium/src/WebCommonWorkerBase.h:47
>> +
> 
> nit: only one new line here please

Done.

[Dmitry Lomov]

Created attachment 103954 [details]
CR feedback

[David Levin]

Comment on attachment 103954 [details]
CR feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=103954&action=review

> Source/WebCore/workers/WorkerMessagingProxy.h:53
> +        virtual ~WorkerMessagingProxy();

This shouldn't be exposed because no one should ever call it. This class is self-deleting. (There should probably be a comment before this destructor.)

Also it is odd that it is virtual at all as discussed.

> Source/WebKit/chromium/ChangeLog:14
> +        three implementations. 

Why are new interfaces needed?

Why are we keeping the old interfaces?

What does shared workers use? (There is a lot of code that got deleted in WebWorkerClientImpl. Did shared workers avoid using that?)

> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:68
> +    WebCommonWorkerClientBase* commonClientBase() { return m_client; }

What about the other method from WebCommonWorkerBase?

> Source/WebKit/chromium/src/WebWorkerBase.h:96
> +    virtual WebView* view() const { return m_webView; }

The comment about "WebCore::WorkerLoaderProxy methods:" should change to "WebCommonWorkerBase" and the "WebView* view" method should be part of that group.

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:82
>      //     return new WorkerMessagingProxy(worker);

Should this commented out code be removed?

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:95
> +    : m_proxy(adoptPtr(new WorkerMessagingProxy(worker)))

As noted previously, WorkerMessagingProxy is self-deleting so best not to store it in a OwnPtr.

Perhaps we should expose a WorkerMessagingProxy::create method which only returns a WorkerMessagingProxy* to emphasize this.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:53
> +using WebCore::SerializedScriptValue;

Avoid using in header files.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:-57
> -    WebWorkerClientImpl(WebCore::Worker*);

Note that Source/WebKit/chromium/src/* try to keep the methods in the file in the same order as the declaration in the class so these rearrangements here should result in the same rearrangements in the .cpp file.

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:67
> +    virtual ~WebWorkerClientImpl();

Why did this become public and why was it private before?

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:114
> +    virtual WebCommonWorkerClientBase* commonClientBase() { return this; }

WebCommonWorkerBase methods?

> Source/WebKit/chromium/src/WebWorkerImpl.cpp:77
> +WebCommonWorkerClientBase* WebWorkerImpl::commonClientBase()

Wrong location.

> Source/WebKit/chromium/src/WebWorkerImpl.h:69
> +    virtual WebCommonWorkerClientBase* commonClientBase();

other methods for WebCommonWorkerBase?

[David Levin]

Ok, I get this now.


State of the world before this patch:

Chromium relies on WebWorkerBase and WebCommonWorkerClient right now to do common operations for Workers across both shared workers and dedicated workers.

WebSharedWorker (WSW) and WebWorker derive from WebWorkerBase (WWB).

WebSharedWorkerClient (WSWC) and WebWorkerClient derive from WebCommonWorkerClient (WCWC).

Oddly, we have "Base"  and "Common" both seemingly being used to mean common functionality (maybe one is used for interfaces and the other for implementations....)

State of the world with this patch:

Chromium will no longer use ipc for dedicated workers, so WebWorker and WebWorkerClient are dead code. Also WWB and WCWC are only used by the ipc plumbing in chromium but no where else and they are essentially no longer base classes but instead part of WSW and WSWC respectively.

The introduction of new Base and Common into what will be the new versions of WWB and WCWC is pretty confusing, so instead I suggested renaming the new versions to NewWebWorkerBase and NewWebCommonWorkerClient temporarily while we do the transition of Chromium code to stop using WWB and WCWC.


Future patches (my interpretation):

1. Remove usage of WWB in Chromium and switch to either WSW or NewWebWorkerBase. Ditto for WCWC.

2. Change WebKit/chromium code to remove WWB and move the remaining functionality to WSW and typedef NewWWB WWB. Ditto for WCWC. 

3. Switch Chromium to use WWB instead of WWB. Ditto for WCWC.

4. Rename NewWWB to WWB. Ditto for WCWC.

[Dmitry Lomov]

Yes this is accurate

[Dmitry Lomov]

Comment on attachment 103954 [details]
CR feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=103954&action=review

>> Source/WebCore/workers/WorkerMessagingProxy.h:53
>> +        virtual ~WorkerMessagingProxy();
> 
> This shouldn't be exposed because no one should ever call it. This class is self-deleting. (There should probably be a comment before this destructor.)
> 
> Also it is odd that it is virtual at all as discussed.

Done.

>> Source/WebKit/chromium/ChangeLog:14
>> +        three implementations. 
> 
> Why are new interfaces needed?
> 
> Why are we keeping the old interfaces?
> 
> What does shared workers use? (There is a lot of code that got deleted in WebWorkerClientImpl. Did shared workers avoid using that?)

> Why are new interfaces needed?
> Why are we keeping the old interfaces?
As I noted already, after this patch we will have 3 implementations of workers, one defunct.  Doing a full cleanup will require coordinated changes on chromium side, that is why I am doing this in two steps
> What does shared workers use?
Nomenclature is: WebWorker is dedicated worker, WebSharedWorker is shared worker. These changes only affect dedicated workers.

>> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:68
>> +    WebCommonWorkerClientBase* commonClientBase() { return m_client; }
> 
> What about the other method from WebCommonWorkerBase?

It comes from WebWorkerBase

>> Source/WebKit/chromium/src/WebWorkerBase.h:96
>> +    virtual WebView* view() const { return m_webView; }
> 
> The comment about "WebCore::WorkerLoaderProxy methods:" should change to "WebCommonWorkerBase" and the "WebView* view" method should be part of that group.

Done

>> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:82
>>      //     return new WorkerMessagingProxy(worker);
> 
> Should this commented out code be removed?

With pleasure.

>> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:95
>> +    : m_proxy(adoptPtr(new WorkerMessagingProxy(worker)))
> 
> As noted previously, WorkerMessagingProxy is self-deleting so best not to store it in a OwnPtr.
> 
> Perhaps we should expose a WorkerMessagingProxy::create method which only returns a WorkerMessagingProxy* to emphasize this.

Done (removed OwnPtr)

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:53
>> +using WebCore::SerializedScriptValue;
> 
> Avoid using in header files.

Done.

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:-57
>> -    WebWorkerClientImpl(WebCore::Worker*);
> 
> Note that Source/WebKit/chromium/src/* try to keep the methods in the file in the same order as the declaration in the class so these rearrangements here should result in the same rearrangements in the .cpp file.

Done

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:67
>> +    virtual ~WebWorkerClientImpl();
> 
> Why did this become public and why was it private before?

Done (change undone).

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:114

> 
> WebCommonWorkerBase methods?

Done.

[Dmitry Lomov]

Created attachment 103993 [details]
CR feedback.

Addressed the comments and rename confusing *Base and Common*Base stuff into New* classes. Got rid of extra files.

[David Levin]

Comment on attachment 103993 [details]
CR feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=103993&action=review

This looks good to me pending a little bit of clean up and a few comments.

> Source/WebKit/chromium/ChangeLog:13
> +        This patch extracts some new common interfaces (WebCommonWorkerBase and WebCommonWorkerClient) for these 

Update names: WebCommonWorkerBase and WebCommonWorkerClient

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:47
> +class NewWebCommonWorkerClient {

It feels like this class should have a comment just like WebCommonWorkerClient has one.

Also all of these methods are simply about calling things on the main webkit thread so I suspect some simplification of the comments below could happen if there were a top level comment.

> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:67
> +    // WebCommonWorkerBase methods:

s/WebCommonWorkerBase/NewWebWorkerBase/

> Source/WebKit/chromium/src/WebWorkerBase.h:61
> +class NewWebWorkerBase : public WebCore::WorkerLoaderProxy {

Comment for this class (like there is one for WWB below).

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:41
> +

It doesn't seem like a blank line is needed here.

> Source/WebKit/chromium/src/WebWorkerImpl.h:68
> +    // WebCommonWorkerBase methods:

WebCommonWorkerBase update name.

[Dmitry Lomov]

Comment on attachment 103993 [details]
CR feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=103993&action=review

>> Source/WebKit/chromium/src/WebSharedWorkerImpl.h:67
>> +    // WebCommonWorkerBase methods:
> 
> s/WebCommonWorkerBase/NewWebWorkerBase/

Done.

>> Source/WebKit/chromium/src/WebWorkerBase.h:61
>> +class NewWebWorkerBase : public WebCore::WorkerLoaderProxy {
> 
> Comment for this class (like there is one for WWB below).

Done.

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:41
>> +
> 
> It doesn't seem like a blank line is needed here.

The blank line divides WebCore and WebKit headers. Most of WebKit follows this convention.

>> Source/WebKit/chromium/src/WebWorkerImpl.h:68
>> +    // WebCommonWorkerBase methods:
> 
> WebCommonWorkerBase update name.

Done.

[Dmitry Lomov]

Created attachment 104084 [details]
CR feedback (changes in comments)

[David Levin]

Comment on attachment 104084 [details]
CR feedback (changes in comments)

View in context: https://bugs.webkit.org/attachment.cgi?id=104084&action=review

LGTM.

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:56
> +class WebCommonWorkerClientBase;

Rename issue.

[Dmitry Lomov]

Created attachment 104089 [details]
Rename issue fixed (thanks David!)

[David Levin]

Comment on attachment 104089 [details]
Rename issue fixed (thanks David!)

LGTM!

[David Levin]

Comment on attachment 104089 [details]
Rename issue fixed (thanks David!)

We'll deal with any other feedback about the Chromium WebKit public api in follow up patches.

[Darin Fisher (:fishd, Google)]

Comment on attachment 104089 [details]
Rename issue fixed (thanks David!)

View in context: https://bugs.webkit.org/attachment.cgi?id=104089&action=review

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:47
> +class NewWebCommonWorkerClient {

Interfaces are supposed to be named with a Web prefix.  However, I understand
that this is just a temporary thing.

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:58
> +

nit: only one new line here

> Source/WebKit/chromium/src/WebWorkerBase.h:60
> +

nit: only one new line here

> Source/WebKit/chromium/src/WebWorkerBase.h:70
> +

nit: only one new line here

[Dmitry Lomov]

Landed in http://trac.webkit.org/changeset/93330. Closing bug.