Bug 220322

Summary: BooleanConstructor should be inlined in DFG / FTL
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch ysuzuki: review+

Description Alexey Shvayka 2021-01-05 05:12:34 PST
BooleanConstructor should be inlined in DFG / FTL
Comment 1 Alexey Shvayka 2021-01-05 05:25:13 PST
Created attachment 416996 [details]
Patch
Comment 2 Alexey Shvayka 2021-01-05 05:27:15 PST
Comment on attachment 416996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416996&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        `array.filter(Boolean)` is a rather popular idiom for compacting an array (removing falsy items).

For example https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_compact

> Source/JavaScriptCore/ChangeLog:18
> +        This change advances provided microbenchmark by 110%, and is neutral for other ToBoolean cases.

                                           r270932                    patch                                       

array-filter-boolean-constructor      130.0720+-2.7747     ^     61.4302+-1.1755        ^ definitely 2.1174x faster
Comment 3 Yusuke Suzuki 2021-01-05 12:24:25 PST
Comment on attachment 416996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416996&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1803
> +void SpeculativeJIT::compileToBooleanObjectOrOther(Edge nodeUse, bool invert)

While this function cares about masqueradesAsUndefined, I don't think this is necessary for Boolean constructor.
Can you change the following things?

1. Let's change ToBoolean DFG node to CallBooleanConstructor node to make it more explicit that this is boolean constructor call (ToBoolean sounds like we should care masqueradesAsUndefined too).
2. Let's remove masqueradesAsUndefined handlings for CallBooleanConstructor.
Comment 4 Alexey Shvayka 2021-01-05 17:38:13 PST
(In reply to Yusuke Suzuki from comment #3)
> While this function cares about masqueradesAsUndefined, I don't think this
> is necessary for Boolean constructor.

Sadly, it is necessary: Annex B for [[IsHTMLDDA]] modifies ToBoolean operation itself: https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot-to-boolean.
Chrome and Firefox follow it for `Boolean(document.all) => false`, as does this patch.
Updated stress tests do cover masqueraders, I will make sure to note this in ChangeLog.
Comment 5 Yusuke Suzuki 2021-01-05 19:23:16 PST
Oh, wow! Fun…
Comment 6 Yusuke Suzuki 2021-01-06 01:25:08 PST
Comment on attachment 416996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416996&action=review

Looks good. But putting r- since I found several bugs.

> Source/JavaScriptCore/ChangeLog:20
> +        * dfg/DFGAbstractInterpreterInlines.h:

Yeah, please add notes about masquerade as undefined thing in ChangeLog, and please add tests for that :)
That's awesome.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1538
>          GPRTemporary result(this, Reuse, value);
> -        m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
> +        if (invert)
> +            m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
>          booleanResult(result.gpr(), node);

Reuse is "Reuse if it is possible" If node->child1's use count is not zero at this point, GPRTemporary allocates a new register.
So, if you do not move value to result, result can contain a garbage.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1905
>              GPRTemporary result(this, Reuse, value);

While this is old code, I wonder if SpeculateBooleanOperand is correct here.
Can you check what format is generated from SpeculateBooleanOperand? JSValue? Or 0/1?
Maybe, in 64bit, we never use DataFormatBoolean in DFG?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1924
> +        if (invert)
> +            m_jit.xor64(TrustedImm32(JSValue::ValueTrue), result.gpr());

Is it correct? If `invert` is false, is result the JSValue format? I think result is 0/1 at this point, and it is not JSBoolean.
Comment 7 Radar WebKit Bug Importer 2021-01-12 05:13:13 PST
<rdar://problem/73036835>
Comment 8 Alexey Shvayka 2021-03-03 07:41:20 PST
Created attachment 422080 [details]
Patch

Move value into Reuse register, fix !invert case of BooleanUse, improve tests, and add note on masquerader to ChangeLog.
Comment 9 Alexey Shvayka 2021-03-03 07:49:33 PST
(In reply to Yusuke Suzuki from comment #6)

Thank you for meticulous review, Yusuke!

> Yeah, please add notes about masquerade as undefined thing in ChangeLog, and
> please add tests for that :)

Added.

> Reuse is "Reuse if it is possible" If node->child1's use count is not zero
> at this point, GPRTemporary allocates a new register.
> So, if you do not move value to result, result can contain a garbage.

Fixed by emitting a `mov`, as we do in 64-bit version.

> While this is old code, I wonder if SpeculateBooleanOperand is correct here.
> Can you check what format is generated from SpeculateBooleanOperand?
> JSValue? Or 0/1?

I can confirm it is JSValue, so XOR against 1 is correct.

> Maybe, in 64bit, we never use DataFormatBoolean in DFG?

That seems to be the case.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1924
> > +        if (invert)
> > +            m_jit.xor64(TrustedImm32(JSValue::ValueTrue), result.gpr());
> 
> Is it correct? If `invert` is false, is result the JSValue format? I think
> result is 0/1 at this point, and it is not JSBoolean.

Yeah, it was 0/1, and non-invert case was not covered by the tests.
I've updated the coverage so it now crashes with previous patch, and fixed to XOR against JSValue::ValueFalse.
Comment 10 Yusuke Suzuki 2021-03-04 10:57:13 PST
Comment on attachment 422080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422080&action=review

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1539
> +        m_jit.move(value.gpr(), result.gpr());
> +        if (invert)
> +            m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());

Maybe,

if (invert)
    m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
else
    m_jit.move(value.gpr(), result.gpr());

is clearer.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1909
>              m_jit.move(value.gpr(), result.gpr());
> -            m_jit.xor64(TrustedImm32(true), result.gpr());
> +            if (invert)
> +                m_jit.xor64(TrustedImm32(1), result.gpr());

Ditto.

if (invert)
    m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
else
    m_jit.move(value.gpr(), result.gpr());
Comment 11 Alexey Shvayka 2021-03-06 06:42:47 PST
Committed r274037 (234973@main): <https://commits.webkit.org/234973@main>
Comment 12 Alexey Shvayka 2021-03-06 06:52:14 PST
(In reply to Yusuke Suzuki from comment #10)
> Maybe,
> 
> if (invert)
>     m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
> else
>     m_jit.move(value.gpr(), result.gpr());
> 
> is clearer.

Nice suggestion, landed.

I wonder if there might be a bug because 32-bit compileToBoolean() has no path with type check for BooleanUse?
Comment 13 Yusuke Suzuki 2021-03-06 22:54:30 PST
(In reply to Alexey Shvayka from comment #12)
> (In reply to Yusuke Suzuki from comment #10)
> > Maybe,
> > 
> > if (invert)
> >     m_jit.xor32(TrustedImm32(1), value.gpr(), result.gpr());
> > else
> >     m_jit.move(value.gpr(), result.gpr());
> > 
> > is clearer.
> 
> Nice suggestion, landed.
> 
> I wonder if there might be a bug because 32-bit compileToBoolean() has no
> path with type check for BooleanUse?

Isn't it checking the type with SpeculateBooleanOperand?