Bug 198253 – [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv take the DFG Int32 fast path even if Baseline constantly produces Double result

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

[Yusuke Suzuki]

It causes unnecessary OSRExit in async-fs Math.random code.

[Yusuke Suzuki]

Created attachment 378452 [details]
Patch

[Mark Lam]

Comment on attachment 378452 [details]
Patch

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

I think this is the wrong place to do this fix.  This method says bigIntOrInt32Type().  It's misleading to have it also return TypeMaybeNumber.  The real issue is in forBitOp(), which happens to be the only client of this method.  I think we should just do the fix in forBitOp() and remove bigIntOrInt32Type().  Alternatively, rename bigIntOrInt32Type() to bigIntOrInt32TypeOrNumber() if you think there will be other clients of it coing soon.

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv in DFG Int32 fast path even if Baseline constantly produces Double result

I suggest rephrasing as "making ArithDiv take the DFG Int32 fast path".

> Source/JavaScriptCore/ChangeLog:8
> +        ResultType of bitwise operation needs including TypeMaybeNumber. TypeInt32 is something like a flag indicating the number looks like a int32.

I suggest rephrasing as "needs to include TypeMaybeNumber".

[Yusuke Suzuki]

(In reply to Mark Lam from comment #2)
> Comment on attachment 378452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378452&action=review
> 
> I think this is the wrong place to do this fix.  This method says
> bigIntOrInt32Type().  It's misleading to have it also return
> TypeMaybeNumber.  The real issue is in forBitOp(), which happens to be the
> only client of this method.  I think we should just do the fix in forBitOp()
> and remove bigIntOrInt32Type().  Alternatively, rename bigIntOrInt32Type()
> to bigIntOrInt32TypeOrNumber() if you think there will be other clients of
> it coing soon.

The name of flags in ResultType is misleading, but TypeInt32 and the other flags are not the same meaning. And TypeInt32 is not even in a type's set. We can see the definition of TypeBits does not include TypeInt32.

static constexpr Type TypeBits = TypeMaybeNumber | TypeMaybeString | TypeMaybeBigInt | TypeMaybeNull | TypeMaybeBool | TypeMaybeOther;

This is because TypeInt32 is a bit special boolean flag while other ones are entry of type set. So logically, ResultType is something like this.

struct ResultType {
    bool m_looksInt32;
    Set<Type> m_maybeTypes;
};

We are using bit set just to encode ResultType in bytecode as an int.
So, bigIntOrInt32Type definition right now is wrong since it is saying,

"It is definitely BigInt, and looks Int32".

This patch adds TypeMaybeNumber to this type set to correct the maybe-types of Bitwise operations.

> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv in DFG Int32 fast path even if Baseline constantly produces Double result
> 
> I suggest rephrasing as "making ArithDiv take the DFG Int32 fast path".
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        ResultType of bitwise operation needs including TypeMaybeNumber. TypeInt32 is something like a flag indicating the number looks like a int32.
> 
> I suggest rephrasing as "needs to include TypeMaybeNumber".

[Mark Lam]

(In reply to Yusuke Suzuki from comment #3)
> (In reply to Mark Lam from comment #2)
> > Comment on attachment 378452 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=378452&action=review
> > 
> > I think this is the wrong place to do this fix.  This method says
> > bigIntOrInt32Type().  It's misleading to have it also return
> > TypeMaybeNumber.  The real issue is in forBitOp(), which happens to be the
> > only client of this method.  I think we should just do the fix in forBitOp()
> > and remove bigIntOrInt32Type().  Alternatively, rename bigIntOrInt32Type()
> > to bigIntOrInt32TypeOrNumber() if you think there will be other clients of
> > it coing soon.
> 
> The name of flags in ResultType is misleading, but TypeInt32 and the other
> flags are not the same meaning. And TypeInt32 is not even in a type's set.
> We can see the definition of TypeBits does not include TypeInt32.
> 
> static constexpr Type TypeBits = TypeMaybeNumber | TypeMaybeString |
> TypeMaybeBigInt | TypeMaybeNull | TypeMaybeBool | TypeMaybeOther;
> 
> This is because TypeInt32 is a bit special boolean flag while other ones are
> entry of type set. So logically, ResultType is something like this.
> 
> struct ResultType {
>     bool m_looksInt32;
>     Set<Type> m_maybeTypes;
> };
> 
> We are using bit set just to encode ResultType in bytecode as an int.
> So, bigIntOrInt32Type definition right now is wrong since it is saying,
> 
> "It is definitely BigInt, and looks Int32".
> 
> This patch adds TypeMaybeNumber to this type set to correct the maybe-types
> of Bitwise operations.

So hokey.  Thanks for explaining it.  We should probably do some renaming here, or maybe rethink how to express these concepts.  Using an enum class would be improvement too for grepability.  But all that can be done later.  I'll r+ this patch.

[Mark Lam]

Comment on attachment 378452 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/ResultType.h:151
> +            return ResultType(TypeMaybeBigInt | TypeInt32 | TypeMaybeNumber);

Should we remove the TypeInt32 since it's not needed, and it's suggesting the wrong idea here?

[Yusuke Suzuki]

Comment on attachment 378452 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:3
>>> +        [JSC] ResultType implementation is wrong for bit ops, and ends up making ArithDiv in DFG Int32 fast path even if Baseline constantly produces Double result
>> 
>> I suggest rephrasing as "making ArithDiv take the DFG Int32 fast path".
> 
> 

Fixed.

>> Source/JavaScriptCore/ChangeLog:8
>> +        ResultType of bitwise operation needs including TypeMaybeNumber. TypeInt32 is something like a flag indicating the number looks like a int32.
> 
> I suggest rephrasing as "needs to include TypeMaybeNumber".

Fixed.

>> Source/JavaScriptCore/parser/ResultType.h:151
>> +            return ResultType(TypeMaybeBigInt | TypeInt32 | TypeMaybeNumber);
> 
> Should we remove the TypeInt32 since it's not needed, and it's suggesting the wrong idea here?

I think currently some code is using this TypeInt32 for preference of Int32 in Number representations.
I would like to consider this in a separate patch, and make this patch just a performance bug fix.
https://bugs.webkit.org/show_bug.cgi?id=201659 Filed.

[Yusuke Suzuki]

Committed r249736: <https://trac.webkit.org/changeset/249736>

[Mark Lam]

Comment on attachment 378452 [details]
Patch

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

>>> Source/JavaScriptCore/parser/ResultType.h:151
>>> +            return ResultType(TypeMaybeBigInt | TypeInt32 | TypeMaybeNumber);
>> 
>> Should we remove the TypeInt32 since it's not needed, and it's suggesting the wrong idea here?
> 
> I think currently some code is using this TypeInt32 for preference of Int32 in Number representations.
> I would like to consider this in a separate patch, and make this patch just a performance bug fix.
> https://bugs.webkit.org/show_bug.cgi?id=201659 Filed.

Sorry, I misread the encoding for TypeInt32.  It's 1.  I misread it as 0.  Yeah, I agree we can't remove it.

[Radar WebKit Bug Importer]

<rdar://problem/55238761>

[Radar WebKit Bug Importer]

<rdar://problem/55238760>