Bug 174188 – NewArray in FTLLowerDFGToB3 does not handle speculating on doubles when having a bad time

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

<rdar://problem/30581423>

[Saam Barati]

Created attachment 314674 [details]
patch

[Mark Lam]

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.

[JF Bastien]

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

[Saam Barati]

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

[WebKit Commit Bot]

Comment on attachment 314674 [details]
patch

Clearing flags on attachment: 314674

Committed r219187: <http://trac.webkit.org/changeset/219187>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

(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.

[Saam Barati]

(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.

[Yusuke Suzuki]

(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.

[Saam Barati]

(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.

[Yusuke Suzuki]

(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.)