Issue metadata
Sign in to add a comment
|
WebGLRenderingContextBase::preserveObjectWrapper is slow |
||||||||||||||||||||||
Issue descriptionVersion: r390575 OS: Android WebGLRenderingContextBase::preserveObjectWrapper adds to over half of the time spent on the Animometer WebGL benchmark. Repro instructions: 1) Visit https://pr.gg/animometer/developer.html 2) Select from Suites: 3D Graphics > WebGL 3) Run benchmark. The attached profile is taken with adb_profile_chrome on Nexus 4. To view the profile in the attached trace, search for the track labelled "CrBrowserMain: cycles:HG" and select all samples in the track.
,
May 3 2016
,
May 3 2016
On Nexus 6P with ::preserveObjectWrapper removed the benchmark score improves from ~458 to ~1261.
,
May 3 2016
,
May 4 2016
preserveObjectWrapper seems to be building strings and calling into v8 to set a property, we should really avoid doing all this manual string interning and building work all the time.
,
May 10 2016
I've extracted the WebGL portion of the Animometer benchmark and am reimplementing preserveObjectWrapper to avoid string concatenation and dynamic allocation of V8 strings. Initial indication is that it's possible to basically eliminate the cost of preserveObjectWrapper.
,
May 10 2016
,
May 11 2016
,
May 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c293972b740368327c4c6e7f16fc749c451a2998 commit c293972b740368327c4c6e7f16fc749c451a2998 Author: kbr <kbr@chromium.org> Date: Thu May 12 00:19:05 2016 Added reduced WebGL case from Animometer to tough_webgl_cases. Updated links to a few of the earlier test cases. BUG= 608576 Review-Url: https://codereview.chromium.org/1967213002 Cr-Commit-Position: refs/heads/master@{#393115} [modify] https://crrev.com/c293972b740368327c4c6e7f16fc749c451a2998/tools/perf/page_sets/data/tough_webgl_cases.json [add] https://crrev.com/c293972b740368327c4c6e7f16fc749c451a2998/tools/perf/page_sets/data/tough_webgl_cases_004.wpr.sha1 [modify] https://crrev.com/c293972b740368327c4c6e7f16fc749c451a2998/tools/perf/page_sets/tough_webgl_cases.py
,
May 13 2016
,
May 14 2016
Kentaro, Sigbjorn: in the latest fix for this: https://codereview.chromium.org/1974713003/ an assertion failure is seen running several of the WebGL conformance tests: ASSERTION FAILED: address[i] == reuseAllowedZapValue || address[i] == reuseForbiddenZapValue Thread 0 (crashed) 0 chrome!blink::FreeList::checkFreedMemoryIsZapped(unsigned char*, unsigned long) + 0x59 1 chrome!sweep [HeapPage.cpp : 1115 + 0xb] 2 chrome!sweepUnsweptPage [HeapPage.cpp : 299 + 0x9] 3 chrome!lazySweepWithDeadline [HeapPage.cpp : 319 + 0x8] 4 chrome!performIdleLazySweep [ThreadState.cpp : 771 + 0x12] 5 chrome!RunWebThreadIdleTask [webthread_base.cc : 80 + 0x6] 6 chrome!Run [bind_internal.h : 159 + 0x3] 7 chrome!RunTask [callback.h : 397 + 0x4] 8 chrome!Run [bind_internal.h : 186 + 0x9] 9 chrome!RunTask [callback.h : 397 + 0x4] 10 chrome!ProcessTaskFromWorkQueue [task_queue_manager.cc : 289 + 0x1b] 11 chrome!DoWork [task_queue_manager.cc : 201 + 0xb] What's the implication of this assertion failure? That an object ended up on Oilpan's free list without going through the appropriate zapping phase? Do you have any ideas where the error in my patch set might be?
,
May 14 2016
The problem's definitely related to the use of the v8::Persistent instances in WebGLRenderingContextBase, WebGLFramebuffer, etc. in https://codereview.chromium.org/1974713003/ . Swapping out the implementation of WebGLRenderingContextBase::preserveObjectWrapper with the slower but simpler version in Issue 611864 , which does not populate those v8::Persistent handles, avoids the assertion failures.
,
May 14 2016
The problem was that v8::Persistent instances don't reset in their destructor by default, so embedding them in Oilpan objects will write all over the Oilpan heap during the next V8 GC after the Oilpan object has been reclaimed (this assumes the Oilpan object is GarbageCollectedFinalized). This has been worked around using an alias template to ensure correct usage, but ideally, the persistent caches would not be necessary -- per Issue 611864 .
,
May 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a21ed3eab013d0240f38bea0c55b9acc10dc864 commit 9a21ed3eab013d0240f38bea0c55b9acc10dc864 Author: kbr <kbr@chromium.org> Date: Sat May 14 20:25:52 2016 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 Review-Url: https://codereview.chromium.org/1974713003 Cr-Commit-Position: refs/heads/master@{#393737} [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.h [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLObject.h [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLProgram.h [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp [modify] https://crrev.com/9a21ed3eab013d0240f38bea0c55b9acc10dc864/third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
,
May 16 2016
The speedup is reflected on the Perf waterfall at this point. See the drop around r343749: https://chromeperf.appspot.com/report?sid=1c4e684a906366fd16022caa6f42fbf224b14b849a42c0714ff9450058502b5c
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de87dfd8ea21826750f8c52a39f85568045e3995 commit de87dfd8ea21826750f8c52a39f85568045e3995 Author: kbr <kbr@chromium.org> Date: Tue May 17 01:44:12 2016 Doubled complexity of webgl-animometer benchmark. The benchmark is now running at 60 FPS on some platforms. Double the complexity of the benchmark to 20000 triangles to try to expose more performance issues. BUG= 608576 Review-Url: https://codereview.chromium.org/1984843002 Cr-Commit-Position: refs/heads/master@{#394011} [modify] https://crrev.com/de87dfd8ea21826750f8c52a39f85568045e3995/tools/perf/page_sets/data/tough_webgl_cases.json [add] https://crrev.com/de87dfd8ea21826750f8c52a39f85568045e3995/tools/perf/page_sets/data/tough_webgl_cases_005.wpr.sha1
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3678e4df9b5256858d965ae8529dc7fb6b14647a commit 3678e4df9b5256858d965ae8529dc7fb6b14647a Author: kbr <kbr@chromium.org> Date: Tue May 17 02:29:50 2016 Replace uses of v8::Persistent with ScopedPersistent. Copyable persistents have been mostly eliminated from the Blink code base, replaced with ScopedPersistent. Re-tested locally. BUG= 608576 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/1983953002 Cr-Commit-Position: refs/heads/master@{#394024} [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/bindings/core/v8/ScopedPersistent.h [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.cpp [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLFramebuffer.h [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLObject.h [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLProgram.h [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp [modify] https://crrev.com/3678e4df9b5256858d965ae8529dc7fb6b14647a/third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
,
May 17 2016
The benchmark is in place and the performance improvement from the above optimization is clearly visible around r393749: https://chromeperf.appspot.com/report?sid=1c4e684a906366fd16022caa6f42fbf224b14b849a42c0714ff9450058502b5c The extra load from the doubling of the benchmark isn't visible on the graphs yet but we'll suppress any alerts that come from it. Closing as fixed.
,
May 24 2016
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by vmi...@chromium.org
, May 3 2016