Bug 62653 – [V8][Chromium] Make StringCache in V8 bindings per-isolate

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Lomov]

This is another prerequisite to moving dedicated webworkers in-process.

[Adam Barth]

Did you answer antonm's questions from the last patch?  It's important to make sure we're not regressing benchmarks with this work.

[Dmitry Lomov]

(In reply to comment #1)
> Did you answer antonm's questions from the last patch? 
I think I did. 

> It's important to make sure we're not regressing benchmarks with this work.
Yes, I am working on getting perf numbers from different platforms.

[Dmitry Lomov]

Created attachment 97163 [details]
The fix moves StringCache to V8BindingPerIsolateData

Here is a result of perf comparison on MacOS:
http://dromaeo.com/?dom?id=142223,142275
Overall impact on dromaeo DOM benchmarks is limited (<1%). 

I am working on getting Windows numbers as well.

[Dmitry Lomov]

Created attachment 97164 [details]
Quick self-review

[Adam Barth]

> > Did you answer antonm's questions from the last patch? 
> I think I did. 

Yep!  I think I just read my email out of order.  :)

[Adam Barth]

Comment on attachment 97164 [details]
Quick self-review

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

> Source/WebCore/bindings/v8/V8Binding.cpp:507
> +            data->lastStringImpl() = stringImpl;
> +            data->lastV8String() = handle;

Can we use setters for this instead?  This code looks strange.

[Dmitry Lomov]

Created attachment 97193 [details]
CR feedback addressed

[Dmitry Lomov]

Created attachment 97195 [details]
CR feedback addressed

[anton muhin]

May the whole logic of string caching be moved into V8BindingPerIsolateData?

[Dmitry Lomov]

(In reply to comment #9)
> May the whole logic of string caching be moved into V8BindingPerIsolateData?

What are the benefits of doing so?

From the code organization standpoint, there are other caches in V8 bindings (DOMStore, NPObjects, per-binding function templates) that will need similar data in V8BindingPerIsolateData. I do not think should all move into V8BindingPerIsolateData.

What we could do though, is to have a separate class encapsulating string caching logic (it would combine StringCache, lastV8String and lastStringImpl; V8BindingPerIsolate data would contain an instance of this class); and do the similar thing for the other caches (a class for per-binding function template encapsulating rawTemplateMap and templateMap &c). 

But I'd prefer this refactoring to be a separate patch.

[anton muhin]

(In reply to comment #10)
> (In reply to comment #9)
> > May the whole logic of string caching be moved into V8BindingPerIsolateData?
> 
> What are the benefits of doing so?
> 
> From the code organization standpoint, there are other caches in V8 bindings (DOMStore, NPObjects, per-binding function templates) that will need similar data in V8BindingPerIsolateData. I do not think should all move into V8BindingPerIsolateData.
> 
> What we could do though, is to have a separate class encapsulating string caching logic (it would combine StringCache, lastV8String and lastStringImpl; V8BindingPerIsolate data would contain an instance of this class); and do the similar thing for the other caches (a class for per-binding function template encapsulating rawTemplateMap and templateMap &c). 
> 
> But I'd prefer this refactoring to be a separate patch.

The reason is simple: there are many parts which never were supposed to be public (roughly, nothing supposed to be public).  Now everything is.  And sooner or later someone will try to abuse this stuff.

[Dmitry Lomov]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > May the whole logic of string caching be moved into V8BindingPerIsolateData?
> > 
> > What are the benefits of doing so?
> > 
> > From the code organization standpoint, there are other caches in V8 bindings (DOMStore, NPObjects, per-binding function templates) that will need similar data in V8BindingPerIsolateData. I do not think should all move into V8BindingPerIsolateData.
> > 
> > What we could do though, is to have a separate class encapsulating string caching logic (it would combine StringCache, lastV8String and lastStringImpl; V8BindingPerIsolate data would contain an instance of this class); and do the similar thing for the other caches (a class for per-binding function template encapsulating rawTemplateMap and templateMap &c). 
> > 
> > But I'd prefer this refactoring to be a separate patch.
> 
> The reason is simple: there are many parts which never were supposed to be public (roughly, nothing supposed to be public).  Now everything is.  And sooner or later someone will try to abuse this stuff.

Right, but this line has been blurred already even before my patch (lastStringImpl and lastV8String were publicly visible). 

I am afraid we will have to hoist a bunch of static locals up to V8BindingPerIsolateData anyway. 

Let me extract a separate class out of StringCache, lastV8String and lastStringImpl though - I think that might address your concern.

[Dmitry Lomov]

Created attachment 97351 [details]
Moved StringImpl->v8 String cache into a separate class

This addresses "private implementation now public" concerns

[anton muhin]

Comment on attachment 97351 [details]
Moved StringImpl->v8 String cache into a separate class

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

LGTM

You should probably remove enableStringImplCache and related stuff now, when cache is accessible from any thread.

> Source/WebCore/bindings/v8/V8Binding.h:57
> +        inline v8::Local<v8::String> v8ExternalString(StringImpl* stringImpl) 

I don't think you need inline here.

[anton muhin]

Overall, I still have concerns about approach taken.

Moving all this stuff makes things more complex.  And most probably slower---even if each separate patch makes as slower by ~1% which is indistinguishable from noise, we may die of death of million cuts.

I would really prefer if there was a separate branch with the full implementation and if overall complexity and perf is good enough, we'll start to bring the changes to WebCore.

I would also appreciate more thorough discussion of different approaches, their benefits and drawbacks.  For example, Vitaly (p.c.) noted that we probably do not need to make all FunctionTenplate into per-isolate data as the set of ones used in Workers vs. ones in DOM pages has minimal (if any) intersection.

[Dmitry Lomov]

(In reply to comment #15)
> Overall, I still have concerns about approach taken.
>   
> And most probably slower---even if each separate patch makes as slower by ~1% which is indistinguishable from noise, we may die of death of million cuts.

I am cautiously optimistic on perf here. Since all we introduce here are TLS lookups for isolates, if we see a lot of those on hot paths, we hopefully be able to fetch those from v8::Object and other V8 entities, as I suggested in the other e-mail. The current perf results do not warrant that though - and this patch is Strings - by far our hottest.

> 
> I would really prefer if there was a separate branch with the full implementation and if overall complexity and perf is good enough, we'll start to bring the changes to WebCore.

I have been thinking about this, and my feeling is that the changes I make here or in other patches are not complex enough to warrant another branch - what do people think? 

I have made a preliminary "spear-headed" patch for isolates as well:
https://bug-61016-attachments.webkit.org/attachment.cgi?id=95694
(I CC'ed you on 61016 as well). It does not hoist much into V8BindingPerIsolateData, but it disables StringCache, which has bad perf implications.

> 
> I would also appreciate more thorough discussion of different approaches, their benefits and drawbacks.  For example, Vitaly (p.c.) noted that we probably do not need to make all FunctionTenplate into per-isolate data as the set of ones used in Workers vs. ones in DOM pages has minimal (if any) intersection.

So what is the concern with FunctionTemplates?
If it is about perf, I didn't observe any change. FunctionTemplates are lazily created, so only the those that are actually used in the isolate will be created.
If it is about the design, I think what has been done is the most simple and straightforward thing - I actually do not see any viable alternatives - but maybe I am missing something?

Regarding StringCache, I actually do not see any viable solution other that what has been done here, or disabling it entirely. What do you think?

> Moving all this stuff makes things more complex.

Well, "complexity" is hard to define precisely.

I think we are on a very straightforward trajectory here for supporting multi-threaded access to multiple instances of V8 in WebCore.  V8BindingPerIsolateData is a natural extension of V8::Isolate for binding-specific data. Hoisting things out of statics is a very natural thing we have to do to support multi-threading. 

If we look from the point of view of multithreading, static data and globals scattered all over the code _is_ complexity, and hoisting it all up in a single state object is _reducing_ complexity.

[Dmitry Lomov]

Some extra perf testing results: the patch makes no difference in Dromaeo/Dom performance on Linux 
http://dromaeo.com/?id=142530,142531

(Linux is the only platform with unoptimized TLS access in v8::Isolate::GetCurrent())

[Dmitry Lomov]

Created attachment 97657 [details]
CR feedback

[WebKit Review Bot]

Comment on attachment 97657 [details]
CR feedback

Attachment 97657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8877743

[Dmitry Lomov]

Created attachment 97660 [details]
Build fixed

[WebKit Review Bot]

Comment on attachment 97660 [details]
Build fixed

Clearing flags on attachment: 97660

Committed r89185: <http://trac.webkit.org/changeset/89185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dmitry Lomov]

Reopening bug: patch caused breakage in Web Inspector (https://bugs.webkit.org/show_bug.cgi?id=62977)

Will combine the patches and upload for review.

http://codereview.chromium.org/7215005/ tracks adding chromium unit tests for 62977.

[Dmitry Lomov]

Created attachment 98033 [details]
New patch, with eager initialization of V8BindingPerIsolateData in WebKit::initialize

[Dmitry Lomov]

Comment on attachment 98033 [details]
New patch, with eager initialization of V8BindingPerIsolateData in WebKit::initialize

No cq - waiting for trybots

[WebKit Review Bot]

Attachment 98033 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/WebKit.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Lomov]

Created attachment 98043 [details]
Style nit fixed

[Dmitry Lomov]

Created attachment 98047 [details]
Fixing a trybot issue

[Pavel Feldman]

Is the navigation issue fixed now?

[Dmitry Lomov]

(In reply to comment #29)
> Is the navigation issue fixed now?

Yes (see changes in WebKit::initialize) - I validated.

[Pavel Feldman]

(In reply to comment #30)
> (In reply to comment #29)
> > Is the navigation issue fixed now?
> 
> Yes (see changes in WebKit::initialize) - I validated.

Ok. The reason I ask is that I briefly applied attachment 98033 [details] to my release build earlier today and it was not fixing the navigation issue. It was not failing for the about:blank case though. My scenario was:

1. Navigate to google.com
2. Open DevTools
3. Navigate to webkit.org

Expected: things work fine
Actual: First time you hit Enter to navigate, navigation does not occur. Second time you hit navigate, devtools collapses.

Please double check this scenario prior to landing. I do realize that it is a cross-renderer navigation and that it goes through the about:blank state, so your fix should apply. But it did not do the job for me.

[Dmitry Lomov]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > Is the navigation issue fixed now?
> > 
> > Yes (see changes in WebKit::initialize) - I validated.
> 
> Ok. The reason I ask is that I briefly applied attachment 98033 [details] to my release build earlier today and it was not fixing the navigation issue. It was not failing for the about:blank case though. My scenario was:
> 
> 1. Navigate to google.com
> 2. Open DevTools
> 3. Navigate to webkit.org
> 
> Expected: things work fine
> Actual: First time you hit Enter to navigate, navigation does not occur. Second time you hit navigate, devtools collapses.
> 
> Please double check this scenario prior to landing. I do realize that it is a cross-renderer navigation and that it goes through the about:blank state, so your fix should apply. But it did not do the job for me.

Just validated this scenario again - all looks good.

[Pavel Feldman]

> Just validated this scenario again - all looks good.

Great, thanks! Leaving it to Vitaly / Yury / Adam / Rest for formal review.

[Adam Barth]

Comment on attachment 98047 [details]
Fixing a trybot issue

Then again, I liked this patch the first time too.  :)

[Dmitry Lomov]

Comment on attachment 98047 [details]
Fixing a trybot issue

 chromium trybot is happy

[WebKit Review Bot]

Comment on attachment 98047 [details]
Fixing a trybot issue

Clearing flags on attachment: 98047

Committed r89390: <http://trac.webkit.org/changeset/89390>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.