Bug 62653

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

(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.

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.

Created attachment 97164 [details] Quick self-review

> > 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 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.

Created attachment 97193 [details] CR feedback addressed

Created attachment 97195 [details] CR feedback addressed

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

(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.

(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.

(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.

Created attachment 97351 [details] Moved StringImpl->v8 String cache into a separate class This addresses "private implementation now public" concerns

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.

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.

(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.

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())

Created attachment 97657 [details] CR feedback

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

Created attachment 97660 [details] Build fixed

Comment on attachment 97660 [details] Build fixed Clearing flags on attachment: 97660 Committed r89185: <http://trac.webkit.org/changeset/89185>

All reviewed patches have been landed. Closing bug.

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.

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

Comment on attachment 98033 [details] New patch, with eager initialization of V8BindingPerIsolateData in WebKit::initialize No cq - waiting for trybots

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.

Created attachment 98043 [details] Style nit fixed

Created attachment 98047 [details] Fixing a trybot issue

Is the navigation issue fixed now?

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

(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.

(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.

> Just validated this scenario again - all looks good. Great, thanks! Leaving it to Vitaly / Yury / Adam / Rest for formal review.

Comment on attachment 98047 [details] Fixing a trybot issue Then again, I liked this patch the first time too. :)

Comment on attachment 98047 [details] Fixing a trybot issue chromium trybot is happy

Comment on attachment 98047 [details] Fixing a trybot issue Clearing flags on attachment: 98047 Committed r89390: <http://trac.webkit.org/changeset/89390>

All reviewed patches have been landed. Closing bug.