Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1746383003:
[js-perf-test] add microbenchmarks for Object.values(), Object.entries() (Closed)
|
Created:
4 years, 9 months ago by caitp (gmail) Modified:
4 years, 9 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[esnext] add microbenchmarks for Object.values(), Object.entries()
Add microbenchmark for these methods.
BUG=v8:4663
LOG=N
R=adamk@chromium.org, littledan@chrommium.org
Committed: https://crrev.com/705cb7fb707acb89405ed42ba4cd4d16d5924189
Cr-Commit-Position: refs/heads/master@{#34556}
Patch Set 1 #Patch Set 2 : Refactor tests for correctness + to take advantage of enum cache #Patch Set 3 : Move lodash tests to a subdirectory + new JSON file, add test with ~25 Maps (I think...?) #Patch Set 4 : rebased #
Total comments: 4
Patch Set 5 : Remove lodash tests :( and refactor tests #Patch Set 6 : bugfix #Patch Set 7 : Make the tests actually work #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 34 (10 generated)
ptal --- some microbenchmarks which might help to measure improvements for common cases. We should probably change the test harness so that the verification of the test results actually does something, and causes an error if they don't work. But that's a separate issue :<
Description was changed from ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. Also incorporates variations using methods provided by lodash/underscore, for comparison. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ========== to ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. Also incorporates variations using methods provided by lodash/underscore, for comparison. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ==========
adamk@chromium.org changed reviewers: + littledan@chromium.org - littledan@chrommium.org
(fixed littledan's email) I can see how it's useful to compare to lodash, but I'm not sure we want to check that in to JSTests (which so far have been pretty small and self-contained).
On 2016/03/03 01:59:37, adamk wrote: > (fixed littledan's email) > > I can see how it's useful to compare to lodash, but I'm not sure we want to > check that in to JSTests (which so far have been pretty small and > self-contained). Well, what's the solution to that? Creating a new json file, or extending the perf runner to be able to specify specific test cases to run? The latter would be a valuable feature anyways
adamk@chromium.org changed reviewers: + machenbach@chromium.org
+machenbach who may have thoughts on the infrastructure part of this
Infrastructure lg
cbruni@chromium.org changed reviewers: + cbruni@chromium.org
I tend to agree with adam on this. I think it would be enough to have a pure JS implementation of Object.values and Object.entries to expose the same pitfals/performance as lodash's version. You should definitely add a polymorphic case to this (THE most common mistake on the jsperf micro benchmarks). JS versions for these library functions work impressively well for monomorphic input data as the optimizing compiler can do a beautiful job. But as soon as you go megamorphic the C++ version will most likely beat the JS version. I would suggest adding a second set with many (>100) different (different maps!) input objects, which should expose a very different behavior.
On 2016/03/04 09:53:39, cbruni wrote: > I tend to agree with adam on this. > I think it would be enough to have a pure JS implementation of Object.values and > Object.entries to expose the same pitfals/performance as lodash's version. > > You should definitely add a polymorphic case to this (THE most common mistake on > the jsperf micro benchmarks). > JS versions for these library functions work impressively well for monomorphic > input data as the optimizing compiler can do a beautiful job. > But as soon as you go megamorphic the C++ version will most likely beat the JS > version. > > I would suggest adding a second set with many (>100) different (different maps!) > input objects, which should expose a very different behavior. If we have a tool for generating random objects with different maps, that would be a lot nicer to use than manually coming up with these test cases and verifying them. I've added cases with 25 objects with different property names / insertion orders, and other things (so I expect they should have different sets of transitions and thus different resulting Maps).
https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Lodas... File test/js-perf-test/Lodash/lodash.custom.js (right): https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Lodas... test/js-perf-test/Lodash/lodash.custom.js:2: * @license The infra isn't happy about the lodash file not containing v8 copyright header --- I'm not sure if this will cause gclient hooks to complain when cls not touching the file are uploaded, or not. It does complain on the dependent CL, so I worry that it might.
ugh, I appreciate the effort but I fear this is very hard to maintain.
So I have some suggestions:
1.) don't verify the output of Object.values (I doubt there is much use to this
here)
2.) construct the various objects with different maps a bit more predictably:
// Create 1k objects with different maps.
var objects = [];
for (var i=0; i<1000; i++) {
var object = {};
for (var j=0; j<10; j++) {
object['key-'+i+'-'+j] = 'property-'+i+'-'+j;
}
objects.push(object);
}
https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec...
File test/js-perf-test/Object/values.js (right):
https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec...
test/js-perf-test/Object/values.js:77: Object.values(object[24]),
How about something more sustainable:
var length = object.length;
for (var i = 0; i<length; i++) {
Object.values(object[i]))
}
I don't think we gain much value by tracking the results here.
https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Lodas... File test/js-perf-test/Lodash/lodash.custom.js (right): https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Lodas... test/js-perf-test/Lodash/lodash.custom.js:2: * @license On 2016/03/04 at 16:50:16, caitp wrote: > The infra isn't happy about the lodash file not containing v8 copyright header --- I'm not sure if this will cause gclient hooks to complain when cls not touching the file are uploaded, or not. It does complain on the dependent CL, so I worry that it might. For our purpose I guess it would suffice to just have microbenchmarks for Object.assign and Object.values.
https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... File test/js-perf-test/Object/values.js (right): https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... test/js-perf-test/Object/values.js:77: Object.values(object[24]), On 2016/03/04 17:15:25, cbruni wrote: > How about something more sustainable: > > var length = object.length; > for (var i = 0; i<length; i++) { > Object.values(object[i])) > } > > I don't think we gain much value by tracking the results here. Are you sure LICM won't just turn this into a "no-op" test, particularly in the lodash case? That's why the results are verified
On 2016/03/04 17:19:30, caitp wrote: > https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... > File test/js-perf-test/Object/values.js (right): > > https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... > test/js-perf-test/Object/values.js:77: Object.values(object[24]), > On 2016/03/04 17:15:25, cbruni wrote: > > How about something more sustainable: > > > > var length = object.length; > > for (var i = 0; i<length; i++) { > > Object.values(object[i])) > > } > > > > I don't think we gain much value by tracking the results here. > > Are you sure LICM won't just turn this into a "no-op" test, particularly in the > lodash case? That's why the results are verified Alright, I've removed the lodash tests, and implemented these tests with your more predictable path to megamorphic-ness.
On 2016/03/04 17:19:30, caitp wrote: > https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... > File test/js-perf-test/Object/values.js (right): > > https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... > test/js-perf-test/Object/values.js:77: Object.values(object[24]), > On 2016/03/04 17:15:25, cbruni wrote: > > How about something more sustainable: > > > > var length = object.length; > > for (var i = 0; i<length; i++) { > > Object.values(object[i])) > > } > > > > I don't think we gain much value by tracking the results here. > > Are you sure LICM won't just turn this into a "no-op" test, particularly in the > lodash case? That's why the results are verified Alright, I've removed the lodash tests, and implemented these tests with your more predictable path to megamorphic-ness.
On 2016/03/04 at 17:19:30, caitpotter88 wrote: > https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... > File test/js-perf-test/Object/values.js (right): > > https://codereview.chromium.org/1746383003/diff/60001/test/js-perf-test/Objec... > test/js-perf-test/Object/values.js:77: Object.values(object[24]), > On 2016/03/04 17:15:25, cbruni wrote: > > How about something more sustainable: > > > > var length = object.length; > > for (var i = 0; i<length; i++) { > > Object.values(object[i])) > > } > > > > I don't think we gain much value by tracking the results here. > > Are you sure LICM won't just turn this into a "no-op" test, particularly in the lodash case? That's why the results are verified You are right, but you could just gather the results and return them, or sum pu all the lengths of the keys/values. Typically that should be enough to avoid optimizing away the whole operation.
lgtm https://codereview.chromium.org/1746383003/diff/120001/test/js-perf-test/Obje... File test/js-perf-test/Object/entries.js (right): https://codereview.chromium.org/1746383003/diff/120001/test/js-perf-test/Obje... test/js-perf-test/Object/entries.js:70: if (JSON.stringify(expected) !== JSON.stringify(result)) { Right, this seems like a good solution to prevent total optimization.
Description was changed from ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. Also incorporates variations using methods provided by lodash/underscore, for comparison. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ========== to ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ==========
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746383003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746383003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11959)
On 2016/03/07 14:03:20, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_presubmit on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11959) ah, still need a top-level owner to sign off
lgtm stamp for toplevel OWNERs
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746383003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746383003/120001
Message was sent while issue was closed.
Description was changed from ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ========== to ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org ========== to ========== [esnext] add microbenchmarks for Object.values(), Object.entries() Add microbenchmark for these methods. BUG=v8:4663 LOG=N R=adamk@chromium.org, littledan@chrommium.org Committed: https://crrev.com/705cb7fb707acb89405ed42ba4cd4d16d5924189 Cr-Commit-Position: refs/heads/master@{#34556} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/705cb7fb707acb89405ed42ba4cd4d16d5924189 Cr-Commit-Position: refs/heads/master@{#34556}
Message was sent while issue was closed.
Sorry for not seeing that immediately: https://codereview.chromium.org/1746383003/diff/120001/test/js-perf-test/JSTe... File test/js-perf-test/JSTests.json (right): https://codereview.chromium.org/1746383003/diff/120001/test/js-perf-test/JSTe... test/js-perf-test/JSTests.json:111: "path": ["."], This fails on arm + android since it landed. I think the path needs to b Object as it was before, since there is the run.js file.
Message was sent while issue was closed.
https://codereview.chromium.org/1746383003/diff/120001/test/js-perf-test/JSTe... File test/js-perf-test/JSTests.json (right): https://codereview.chromium.org/1746383003/diff/120001/test/js-perf-test/JSTe... test/js-perf-test/JSTests.json:111: "path": ["."], On 2016/03/09 08:22:53, Michael Achenbach wrote: > This fails on arm + android since it landed. I think the path needs to b Object > as it was before, since there is the run.js file. oops, I'll fix that |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
