198253 2019-05-25 20:30:02 -0700 [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 2019-09-10 15:08:18 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1539238 0 ysuzuki 2019-05-25 20:30:02 -0700 It causes unnecessary OSRExit in async-fs Math.random code. 1569488 1 378452 ysuzuki 2019-09-10 03:30:57 -0700 Created attachment 378452 Patch 1569524 2 378452 mark.lam 2019-09-10 07:04:31 -0700 Comment on attachment 378452 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". 1569586 3 ysuzuki 2019-09-10 10:40:04 -0700 (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". 1569680 4 mark.lam 2019-09-10 14:53:03 -0700 (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. 1569681 5 378452 mark.lam 2019-09-10 14:53:55 -0700 Comment on attachment 378452 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? 1569686 6 378452 ysuzuki 2019-09-10 15:04:08 -0700 Comment on attachment 378452 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. 1569687 7 ysuzuki 2019-09-10 15:07:17 -0700 Committed r249736: <https://trac.webkit.org/changeset/249736> 1569688 8 378452 mark.lam 2019-09-10 15:07:33 -0700 Comment on attachment 378452 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. 1569689 9 webkit-bug-importer 2019-09-10 15:08:18 -0700 <rdar://problem/55238761> 1569690 10 webkit-bug-importer 2019-09-10 15:08:18 -0700 <rdar://problem/55238760> 378452 2019-09-10 03:30:57 -0700 2019-09-10 14:53:55 -0700 Patch bug-198253-20190910033056.patch text/plain 1963 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ5NzA2CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBk YjRhM2EwMDNmMDY2YWY1MGE4N2RhMjdlMWFjMmVmM2I2M2Y5NDgxLi4zZmVjZDY5MWRiNDc0MTU0 MmFjY2QxZWY4ZWY5MDNmZjdhY2I3NDU0IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOSBAQAorMjAxOS0wOS0xMCAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIFtKU0NdIFJlc3VsdFR5cGUgaW1wbGVtZW50YXRpb24gaXMgd3JvbmcgZm9y IGJpdCBvcHMsIGFuZCBlbmRzIHVwIG1ha2luZyBBcml0aERpdiBpbiBERkcgSW50MzIgZmFzdCBw YXRoIGV2ZW4gaWYgQmFzZWxpbmUgY29uc3RhbnRseSBwcm9kdWNlcyBEb3VibGUgcmVzdWx0Cisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTgyNTMKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZXN1bHRUeXBl IG9mIGJpdHdpc2Ugb3BlcmF0aW9uIG5lZWRzIGluY2x1ZGluZyBUeXBlTWF5YmVOdW1iZXIuIFR5 cGVJbnQzMiBpcyBzb21ldGhpbmcgbGlrZSBhIGZsYWcgaW5kaWNhdGluZyB0aGUgbnVtYmVyIGxv b2tzIGxpa2UgYSBpbnQzMi4KKyAgICAgICAgV2hlbiBpdCBpcyBzcGVjaWZpZWQsIFR5cGVNYXli ZU51bWJlciBtdXN0IGV4aXN0IHRvby4gVGhpcyBpc3N1ZSBjb21waWxlcyBvcF9kaXYgaW4gSmV0 U3RyZWFtMi9hc3luYy1mcyBzbG93LXBhdGguIEFuZCBldmVudHVhbGx5IERGRyBmaXJzdCBtaXMt Y29tcGlsZXMKKyAgICAgICAgaXQgd2l0aCBJbnQzMiBBcml0aERpdiB3aGlsZSB0aGF0IGRpdiBh bHdheXMgcHJvZHVjZXMgZG91YmxlLiBBbmQgdW5uZWNlc3NhcnkgT1NSIGV4aXQgaGFwcGVucy4K KworICAgICAgICBJbiB0aGlzIHBhdGNoLCB3ZSBhZGQgVHlwZU1heWJlTnVtYmVyIHRvIGJpZ0lu dE9ySW50MzJUeXBlIGNvcnJlY3RseS4KKworICAgICAgICAqIHBhcnNlci9SZXN1bHRUeXBlLmg6 CisgICAgICAgIChKU0M6OlJlc3VsdFR5cGU6OmJpZ0ludE9ySW50MzJUeXBlKToKKwogMjAxOS0w OS0wOSAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29tPgogCiAgICAgICAgIFtKU0Nd IENvZGVCbG9jazo6bV9jb25zdGFudFJlZ2lzdGVycyBzaG91bGQgYmUgZ3VhcmRlZCBieSBDb25j dXJyZW50SlNMb2NrIHdoZW4gVmVjdG9yIHJlYWxsb2NhdGUgbWVtb3J5CmRpZmYgLS1naXQgYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvcGFyc2VyL1Jlc3VsdFR5cGUuaCBiL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9wYXJzZXIvUmVzdWx0VHlwZS5oCmluZGV4IGY1ZmFkZGU2ZjY2MjRlNzYwZjQ5YzVl YWNjZjE0OTYwMDQ0MGUwMTEuLjNkNDY4ZWM5MTliNWI2NDAzMGNlNmU2N2IyNTVmMGMxYTc1ZDBl ZTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9wYXJzZXIvUmVzdWx0VHlwZS5o CisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9wYXJzZXIvUmVzdWx0VHlwZS5oCkBAIC0xNDgs NyArMTQ4LDcgQEAgbmFtZXNwYWNlIEpTQyB7CiAgICAgICAgIAogICAgICAgICBzdGF0aWMgY29u c3RleHByIFJlc3VsdFR5cGUgYmlnSW50T3JJbnQzMlR5cGUoKQogICAgICAgICB7Ci0gICAgICAg ICAgICByZXR1cm4gUmVzdWx0VHlwZShUeXBlTWF5YmVCaWdJbnQgfCBUeXBlSW50MzIpOworICAg ICAgICAgICAgcmV0dXJuIFJlc3VsdFR5cGUoVHlwZU1heWJlQmlnSW50IHwgVHlwZUludDMyIHwg VHlwZU1heWJlTnVtYmVyKTsKICAgICAgICAgfQogCiAgICAgICAgIHN0YXRpYyBjb25zdGV4cHIg UmVzdWx0VHlwZSB1bmtub3duVHlwZSgpCg==