Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1146073002:
Add perf json for simd.js benchmarks. (Closed)
DescriptionAdd perf json for simd.js benchmarks.
Using a script to generate the somewhat redundant json.
BUG=v8:4124
LOG=N
R=machenbach@chromium.org,bbudge@chromium.org
TEST=None
Committed: https://crrev.com/e77c69b5caadba2ca42e0019fad7836225b27d6d
Cr-Commit-Position: refs/heads/master@{#28514}
Patch Set 1 #
Total comments: 17
Patch Set 2 : dump json #
Total comments: 2
Patch Set 3 : sort #
Messages
Total messages: 11 (2 generated)
lgtm ~ comments: https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json File test/simdjs/SimdJs.json (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:3: "run_count": 5, Is 5 ok? What's the total time of running this? https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:27: "flags": ["--harmony-object", "test/simdjs/harness-adapt.js"], Should this be benchmark-adapt? https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:28: "path": ["../../"], Can the workdir not be "." and all paths be relative to it? Or are there hardcoded loads in the runner relative to the v8 base dir? https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:32: "main": "test/simdjs/harness-finish.js", It will make a new call to d8 for each "main" file. I guess that's what you want. The benchmarks will appear in a hierarchy like this: SIMDJS/kernel-template/SIMD SIMDJS/kernel-template/Non-SIMD etc https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:45: "flags": ["test/simdjs/data/src/benchmarks/averageFloat32x4.js"], Does this work as expected? Flags are passed to d8 before the main file I think. So you'd have: d8 --harmony-object test/simdjs/harness-adapt.js test/simdjs/data/src/benchmarks/averageFloat32x4.js test/simdjs/harness-finish.js https://codereview.chromium.org/1146073002/diff/1/test/simdjs/benchmarks-adap... File test/simdjs/benchmarks-adapt.js (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/benchmarks-adap... test/simdjs/benchmarks-adapt.js:6: This is not used yet. See comment in other file. https://codereview.chromium.org/1146073002/diff/1/test/simdjs/generate.py File test/simdjs/generate.py (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/generate.py#new... test/simdjs/generate.py:12: I'm rubber-stamping this. Instead of a string, a python dict that's json.dumped could have been more readable. But I don't care...
PTAL https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json File test/simdjs/SimdJs.json (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:3: "run_count": 5, On 2015/05/20 10:33:16, Michael Achenbach wrote: > Is 5 ok? What's the total time of running this? It seems to be ok (I'm assuming this is because the 4 variants run for testings aren't done here and the default for the slow test is reasonably fast). https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:27: "flags": ["--harmony-object", "test/simdjs/harness-adapt.js"], On 2015/05/20 10:33:17, Michael Achenbach wrote: > Should this be benchmark-adapt? Actually this is right, but that file was from an attempt to approach this differently (invoke run.js once and collect everything), that proved more verbose. https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:28: "path": ["../../"], On 2015/05/20 10:33:16, Michael Achenbach wrote: > Can the workdir not be "." and all paths be relative to it? Or are there > hardcoded loads in the runner relative to the v8 base dir? This lets me share the harness-adapt/finish with the testing version of this. I could make another one for this case that's rooted here, but would need slightly different versions of those two. Which do you prefer https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:32: "main": "test/simdjs/harness-finish.js", On 2015/05/20 10:33:17, Michael Achenbach wrote: > It will make a new call to d8 for each "main" file. I guess that's what you > want. The benchmarks will appear in a hierarchy like this: > SIMDJS/kernel-template/SIMD > SIMDJS/kernel-template/Non-SIMD > etc I assume this is preferrable, as then then run_perf knows about each? I had tried it the other way round (run run.js once), but then it seemed more verbose and then run_perf doesn't know about all the sub -tests. https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:45: "flags": ["test/simdjs/data/src/benchmarks/averageFloat32x4.js"], On 2015/05/20 10:33:17, Michael Achenbach wrote: > Does this work as expected? Flags are passed to d8 before the main file I think. > So you'd have: > d8 --harmony-object test/simdjs/harness-adapt.js > test/simdjs/data/src/benchmarks/averageFloat32x4.js > test/simdjs/harness-finish.js Yes this is the intended set of args. https://codereview.chromium.org/1146073002/diff/1/test/simdjs/benchmarks-adap... File test/simdjs/benchmarks-adapt.js (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/benchmarks-adap... test/simdjs/benchmarks-adapt.js:6: On 2015/05/20 10:33:17, Michael Achenbach wrote: > This is not used yet. See comment in other file. Dropped, leftover from the other approach. https://codereview.chromium.org/1146073002/diff/1/test/simdjs/generate.py File test/simdjs/generate.py (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/generate.py#new... test/simdjs/generate.py:12: On 2015/05/20 10:33:17, Michael Achenbach wrote: > I'm rubber-stamping this. Instead of a string, a python dict that's json.dumped > could have been more readable. But I don't care... Yeah I almost did that. Switching.
lgtm https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json File test/simdjs/SimdJs.json (right): https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:27: "flags": ["--harmony-object", "test/simdjs/harness-adapt.js"], On 2015/05/20 11:30:05, bradn wrote: > On 2015/05/20 10:33:17, Michael Achenbach wrote: > > Should this be benchmark-adapt? > > Actually this is right, but that file was from an attempt to approach this > differently (invoke run.js once and collect everything), that proved more > verbose. ok https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:28: "path": ["../../"], On 2015/05/20 11:30:04, bradn wrote: > On 2015/05/20 10:33:16, Michael Achenbach wrote: > > Can the workdir not be "." and all paths be relative to it? Or are there > > hardcoded loads in the runner relative to the v8 base dir? > > This lets me share the harness-adapt/finish with the testing version of this. > I could make another one for this case that's rooted here, but would need > slightly different versions of those two. > Which do you prefer I'm fine if you keep it like that for now. With harness-adapt/finish, do you mean the file test/simdjs/harness-finish.js? If yes, couldn't the relative path be left out there as well? https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... test/simdjs/SimdJs.json:32: "main": "test/simdjs/harness-finish.js", On 2015/05/20 11:30:04, bradn wrote: > On 2015/05/20 10:33:17, Michael Achenbach wrote: > > It will make a new call to d8 for each "main" file. I guess that's what you > > want. The benchmarks will appear in a hierarchy like this: > > SIMDJS/kernel-template/SIMD > > SIMDJS/kernel-template/Non-SIMD > > etc > > I assume this is preferrable, as then then run_perf knows about each? > I had tried it the other way round (run run.js once), but then it seemed more > verbose and then run_perf doesn't know about all the sub -tests. If the behavior of the benchmark is what you want then this is preferred. There are some benchmarks that behave differently when one runner is called, e.g. the different line items influence each other and optimizations from one line item might survive into the methods of the next. Even though this sounds bad, we sometimes keep this kind of behavior if it's required to be as close as possible to some publicly existing benchmark. If the benchmark is run by other parties in a different way, you might sometimes not see the same results as they do. https://codereview.chromium.org/1146073002/diff/20001/test/simdjs/generate.py File test/simdjs/generate.py (right): https://codereview.chromium.org/1146073002/diff/20001/test/simdjs/generate.py... test/simdjs/generate.py:56: fh.write(json.dumps(output, separators=(',',': '), indent=2)) Maybe add sort_keys=True to be deterministic...
On 2015/05/20 12:20:03, Michael Achenbach wrote: > lgtm > > https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json > File test/simdjs/SimdJs.json (right): > > https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... > test/simdjs/SimdJs.json:27: "flags": ["--harmony-object", > "test/simdjs/harness-adapt.js"], > On 2015/05/20 11:30:05, bradn wrote: > > On 2015/05/20 10:33:17, Michael Achenbach wrote: > > > Should this be benchmark-adapt? > > > > Actually this is right, but that file was from an attempt to approach this > > differently (invoke run.js once and collect everything), that proved more > > verbose. > > ok > > https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... > test/simdjs/SimdJs.json:28: "path": ["../../"], > On 2015/05/20 11:30:04, bradn wrote: > > On 2015/05/20 10:33:16, Michael Achenbach wrote: > > > Can the workdir not be "." and all paths be relative to it? Or are there > > > hardcoded loads in the runner relative to the v8 base dir? > > > > This lets me share the harness-adapt/finish with the testing version of this. > > I could make another one for this case that's rooted here, but would need > > slightly different versions of those two. > > Which do you prefer > > I'm fine if you keep it like that for now. With harness-adapt/finish, do you > mean the file test/simdjs/harness-finish.js? If yes, couldn't the relative path > be left out there as well? > It needs to be rooted that way because the test is run from there. > https://codereview.chromium.org/1146073002/diff/1/test/simdjs/SimdJs.json#new... > test/simdjs/SimdJs.json:32: "main": "test/simdjs/harness-finish.js", > On 2015/05/20 11:30:04, bradn wrote: > > On 2015/05/20 10:33:17, Michael Achenbach wrote: > > > It will make a new call to d8 for each "main" file. I guess that's what you > > > want. The benchmarks will appear in a hierarchy like this: > > > SIMDJS/kernel-template/SIMD > > > SIMDJS/kernel-template/Non-SIMD > > > etc > > > > I assume this is preferrable, as then then run_perf knows about each? > > I had tried it the other way round (run run.js once), but then it seemed more > > verbose and then run_perf doesn't know about all the sub -tests. > > If the behavior of the benchmark is what you want then this is preferred. There > are some benchmarks that behave differently when one runner is called, e.g. the > different line items influence each other and optimizations from one line item > might survive into the methods of the next. > > Even though this sounds bad, we sometimes keep this kind of behavior if it's > required to be as close as possible to some publicly existing benchmark. If the > benchmark is run by other parties in a different way, you might sometimes not > see the same results as they do. > That's actually an interesting point in light of a lunch conversation. It might be worth considering long term if this benchmark suite grows. My impression is that at the moment each tests is othogonal so it won't matter. But that could easily change. > https://codereview.chromium.org/1146073002/diff/20001/test/simdjs/generate.py > File test/simdjs/generate.py (right): > > https://codereview.chromium.org/1146073002/diff/20001/test/simdjs/generate.py... > test/simdjs/generate.py:56: fh.write(json.dumps(output, separators=(',',': '), > indent=2)) > Maybe add sort_keys=True to be deterministic...
https://codereview.chromium.org/1146073002/diff/20001/test/simdjs/generate.py File test/simdjs/generate.py (right): https://codereview.chromium.org/1146073002/diff/20001/test/simdjs/generate.py... test/simdjs/generate.py:56: fh.write(json.dumps(output, separators=(',',': '), indent=2)) On 2015/05/20 12:20:03, Michael Achenbach wrote: > Maybe add sort_keys=True to be deterministic... Done.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1146073002/#ps40001 (title: "sort")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146073002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e77c69b5caadba2ca42e0019fad7836225b27d6d Cr-Commit-Position: refs/heads/master@{#28514} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
