Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962213002:
Make String16 consturctors non-inline to save binary size (150kb) (Closed)
DescriptionAdd js-perf-test to exercise v8_inspector::String16
BUG=chromium:738469
Review-Url: https://codereview.chromium.org/2962213002
Cr-Commit-Position: refs/heads/master@{#46612}
Committed: https://chromium.googlesource.com/v8/v8/+/19ffed65ba3adc0f4e3103b14a5b31726d75414d
Patch Set 1 #Patch Set 2 : add perftest #
Total comments: 6
Patch Set 3 : tweak tests #
Total comments: 2
Patch Set 4 : rename test (String16Cstor) #Patch Set 5 : test onliy #
Messages
Total messages: 33 (16 generated)
agrieve@chromium.org changed reviewers: + kozyatinskiy@chromium.org
Note: this is my first attempt at a v8 change. A fresh v8 checkout left me unable to upload due to cpplint.py not existing, so this was uploaded with --bypass-hooks.
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we measure performance changes? For cpplint.py I think you need to have depot_tools in PATH, ideally depot_tools should be first in PATH string.
On 2017/06/30 01:06:46, kozy wrote: > Can we measure performance changes? > For cpplint.py I think you need to have depot_tools in PATH, ideally depot_tools > should be first in PATH string. Definitely! Any pointers for how to go about doing so? Does v8 have perf bots? Is there a way to run Chrome's perf bots with this change patched in?
On 2017/06/30 01:32:14, agrieve wrote: > On 2017/06/30 01:06:46, kozy wrote: > > Can we measure performance changes? > > For cpplint.py I think you need to have depot_tools in PATH, ideally > depot_tools > > should be first in PATH string. > > Definitely! Any pointers for how to go about doing so? Does v8 have perf bots? > Is there a way to run Chrome's perf bots with this change patched in? Added a perf test. ptal. Ran the tests via: python -u tools/run_perf.py --arch x64 --binary-override-path out/Release/d8 test/js-perf-test/JSTests.json --filter="JSTests/Inspector" Results before change: >>> Running suite: JSTests/Inspector >>> Stdout (#1): Debugger.paused-Inspector(Score): 50.0 Runtime.evaluate-Inspector(Score): 97.6 >>> Stdout (#2): Debugger.paused-Inspector(Score): 50.8 Runtime.evaluate-Inspector(Score): 99.0 >>> Stdout (#3): Debugger.paused-Inspector(Score): 50.1 Runtime.evaluate-Inspector(Score): 98.7 Results after change: >>> Running suite: JSTests/Inspector >>> Stdout (#1): Debugger.paused-Inspector(Score): 54.3 Runtime.evaluate-Inspector(Score): 95.0 >>> Stdout (#2): Debugger.paused-Inspector(Score): 49.7 Runtime.evaluate-Inspector(Score): 77.2 >>> Stdout (#3): Debugger.paused-Inspector(Score): 51.6 Runtime.evaluate-Inspector(Score): 98.4
https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... File test/js-perf-test/Inspector/debugger.js (right): https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... test/js-perf-test/Inspector/debugger.js:11: SendMessage('Debugger.enable'); Please bring back: // force lazy compilation of inspector related scripts send(JSON.stringify({id: ++lastId, method: 'Runtime.evaluate', params: {expression: ''}})); It forces injected-script-source.js compilation, otherwise we'll measure not only Debugger.paused event but injected-script-source.js compilation as well. https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... File test/js-perf-test/Inspector/runtime.js (right): https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... test/js-perf-test/Inspector/runtime.js:11: SendMessage('Debugger.enable'); You don't need to enable Debugger to run Runtime.evaluate. https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... test/js-perf-test/Inspector/runtime.js:20: let result = SendMessage('Runtime.evaluate', {expression: '"foo"'}); I was experimenting with callgrind and tried to maximize time which we spend in String16 cstor and in maximum got 5% time, I hope it will give us at least 10% in string16 cstors in benchmark (since 5% it's for all code including shell startup and everything else) and will make it useful: SendMessage('Runtime.evaluate', {expression: '({})', returnByValue: true}); You actually don't need to check result of this method since we only measure performance here.
New results: Before out-lining: >>> Running suite: JSTests/Inspector >>> Stdout (#1): Debugger.paused-Inspector(Score): 54.1 Runtime.evaluate-Inspector(Score): 299 >>> Stdout (#2): Debugger.paused-Inspector(Score): 52.2 Runtime.evaluate-Inspector(Score): 318 >>> Stdout (#3): Debugger.paused-Inspector(Score): 53.3 Runtime.evaluate-Inspector(Score): 312 After: >>> Running suite: JSTests/Inspector >>> Stdout (#1): Debugger.paused-Inspector(Score): 46.7 Runtime.evaluate-Inspector(Score): 316 >>> Stdout (#2): Debugger.paused-Inspector(Score): 56.0 Runtime.evaluate-Inspector(Score): 359 >>> Stdout (#3): Debugger.paused-Inspector(Score): 55.5 Runtime.evaluate-Inspector(Score): 309 https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... File test/js-perf-test/Inspector/debugger.js (right): https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... test/js-perf-test/Inspector/debugger.js:11: SendMessage('Debugger.enable'); On 2017/07/06 21:18:31, kozy wrote: > Please bring back: > // force lazy compilation of inspector related scripts > send(JSON.stringify({id: ++lastId, method: 'Runtime.evaluate', params: > {expression: ''}})); > > It forces injected-script-source.js compilation, otherwise we'll measure not > only Debugger.paused event but injected-script-source.js compilation as well. Whoops! Done https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... File test/js-perf-test/Inspector/runtime.js (right): https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... test/js-perf-test/Inspector/runtime.js:11: SendMessage('Debugger.enable'); On 2017/07/06 21:18:32, kozy wrote: > You don't need to enable Debugger to run Runtime.evaluate. Done. https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... test/js-perf-test/Inspector/runtime.js:20: let result = SendMessage('Runtime.evaluate', {expression: '"foo"'}); On 2017/07/06 21:18:32, kozy wrote: > I was experimenting with callgrind and tried to maximize time which we spend in > String16 cstor and in maximum got 5% time, I hope it will give us at least 10% > in string16 cstors in benchmark (since 5% it's for all code including shell > startup and everything else) and will make it useful: > > SendMessage('Runtime.evaluate', {expression: '({})', returnByValue: true}); > > You actually don't need to check result of this method since we only measure > performance here. Done.
On 2017/07/07 18:34:49, agrieve wrote: > New results: > > Before out-lining: > >>> Running suite: JSTests/Inspector > >>> Stdout (#1): > Debugger.paused-Inspector(Score): 54.1 > Runtime.evaluate-Inspector(Score): 299 > > >>> Stdout (#2): > Debugger.paused-Inspector(Score): 52.2 > Runtime.evaluate-Inspector(Score): 318 > > >>> Stdout (#3): > Debugger.paused-Inspector(Score): 53.3 > Runtime.evaluate-Inspector(Score): 312 > > After: > >>> Running suite: JSTests/Inspector > >>> Stdout (#1): > Debugger.paused-Inspector(Score): 46.7 > Runtime.evaluate-Inspector(Score): 316 > > >>> Stdout (#2): > Debugger.paused-Inspector(Score): 56.0 > Runtime.evaluate-Inspector(Score): 359 > > >>> Stdout (#3): > Debugger.paused-Inspector(Score): 55.5 > Runtime.evaluate-Inspector(Score): 309 > > https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... > File test/js-perf-test/Inspector/debugger.js (right): > > https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... > test/js-perf-test/Inspector/debugger.js:11: SendMessage('Debugger.enable'); > On 2017/07/06 21:18:31, kozy wrote: > > Please bring back: > > // force lazy compilation of inspector related scripts > > send(JSON.stringify({id: ++lastId, method: 'Runtime.evaluate', params: > > {expression: ''}})); > > > > It forces injected-script-source.js compilation, otherwise we'll measure not > > only Debugger.paused event but injected-script-source.js compilation as well. > > Whoops! Done > > https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... > File test/js-perf-test/Inspector/runtime.js (right): > > https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... > test/js-perf-test/Inspector/runtime.js:11: SendMessage('Debugger.enable'); > On 2017/07/06 21:18:32, kozy wrote: > > You don't need to enable Debugger to run Runtime.evaluate. > > Done. > > https://codereview.chromium.org/2962213002/diff/20001/test/js-perf-test/Inspe... > test/js-perf-test/Inspector/runtime.js:20: let result = > SendMessage('Runtime.evaluate', {expression: '"foo"'}); > On 2017/07/06 21:18:32, kozy wrote: > > I was experimenting with callgrind and tried to maximize time which we spend > in > > String16 cstor and in maximum got 5% time, I hope it will give us at least 10% > > in string16 cstors in benchmark (since 5% it's for all code including shell > > startup and everything else) and will make it useful: > > > > SendMessage('Runtime.evaluate', {expression: '({})', returnByValue: true}); > > > > You actually don't need to check result of this method since we only measure > > performance here. > > Done. Also - figured out presubmit trouble. Turns our our machines has a /usr/bin/cpplint.py that's symlinked to a bash file :(. Works fine when invoked directly, but not when running "python /usr/bin/cpplint.py"
Thanks! lgtm! Some kind of workaround for cpplint issue: you can put depot_tools first in your PATH variable. But it will work only when you don't need original cpplint. https://codereview.chromium.org/2962213002/diff/40001/test/js-perf-test/JSTes... File test/js-perf-test/JSTests.json (right): https://codereview.chromium.org/2962213002/diff/40001/test/js-perf-test/JSTes... test/js-perf-test/JSTests.json:518: {"name": "Runtime.evaluate"} Please call it Runtime.evaluate(String16Cstor) (I hope brackets are allowed in test name).
https://codereview.chromium.org/2962213002/diff/40001/test/js-perf-test/JSTes... File test/js-perf-test/JSTests.json (right): https://codereview.chromium.org/2962213002/diff/40001/test/js-perf-test/JSTes... test/js-perf-test/JSTests.json:518: {"name": "Runtime.evaluate"} On 2017/07/07 21:51:06, kozy wrote: > Please call it Runtime.evaluate(String16Cstor) (I hope brackets are allowed in > test name). Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kozyatinskiy@chromium.org Link to the patchset: https://codereview.chromium.org/2962213002/#ps60001 (title: "rename test (String16Cstor)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
agrieve@chromium.org changed reviewers: + rmcilroy@chromium.org
rmcilroy@chromium.org: Please review changes in js-perf-test
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/44931)
RS-LGTM
Could you please split this CL: land test first in this CL and then separately in 2-3 days land String16 part. It allows perf bot to measure actual difference with and without change.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kozyatinskiy@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2962213002/#ps80001 (title: "test onliy")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make String16 consturctors non-inline to save binary size (150kb) Measured on Android arm32 build ========== to ========== Add js-perf-test to excercise v8_inspector::String16 ==========
Description was changed from ========== Add js-perf-test to excercise v8_inspector::String16 ========== to ========== Add js-perf-test to exercise v8_inspector::String16 ==========
Description was changed from ========== Add js-perf-test to exercise v8_inspector::String16 ========== to ========== Add js-perf-test to exercise v8_inspector::String16 BUG=chromium:738469 ==========
On 2017/07/10 17:15:43, kozy wrote: > Could you please split this CL: land test first in this CL and then separately > in 2-3 days land String16 part. It allows perf bot to measure actual difference > with and without change. Done. Split into: https://codereview.chromium.org/2975133002 Will aim to land on Monday.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499903592376960,
"parent_rev": "ea632716d72cb87904e3d78ddb55a6c0c5f1d59f", "commit_rev":
"19ffed65ba3adc0f4e3103b14a5b31726d75414d"}
Message was sent while issue was closed.
Description was changed from ========== Add js-perf-test to exercise v8_inspector::String16 BUG=chromium:738469 ========== to ========== Add js-perf-test to exercise v8_inspector::String16 BUG=chromium:738469 Review-Url: https://codereview.chromium.org/2962213002 Cr-Commit-Position: refs/heads/master@{#46612} Committed: https://chromium.googlesource.com/v8/v8/+/19ffed65ba3adc0f4e3103b14a5b31726d7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/19ffed65ba3adc0f4e3103b14a5b31726d7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
