Speed up fix for expando-loss conformance test.

Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (This call is expensive; see below.)

On a Linux desktop workstation testing a Release component build,
this speeds up the WebGL animometer benchmark(*) running with a
fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark runs at 23 ms/frame. Without
the persistent cache, calling V8HiddenValue::getHiddenValue each
time, the benchmark runs at 55 ms/frame.

(*) ./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

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

Committed: https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864
Cr-Commit-Position: refs/heads/master@{#393737}

[Ken Russell (switch to Gerrit), 2016-05-12 21:24:22]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive --
it resulted in 71 ms/frame in the times below.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 113 ms/frame to 48
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 39 ms/frame.

BUG=608576
==========

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive --
it resulted in 71 ms/frame in the times below.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 113 ms/frame to 48
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 39 ms/frame.

BUG=608576
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-12 21:26:16]

kbr@chromium.org changed reviewers:
+ zmo@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-12 21:26:17]

zmo: please review.

haraken, sigbjornf: FYI, in case you'd like to look over the use of
v8::Persistent and SetWeak. I'm pretty sure this is correct.

esprehn: also FYI.

[Zhenyao Mo, 2016-05-12 21:40:05]

On 2016/05/12 21:26:17, Ken Russell wrote:
> zmo: please review.
> 
> haraken, sigbjornf: FYI, in case you'd like to look over the use of
> v8::Persistent and SetWeak. I'm pretty sure this is correct.
> 
> esprehn: also FYI.

great! lgtm

[Ken Russell (switch to Gerrit), 2016-05-12 21:54:25]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive --
it resulted in 71 ms/frame in the times below.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 113 ms/frame to 48
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 39 ms/frame.

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

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive --
it resulted in 71 ms/frame in the times below.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 113 ms/frame to 48
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 39 ms/frame.

BUG=608576
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-12 21:54:25]

kbr@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-12 21:54:52]

On 2016/05/12 21:40:05, Zhenyao Mo wrote:
> On 2016/05/12 21:26:17, Ken Russell wrote:
> > zmo: please review.
> > 
> > haraken, sigbjornf: FYI, in case you'd like to look over the use of
> > v8::Persistent and SetWeak. I'm pretty sure this is correct.
> > 
> > esprehn: also FYI.
> 
> great! lgtm

Thanks Mo. Actually, will need an OWNERS review in bindings/ so esprehn@ or
haraken@ could you please review too?

[esprehn, 2016-05-12 22:10:00]

Can you benchmark without dcheck turned on? That makes many things very slow.

[haraken, 2016-05-13 01:32:41]

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6311:
v8::Maybe<bool> result = persistentCache->Get(isolate)->Set(

Avoid using Maybe APIs -- you can use macros in V8BindingMacros.h.

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1046:
v8::Persistent<v8::Array> m_extensionWrappers;

Would you help me understand why these persistent handles don't cause leaks?

[haraken, 2016-05-13 01:33:10]

On 2016/05/12 22:10:00, esprehn wrote:
> Can you benchmark without dcheck turned on? That makes many things very slow.

Just curious, is dcheck enabled on Release builds?

[esprehn, 2016-05-13 01:35:27]

On 2016/05/13 at 01:33:10, haraken wrote:
> On 2016/05/12 22:10:00, esprehn wrote:
> > Can you benchmark without dcheck turned on? That makes many things very
slow.
> 
> Just curious, is dcheck enabled on Release builds?

Only if you set dcheck_always_on=true. It makes many things like v8 quite a lot
slower though.

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

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6311:
v8::Maybe<bool> result = persistentCache->Get(isolate)->Set(
On 2016/05/13 01:32:41, haraken wrote:
> 
> Avoid using Maybe APIs -- you can use macros in V8BindingMacros.h.

Thanks, will revise.

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1046:
v8::Persistent<v8::Array> m_extensionWrappers;
On 2016/05/13 01:32:41, haraken wrote:
> 
> Would you help me understand why these persistent handles don't cause leaks?

I specifically made them weak to avoid possible leaks. See the call to SetWeak
in WebGLRenderingContextBase::preserveObjectWrapper.

[Ken Russell (switch to Gerrit), 2016-05-13 01:52:47]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive --
it resulted in 71 ms/frame in the times below.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 113 ms/frame to 48
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 39 ms/frame.

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

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 23 ms/frame.

BUG=608576
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-13 01:53:32]

On 2016/05/12 22:10:00, esprehn wrote:
> Can you benchmark without dcheck turned on? That makes many things very slow.

I've revised the numbers above with a Release build without dchecks turned on.
It seems clear there's still more speedup to be gained, but this is at least a
good first step.

[Ken Russell (switch to Gerrit), 2016-05-13 02:10:14]

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6311:
v8::Maybe<bool> result = persistentCache->Get(isolate)->Set(
On 2016/05/13 01:45:35, Ken Russell wrote:
> On 2016/05/13 01:32:41, haraken wrote:
> > 
> > Avoid using Maybe APIs -- you can use macros in V8BindingMacros.h.
> 
> Thanks, will revise.

Done.

[haraken, 2016-05-13 02:12:20]

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6287:
static void NoopWeakCallback(const v8::WeakCallbackInfo<int>& data)

You can use a phantom handle instead (see
https://codereview.chromium.org/1944363002/). Then V8 doesn't need to call back
C++ during weak processing, which will make it even faster.

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6302:
persistentCache->Get(isolate));

So you're adding a strong reference from the JS wrapper to the Array (to make
sure that the weak persistent is kept alive), right?

How about just directly storing the v8::Array object on the hidden value of the
JS wrapper? i.e., I don't understand why you need to keep track of the weak
persistent handle to the Array. You can always retrieve the Array from the
hidden value of the JS object.

[Ken Russell (switch to Gerrit), 2016-05-13 02:24:30]

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6287:
static void NoopWeakCallback(const v8::WeakCallbackInfo<int>& data)
On 2016/05/13 02:12:20, haraken wrote:
> 
> You can use a phantom handle instead (see
> https://codereview.chromium.org/1944363002/). Then V8 doesn't need to call
back
> C++ during weak processing, which will make it even faster.

Thanks for the suggestion; didn't know how to do that. Done.

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6302:
persistentCache->Get(isolate));
On 2016/05/13 02:12:20, haraken wrote:
> 
> So you're adding a strong reference from the JS wrapper to the Array (to make
> sure that the weak persistent is kept alive), right?

Correct.

> How about just directly storing the v8::Array object on the hidden value of
the
> JS wrapper? i.e., I don't understand why you need to keep track of the weak
> persistent handle to the Array. You can always retrieve the Array from the
> hidden value of the JS object.

I measured it and calling V8HiddenValue::getHiddenValue is expensive. Using this
persistent cache, so that the hidden v8::Array object can be fetched quickly,
delivers a big speedup.

[esprehn, 2016-05-13 03:00:40]

Your description still says dcheck was on?

[Ken Russell (switch to Gerrit), 2016-05-13 03:07:16]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive.)

On a Linux desktop workstation testing a Release build with
dcheck_always_on=1, this sped up the WebGL animometer benchmark
running with a fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark ran at 23 ms/frame.

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

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive.)

On a Linux desktop workstation testing a Release component build,
this sped up the WebGL animometer benchmark running with a fixed
load of 10000 triangles from 71 ms/frame to 37 ms/frame. For
comparison, with the preserveObjectWrapper calls completely
removed, the benchmark ran at 23 ms/frame.

BUG=608576
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-13 03:07:29]

On 2016/05/13 03:00:40, esprehn wrote:
> Your description still says dcheck was on?

Oops, sorry, that was wrong. Fixed.

[haraken, 2016-05-13 04:27:39]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org

[haraken, 2016-05-13 04:27:39]

+jochen: FYI, V8HiddenValue::getHiddenValue is slow so we need to manually cache
the contents of the hidden value...

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

https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6302:
persistentCache->Get(isolate));
On 2016/05/13 02:24:30, Ken Russell wrote:
> On 2016/05/13 02:12:20, haraken wrote:
> > 
> > So you're adding a strong reference from the JS wrapper to the Array (to
make
> > sure that the weak persistent is kept alive), right?
> 
> Correct.
> 
> > How about just directly storing the v8::Array object on the hidden value of
> the
> > JS wrapper? i.e., I don't understand why you need to keep track of the weak
> > persistent handle to the Array. You can always retrieve the Array from the
> > hidden value of the JS object.
> 
> I measured it and calling V8HiddenValue::getHiddenValue is expensive. Using
this
> persistent cache, so that the hidden v8::Array object can be fetched quickly,
> delivers a big speedup.

How slow was it?

It's nasty that you have to manually cache the contents of the hidden value
because V8HiddenValue::getHiddenValue is slow... I'm okay with landing this CL
with TODO if it's unacceptably slower, but it needs to be fixed.

[haraken, 2016-05-13 04:33:00]

LGTM

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

https://codereview.chromium.org/1974713003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6307:
V8_CALL(result, localCache, Set(scriptState->context(), index,
targetObject->newLocalWrapper(isolate)), return);

v8CallOrCrash

https://codereview.chromium.org/1974713003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6311:
V8_CALL(result, localCache, Set(scriptState->context(), index,
v8::Null(isolate)), return);

v8CallOrCrash

[Ken Russell (switch to Gerrit), 2016-05-13 21:22:50]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (In testing, this call was expensive.)

On a Linux desktop workstation testing a Release component build,
this sped up the WebGL animometer benchmark running with a fixed
load of 10000 triangles from 71 ms/frame to 37 ms/frame. For
comparison, with the preserveObjectWrapper calls completely
removed, the benchmark ran at 23 ms/frame.

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

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (This call is expensive; see below.)

On a Linux desktop workstation testing a Release component build,
this speeds up the WebGL animometer benchmark(*) running with a
fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark runs at 23 ms/frame. Without
the persistent cache, calling V8HiddenValue::getHiddenValue each
time, the benchmark runs at 55 ms/frame.

(*) ./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

BUG=608576
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-13 21:23:42]

On 2016/05/13 04:27:39, haraken wrote:
> +jochen: FYI, V8HiddenValue::getHiddenValue is slow so we need to manually
cache
> the contents of the hidden value...
> 
>
https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/1974713003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6302:
> persistentCache->Get(isolate));
> On 2016/05/13 02:24:30, Ken Russell wrote:
> > On 2016/05/13 02:12:20, haraken wrote:
> > > 
> > > So you're adding a strong reference from the JS wrapper to the Array (to
> make
> > > sure that the weak persistent is kept alive), right?
> > 
> > Correct.
> > 
> > > How about just directly storing the v8::Array object on the hidden value
of
> > the
> > > JS wrapper? i.e., I don't understand why you need to keep track of the
weak
> > > persistent handle to the Array. You can always retrieve the Array from the
> > > hidden value of the JS object.
> > 
> > I measured it and calling V8HiddenValue::getHiddenValue is expensive. Using
> this
> > persistent cache, so that the hidden v8::Array object can be fetched
quickly,
> > delivers a big speedup.
> 
> How slow was it?

I've updated the CL description with that number again, as well as instructions
for reproducing it.

> It's nasty that you have to manually cache the contents of the hidden value
> because V8HiddenValue::getHiddenValue is slow... I'm okay with landing this CL
> with TODO if it's unacceptably slower, but it needs to be fixed.

I agree that V8HiddenValue::getHiddenValue should be faster. I'll file a
follow-on bug and add a TODO.

[Ken Russell (switch to Gerrit), 2016-05-13 21:56:09]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-13 21:56:09]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1974713003/#ps60001
(title: "Use v8CallOrCrash. Add V8HiddenValue::getHiddenValue TODO.")

[commit-bot: I haz the power, 2016-05-13 21:56:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974713003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974713003/60001

[commit-bot: I haz the power, 2016-05-13 22:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-13 22:59:20]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Russell (switch to Gerrit), 2016-05-13 23:17:02]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-13 23:17:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1974713003/#ps80001
(title: "Fixed Windows compile errors.")

[commit-bot: I haz the power, 2016-05-13 23:17:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974713003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974713003/80001

[commit-bot: I haz the power, 2016-05-14 00:43:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-14 00:43:10]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

On 2016/05/14 00:43:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Asking Oilpan team for help with these assertion failures in
https://bugs.chromium.org/p/chromium/issues/detail?id=608576#c11 . Will attempt
to debug myself as well.

[Ken Russell (switch to Gerrit), 2016-05-14 02:51:21]

On 2016/05/14 01:24:26, Ken Russell wrote:
> On 2016/05/14 00:43:10, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Asking Oilpan team for help with these assertion failures in
> https://bugs.chromium.org/p/chromium/issues/detail?id=608576#c11 . Will
attempt
> to debug myself as well.

Figured this out; see
https://bugs.chromium.org/p/chromium/issues/detail?id=608576#c13 . The changes
are only in WebGL-related files, so I'm re-CQ'ing this. IT turns out the only
other user of v8::Persistent in the Blink code base is ScriptWrappable, so
ideally, all of the uses of v8::Persistent in the WebGL classes would be removed
(Issue 611864).

[Ken Russell (switch to Gerrit), 2016-05-14 02:51:28]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-14 02:51:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1974713003/#ps100001
(title: "Add V8CopyablePersistent alias to avoid incorrect usage.")

[commit-bot: I haz the power, 2016-05-14 02:51:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974713003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974713003/100001

[haraken, 2016-05-14 02:56:40]

If this CL is not urgent, I want to reconsider the approach on Monday.

We've tried hard to entirely remove copyable persistents from the code base
(which had been a cause of use-after-frees) but this CL reintroduces them.

[commit-bot: I haz the power, 2016-05-14 20:26:04]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (This call is expensive; see below.)

On a Linux desktop workstation testing a Release component build,
this speeds up the WebGL animometer benchmark(*) running with a
fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark runs at 23 ms/frame. Without
the persistent cache, calling V8HiddenValue::getHiddenValue each
time, the benchmark runs at 55 ms/frame.

(*) ./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

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

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (This call is expensive; see below.)

On a Linux desktop workstation testing a Release component build,
this speeds up the WebGL animometer benchmark(*) running with a
fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark runs at 23 ms/frame. Without
the persistent cache, calling V8HiddenValue::getHiddenValue each
time, the benchmark runs at 55 ms/frame.

(*) ./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

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

[commit-bot: I haz the power, 2016-05-14 20:26:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-14 20:27:29]

Description was changed from

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (This call is expensive; see below.)

On a Linux desktop workstation testing a Release component build,
this speeds up the WebGL animometer benchmark(*) running with a
fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark runs at 23 ms/frame. Without
the persistent cache, calling V8HiddenValue::getHiddenValue each
time, the benchmark runs at 55 ms/frame.

(*) ./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

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

to

==========
Speed up fix for expando-loss conformance test.

The previously-added preserveObjectWrapper was spending a lot of time in
string allocation and concatenation.

Reimplement it using multiple object arrays attached to the JavaScript
wrappers, and simple indexing into those arrays. Also, maintain a weak
persistent cache of these arrays in the containing objects (the context,
programs, framebufffers, etc.) to avoid lookups via
V8HiddenValue::getHiddenValue. (This call is expensive; see below.)

On a Linux desktop workstation testing a Release component build,
this speeds up the WebGL animometer benchmark(*) running with a
fixed load of 10000 triangles from 71 ms/frame to 37
ms/frame. For comparison, with the preserveObjectWrapper calls
completely removed, the benchmark runs at 23 ms/frame. Without
the persistent cache, calling V8HiddenValue::getHiddenValue each
time, the benchmark runs at 55 ms/frame.

(*) ./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

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

Committed: https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864
Cr-Commit-Position: refs/heads/master@{#393737}
==========

[commit-bot: I haz the power, 2016-05-14 20:27:30]

Patchset 6 (id:??) landed as
https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864
Cr-Commit-Position: refs/heads/master@{#393737}

[haraken, 2016-05-15 09:18:46]

https://codereview.chromium.org/1974713003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/1974713003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:69: using
V8CopyablePersistent = v8::Persistent<T, v8::CopyablePersistentTraits<T>>;

Instead you should use ScopedPersistent. Then ScopedPersistent's destructor
resets the underlying persistent handle, which will solve the problem.

[Ken Russell (switch to Gerrit), 2016-05-17 00:26:58]

On 2016/05/15 09:18:46, haraken wrote:
>
https://codereview.chromium.org/1974713003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):
> 
>
https://codereview.chromium.org/1974713003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webgl/WebGLObject.h:69: using
> V8CopyablePersistent = v8::Persistent<T, v8::CopyablePersistentTraits<T>>;
> 
> Instead you should use ScopedPersistent. Then ScopedPersistent's destructor
> resets the underlying persistent handle, which will solve the problem.

Thanks. Making this change in https://codereview.chromium.org/1983953002 .