Bug 62653

Summary: [V8][Chromium] Make StringCache in V8 bindings per-isolate
Product: WebKit Reporter: Dmitry Lomov <dslomov>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, antonm, dglazkov, dimich, dslomov, levin, pfeldman, vitalyr, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 63068    
Bug Blocks: 61016    
Attachments:
Description Flags
The fix moves StringCache to V8BindingPerIsolateData
none
Quick self-review
none
CR feedback addressed
none
CR feedback addressed
none
Moved StringImpl->v8 String cache into a separate class
none
CR feedback
webkit.review.bot: commit-queue-
Build fixed
none
New patch, with eager initialization of V8BindingPerIsolateData in WebKit::initialize
none
Style nit fixed
none
Fixing a trybot issue none

Description Dmitry Lomov 2011-06-14 11:14:23 PDT
This is another prerequisite to moving dedicated webworkers in-process.
Comment 1 Adam Barth 2011-06-14 12:03:11 PDT
Did you answer antonm's questions from the last patch?  It's important to make sure we're not regressing benchmarks with this work.
Comment 2 Dmitry Lomov 2011-06-14 13:38:29 PDT
(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.
Comment 3 Dmitry Lomov 2011-06-14 13:54:02 PDT
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.
Comment 4 Dmitry Lomov 2011-06-14 14:00:51 PDT
Created attachment 97164 [details]
Quick self-review
Comment 5 Adam Barth 2011-06-14 14:03:28 PDT
> > 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.  :)
Comment 6 Adam Barth 2011-06-14 14:04:48 PDT
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.
Comment 7 Dmitry Lomov 2011-06-14 16:49:05 PDT
Created attachment 97193 [details]
CR feedback addressed
Comment 8 Dmitry Lomov 2011-06-14 16:51:18 PDT
Created attachment 97195 [details]
CR feedback addressed
Comment 9 anton muhin 2011-06-15 07:51:55 PDT
May the whole logic of string caching be moved into V8BindingPerIsolateData?
Comment 10 Dmitry Lomov 2011-06-15 09:48:52 PDT
(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.
Comment 11 anton muhin 2011-06-15 09:54:55 PDT
(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.
Comment 12 Dmitry Lomov 2011-06-15 10:08:53 PDT
(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.
Comment 13 Dmitry Lomov 2011-06-15 13:10:21 PDT
Created attachment 97351 [details]
Moved StringImpl->v8 String cache into a separate class

This addresses "private implementation now public" concerns
Comment 14 anton muhin 2011-06-16 05:52:19 PDT
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.
Comment 15 anton muhin 2011-06-16 05:56:51 PDT
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.
Comment 16 Dmitry Lomov 2011-06-16 08:06:01 PDT
(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.
Comment 17 Dmitry Lomov 2011-06-17 14:14:20 PDT
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())
Comment 18 Dmitry Lomov 2011-06-17 15:18:11 PDT
Created attachment 97657 [details]
CR feedback
Comment 19 WebKit Review Bot 2011-06-17 15:31:04 PDT
Comment on attachment 97657 [details]
CR feedback

Attachment 97657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8877743
Comment 20 Dmitry Lomov 2011-06-17 15:37:56 PDT
Created attachment 97660 [details]
Build fixed
Comment 21 WebKit Review Bot 2011-06-17 21:42:08 PDT
Comment on attachment 97660 [details]
Build fixed

Clearing flags on attachment: 97660

Committed r89185: <http://trac.webkit.org/changeset/89185>
Comment 22 WebKit Review Bot 2011-06-17 21:42:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Dmitry Lomov 2011-06-21 09:09:06 PDT
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.
Comment 24 Dmitry Lomov 2011-06-21 11:56:17 PDT
Created attachment 98033 [details]
New patch, with eager initialization of V8BindingPerIsolateData in WebKit::initialize
Comment 25 Dmitry Lomov 2011-06-21 11:56:51 PDT
Comment on attachment 98033 [details]
New patch, with eager initialization of V8BindingPerIsolateData in WebKit::initialize

No cq - waiting for trybots
Comment 26 WebKit Review Bot 2011-06-21 11:58:34 PDT
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.
Comment 27 Dmitry Lomov 2011-06-21 12:42:36 PDT
Created attachment 98043 [details]
Style nit fixed
Comment 28 Dmitry Lomov 2011-06-21 13:03:08 PDT
Created attachment 98047 [details]
Fixing a trybot issue
Comment 29 Pavel Feldman 2011-06-21 13:11:18 PDT
Is the navigation issue fixed now?
Comment 30 Dmitry Lomov 2011-06-21 13:21:19 PDT
(In reply to comment #29)
> Is the navigation issue fixed now?

Yes (see changes in WebKit::initialize) - I validated.
Comment 31 Pavel Feldman 2011-06-21 13:46:17 PDT
(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.
Comment 32 Dmitry Lomov 2011-06-21 13:56:24 PDT
(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.
Comment 33 Pavel Feldman 2011-06-21 14:05:25 PDT
> Just validated this scenario again - all looks good.

Great, thanks! Leaving it to Vitaly / Yury / Adam / Rest for formal review.
Comment 34 Adam Barth 2011-06-21 14:11:10 PDT
Comment on attachment 98047 [details]
Fixing a trybot issue

Then again, I liked this patch the first time too.  :)
Comment 35 Dmitry Lomov 2011-06-21 15:09:49 PDT
Comment on attachment 98047 [details]
Fixing a trybot issue

 chromium trybot is happy
Comment 36 WebKit Review Bot 2011-06-21 16:09:27 PDT
Comment on attachment 98047 [details]
Fixing a trybot issue

Clearing flags on attachment: 98047

Committed r89390: <http://trac.webkit.org/changeset/89390>
Comment 37 WebKit Review Bot 2011-06-21 16:09:33 PDT
All reviewed patches have been landed.  Closing bug.