Fix content scripts that use WebGL.

Fix content scripts that use WebGL.

The code which adds hidden links between WebGL objects' JavaScript
wrappers has basically been broken since it was introduced, and through
the recent performance rewrite.

Use toV8 rather than ScriptWrappable::mainWorldWrapper to get wrappers
in the correct isolated world. Switch to using V8PrivateProperty rather
than V8HiddenValue. Continue to use persistent caches on the main world,
which was found necessary for performance reasons.

BUG=581997
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

[Ken Russell (switch to Gerrit), 2016-05-28 01:32:42]

Description was changed from

==========
Fix content scripts that use WebGL.

The code which adds hidden links between WebGL objects' JavaScript
wrappers has basically been broken since it was introduced, and through
the recent performance rewrite.

Use toV8 rather than ScriptWrappable::newLocalWrapper to get wrappers in
the correct isolated world. Switch to using V8PrivateProperty rather
than V8HiddenValue for performance reasons. Eliminate persistent caches,
which won't handle multiple isolated worlds correctly.

BUG=581997
==========

to

==========
Fix content scripts that use WebGL.

The code which adds hidden links between WebGL objects' JavaScript
wrappers has basically been broken since it was introduced, and through
the recent performance rewrite.

Use toV8 rather than ScriptWrappable::newLocalWrapper to get wrappers in
the correct isolated world. Switch to using V8PrivateProperty rather
than V8HiddenValue for performance reasons. Eliminate persistent caches,
which won't handle multiple isolated worlds correctly.

BUG=581997
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-05-28 01:35:33]

kbr@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-28 01:35:34]

haraken, yukishiino: please review the small change to
V8PrivateProperty::Symbol, as well as anything else you'd like to.

zmo: please review everything.

This depends on https://codereview.chromium.org/2007463003 . I also haven't
benchmarked it yet, but verified it fixes the bug and also passes the
expando-loss WebGL conformance test. Will benchmark before submitting.

[haraken, 2016-05-28 07:56:55]

LGTM assuming that performance looks good.

[Yuki, 2016-05-30 06:53:44]

https://codereview.chromium.org/2023603003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2023603003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6326:
v8::Local<v8::Object> sourceWrapper = v8::Local<v8::Object>::Cast(toV8(
You can use a shorthand.
    toV8(...).As<v8::Object>()

https://codereview.chromium.org/2023603003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6328:
v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(
Ditto.

https://codereview.chromium.org/2023603003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6335:
v8CallOrCrash(array->Set(context, index, toV8(targetObject, context->Global(),
isolate)));
You can use |sourceWrapper| instead of |context->Global()|.

[Yuki, 2016-05-30 06:54:04]

lgtm

[Zhenyao Mo, 2016-05-31 02:06:12]

On 2016/05/30 06:54:04, Yuki wrote:
> lgtm

lgtm

[Ken Russell (switch to Gerrit), 2016-06-01 22:28:35]

yukishiino@: please take another look at this CL.

After rebasing on top of your https://codereview.chromium.org/2007463003 , this
CL unfortunately has a negative performance impact on the WebGL portion of the
Animometer benchmark. Compare runs of this benchmark with and without this CL:

./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases >
output.txt

and scan through the log for "mean_frame_time" for
"http___kenrussell.github.io_webgl-animometer_...".

On my Linux workstation, current ToT has a mean frame time of 56.217 ms. With
this CL, it jumps to 83.02 ms.

V8PrivateProperty still isn't as fast as it needs to be. Could you please
profile the benchmark above with and without this CL and see where it needs to
be improved? I don't think this CL should land until it's been sped up further.

[Yuki, 2016-06-02 13:07:23]

I successfully repro the regression on my local environment.

There is almost no room to improve the performance of V8PrivateProperty.  I have
only one thing in my mind, and tried it, but didn't observe a major improvement.
 The one thing is to remove the call to obj->HasPrivate() check.
See https://codereview.chromium.org/2034573004/ fyi.

I think that we cannot avoid a performance regression because
a) V8's private property is slower than V8's persistent handle.
b) The code that supports multiple worlds is slower than the code that only
supports the main world.

As long as you need to save a wrapper object in another wrapper object as a
private property, we cannot avoid a regression, I think.

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:328:
preserveObjectWrapper(scriptState, framebufferBinding,
V8PrivateProperty::getWebGLFramebufferAttachments(scriptState->isolate()),
attachment, texture);
Could you tell us why you need to preserve the wrapper of |texture|?

Preserved wrapper objects seem never used.  If you only need to keep |texture|
alive, is it enough to keep Member<WebGLTexture> and let Oilpan know the
dependency through DEFINE_TRACE()?  Or do you need the wrapper object of
|texture| alive in addition to |texture| itself?

By the way, the following member variables seem not used.
    ScopedPersistent<v8::Array> m_samplerWrappers;
    ScopedPersistent<v8::Array> m_queryWrappers;

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6335:
v8CallOrCrash(array->Set(context, index, toV8(targetObject, context->Global(),
isolate)));
This is another reason why this CL is slower. 
|targetObject->mainWorldWrapper()| is faster than |toV8(targetObject)|.

[haraken, 2016-06-02 13:12:11]

I don't really like this idea but what you could do is to introduce two
functions: one super fast function for the main world case (using persistent
handles) and the other function for the isolated world case (using
V8PrivateProperty). You can switch the functions depending on
if(scriptState->world()->isMainWorld()).

[Ken Russell (switch to Gerrit), 2016-06-03 00:54:27]

Description was changed from

==========
Fix content scripts that use WebGL.

The code which adds hidden links between WebGL objects' JavaScript
wrappers has basically been broken since it was introduced, and through
the recent performance rewrite.

Use toV8 rather than ScriptWrappable::newLocalWrapper to get wrappers in
the correct isolated world. Switch to using V8PrivateProperty rather
than V8HiddenValue for performance reasons. Eliminate persistent caches,
which won't handle multiple isolated worlds correctly.

BUG=581997
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Fix content scripts that use WebGL.

The code which adds hidden links between WebGL objects' JavaScript
wrappers has basically been broken since it was introduced, and through
the recent performance rewrite.

Use toV8 rather than ScriptWrappable::mainWorldWrapper to get wrappers
in the correct isolated world. Switch to using V8PrivateProperty rather
than V8HiddenValue. Continue to use persistent caches on the main world,
which was found necessary for performance reasons.

BUG=581997
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-06-03 00:59:14]

On 2016/06/02 13:12:11, haraken wrote:
> I don't really like this idea but what you could do is to introduce two
> functions: one super fast function for the main world case (using persistent
> handles) and the other function for the isolated world case (using
> V8PrivateProperty). You can switch the functions depending on
> if(scriptState->world()->isMainWorld()).

Thanks for this suggestion. Unfortunately it looks like this is necessary for
performance reasons. I'll submit another patch set implementing this.

[Ken Russell (switch to Gerrit), 2016-06-03 00:59:19]

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:328:
preserveObjectWrapper(scriptState, framebufferBinding,
V8PrivateProperty::getWebGLFramebufferAttachments(scriptState->isolate()),
attachment, texture);
On 2016/06/02 13:07:22, Yuki wrote:
> Could you tell us why you need to preserve the wrapper of |texture|?
> 
> Preserved wrapper objects seem never used.  If you only need to keep |texture|
> alive, is it enough to keep Member<WebGLTexture> and let Oilpan know the
> dependency through DEFINE_TRACE()?  Or do you need the wrapper object of
> |texture| alive in addition to |texture| itself?

Yes, the wrapper objects need to be kept alive, per the WebGL specification and
the associated conformance tests.

> By the way, the following member variables seem not used.
>     ScopedPersistent<v8::Array> m_samplerWrappers;
>     ScopedPersistent<v8::Array> m_queryWrappers;

Thanks for pointing that out; I missed them. The persistent caches had to be
reintroduced for performance reasons, so these are used again.

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2023603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6335:
v8CallOrCrash(array->Set(context, index, toV8(targetObject, context->Global(),
isolate)));
On 2016/06/02 13:07:22, Yuki wrote:
> This is another reason why this CL is slower. 
> |targetObject->mainWorldWrapper()| is faster than |toV8(targetObject)|.

Thanks for pointing this out. I tried adding two code paths in this function,
one using mainWorldWrapper and one using toV8. Unfortunately there's still a
performance hit for eliminating the persistent caches. On my Linux workstation
running the WebGL Animometer benchmark it's the difference between 56 ms/frame
and 66 ms/frame. For this reason I reintroduced the persistent caches, but only
use them in the main world.

[Ken Russell (switch to Gerrit), 2016-06-03 01:02:16]

Please re-review. This patch set eliminates the performance cost of this fix.

[Yuki, 2016-06-03 06:50:59]

We're very sorry for having you take a long time on this issue, but Haraken and
I had a discussion about if it's possible or not to get rid of the hack.

Let us confirm what you need.
a) You need to keep wrapper objects that are passed to WebGL APIs as function
arguments.
b) WebGL implementation itself does NOT directly use those wrapper objects. 
You're using C++ objects instead of wrapper objects.

If a) and b) are true, i.e. what you need is only to keep wrapper objects alive
and no need to fast access to those wrapper objects, we think you can use
VisitDOMWrapper and get rid of direct use of persistent handles and related
hacks.

See the following code about how to use VisitDOMWrapper.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Our idea is that
1) You keep the pointers to C++ objects instead of keeping wrapper objects in
persistent handles or private properties.
2) You define a custom visitDOMWrapper function.
3) In the visitDOMWrapper function, you tell GC the references from the WebGL
object to wrapper objects for the C++ objects kept in the WebGL object.

In this way, 3) is done only when GC starts the marking.  It should be not as
frequest as API calls in the benchmark.  In the API calls, you just need to copy
the C++ pointers of the arguments.  It must be very cheap.

Does this make sense?  Does this work for you?

[haraken, 2016-08-12 00:58:53]

On 2016/08/11 20:16:15, Ken Russell wrote:

What does the empty message imply? :)

[Ken Russell (switch to Gerrit), 2016-08-15 20:55:25]

On 2016/08/12 00:58:53, haraken wrote:
> On 2016/08/11 20:16:15, Ken Russell wrote:
> 
> What does the empty message imply? :)

Sorry, was adding kainino@ to the CC: list. I expect he'll take over this CL
from me in the next couple of weeks.

We started studying existing uses of VisitDOMWrapper in the Blink sources. I
have to admit I don't fully understand how this CL should be converted to use
it. I'll send a separate email on this topic.

[Ken Russell (switch to Gerrit), 2016-10-05 01:35:26]

On 2016/08/15 20:55:25, Ken Russell wrote:
> On 2016/08/12 00:58:53, haraken wrote:
> > On 2016/08/11 20:16:15, Ken Russell wrote:
> > 
> > What does the empty message imply? :)
> 
> Sorry, was adding kainino@ to the CC: list. I expect he'll take over this CL
> from me in the next couple of weeks.
> 
> We started studying existing uses of VisitDOMWrapper in the Blink sources. I
> have to admit I don't fully understand how this CL should be converted to use
> it. I'll send a separate email on this topic.

kainino@ implemented this in https://codereview.chromium.org/2023603003/ .
Closing this CL.

[Ken Russell (switch to Gerrit), 2017-03-03 23:27:39]

On 2016/10/05 01:35:26, Ken Russell wrote:
> On 2016/08/15 20:55:25, Ken Russell wrote:
> > On 2016/08/12 00:58:53, haraken wrote:
> > > On 2016/08/11 20:16:15, Ken Russell wrote:
> > > 
> > > What does the empty message imply? :)
> > 
> > Sorry, was adding kainino@ to the CC: list. I expect he'll take over this CL
> > from me in the next couple of weeks.
> > 
> > We started studying existing uses of VisitDOMWrapper in the Blink sources. I
> > have to admit I don't fully understand how this CL should be converted to
use
> > it. I'll send a separate email on this topic.
> 
> kainino@ implemented this in https://codereview.chromium.org/2023603003/ .
> Closing this CL.

Correction: kainino@ implemented this in
https://codereview.chromium.org/2273683003 .