...
<rdar://problem/30581423>
Created attachment 314674 [details] patch
Comment on attachment 314674 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314674&action=review r=me with question addressed. > Tools/Scripts/run-jsc-stress-tests:862 > - run("ftl-no-cjit-b3o1", *(FTL_OPTIONS + NO_CJIT_OPTIONS + B3O1_OPTIONS + optionalTestSpecificOptions)) > + run("ftl-no-cjit-b3o1", "--useArrayAllocationProfiling=false", *(FTL_OPTIONS + NO_CJIT_OPTIONS + B3O1_OPTIONS + optionalTestSpecificOptions)) Does it make sense to use --useArrayAllocationProfiling=false for all ftl-no-cjit-b3o1 tests? If not, you can just as easily add //@ runFTLNoCJITB3O1("--useArrayAllocationProfiling=false") at the top of the new test to configure that test alone to use this option.
Comment on attachment 314674 [details] patch r=me > JSTests/stress/new-array-having-a-bad-time-double.js:12 > +Object.defineProperty(Object.prototype, "10000", {get() { return 20; }}); Ew
Comment on attachment 314674 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=314674&action=review >> Tools/Scripts/run-jsc-stress-tests:862 >> + run("ftl-no-cjit-b3o1", "--useArrayAllocationProfiling=false", *(FTL_OPTIONS + NO_CJIT_OPTIONS + B3O1_OPTIONS + optionalTestSpecificOptions)) > > Does it make sense to use --useArrayAllocationProfiling=false for all ftl-no-cjit-b3o1 tests? If not, you can just as easily add //@ runFTLNoCJITB3O1("--useArrayAllocationProfiling=false") at the top of the new test to configure that test alone to use this option. The reason I added it this way is I thought we might as well run a test type this way since it’ll make our compilers hit certain code paths more often than it would otherwise
Comment on attachment 314674 [details] patch Clearing flags on attachment: 314674 Committed r219187: <http://trac.webkit.org/changeset/219187>
All reviewed patches have been landed. Closing bug.
(In reply to WebKit Commit Bot from comment #7) > All reviewed patches have been landed. Closing bug. It seems that this causes regression. https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=object-assign-es5 https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=object-assign-es6 But it seems a bit strange. I'll check it when I have time.
(In reply to Yusuke Suzuki from comment #8) > (In reply to WebKit Commit Bot from comment #7) > > All reviewed patches have been landed. Closing bug. > > It seems that this causes regression. > https://arewefastyet.com/#machine=29&view=single&suite=six- > speed&subtest=object-assign-es5 > https://arewefastyet.com/#machine=29&view=single&suite=six- > speed&subtest=object-assign-es6 > > But it seems a bit strange. I'll check it when I have time. I'm not sure we even care about such a microbenchmark. However, there are other commits in that range. Maybe it's them? I don't see how this patch could slow that code down.
(In reply to Saam Barati from comment #9) > (In reply to Yusuke Suzuki from comment #8) > > (In reply to WebKit Commit Bot from comment #7) > > > All reviewed patches have been landed. Closing bug. > > > > It seems that this causes regression. > > https://arewefastyet.com/#machine=29&view=single&suite=six- > > speed&subtest=object-assign-es5 > > https://arewefastyet.com/#machine=29&view=single&suite=six- > > speed&subtest=object-assign-es6 > > > > But it seems a bit strange. I'll check it when I have time. > > I'm not sure we even care about such a microbenchmark. However, there are > other commits in that range. Maybe it's them? I don't see how this patch > could slow that code down. If this regression is real (I'm still suspecting b/c this change is unlikely related to this), I think we should fix it because this bench measures the time of Object.assign(). Which is super frequently used in user's code including Speedometer2.0 Redux-React.
(In reply to Yusuke Suzuki from comment #10) > (In reply to Saam Barati from comment #9) > > (In reply to Yusuke Suzuki from comment #8) > > > (In reply to WebKit Commit Bot from comment #7) > > > > All reviewed patches have been landed. Closing bug. > > > > > > It seems that this causes regression. > > > https://arewefastyet.com/#machine=29&view=single&suite=six- > > > speed&subtest=object-assign-es5 > > > https://arewefastyet.com/#machine=29&view=single&suite=six- > > > speed&subtest=object-assign-es6 > > > > > > But it seems a bit strange. I'll check it when I have time. > > > > I'm not sure we even care about such a microbenchmark. However, there are > > other commits in that range. Maybe it's them? I don't see how this patch > > could slow that code down. > > If this regression is real (I'm still suspecting b/c this change is unlikely > related to this), I think we should fix it because this bench measures the > time of Object.assign(). Which is super frequently used in user's code > including Speedometer2.0 Redux-React. I'll measure this locally.
(In reply to Saam Barati from comment #11) > (In reply to Yusuke Suzuki from comment #10) > > (In reply to Saam Barati from comment #9) > > > (In reply to Yusuke Suzuki from comment #8) > > > > (In reply to WebKit Commit Bot from comment #7) > > > > > All reviewed patches have been landed. Closing bug. > > > > > > > > It seems that this causes regression. > > > > https://arewefastyet.com/#machine=29&view=single&suite=six- > > > > speed&subtest=object-assign-es5 > > > > https://arewefastyet.com/#machine=29&view=single&suite=six- > > > > speed&subtest=object-assign-es6 > > > > > > > > But it seems a bit strange. I'll check it when I have time. > > > > > > I'm not sure we even care about such a microbenchmark. However, there are > > > other commits in that range. Maybe it's them? I don't see how this patch > > > could slow that code down. > > > > If this regression is real (I'm still suspecting b/c this change is unlikely > > related to this), I think we should fix it because this bench measures the > > time of Object.assign(). Which is super frequently used in user's code > > including Speedometer2.0 Redux-React. > > I'll measure this locally. It seems that performance regression is fixed. I'm still not sure what happens... (My guess is C++ compiler inlining...., but idk. If such a thing can change performance so significantly, we should check the implementation in Object.assign and we should annotate NEVER_INLINE / ALWAYS_INLINE carefully since this Object.assign() is very important function.)