Bug 198253

Summary: [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
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mark.lam: review+

Yusuke Suzuki
Reported 2019-05-25 20:30:02 PDT
It causes unnecessary OSRExit in async-fs Math.random code.
Attachments
Patch (1.92 KB, patch)
2019-09-10 03:30 PDT, Yusuke Suzuki
mark.lam: review+
Yusuke Suzuki
Comment 1 2019-09-10 03:30:57 PDT
Mark Lam
Comment 2 2019-09-10 07:04:31 PDT
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
Comment 3 2019-09-10 10:40:04 PDT
(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
Comment 4 2019-09-10 14:53:03 PDT
(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 5 2019-09-10 14:53:55 PDT
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 6 2019-09-10 15:04:08 PDT
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
Comment 7 2019-09-10 15:07:17 PDT
Mark Lam
Comment 8 2019-09-10 15:07:33 PDT
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
Comment 9 2019-09-10 15:08:18 PDT
Radar WebKit Bug Importer
Comment 10 2019-09-10 15:08:18 PDT
Note You need to log in before you can comment on or make changes to this bug.