RESOLVED FIXED 186229
[ESNext][BigInt] Implement support for "|"
https://bugs.webkit.org/show_bug.cgi?id=186229
Summary [ESNext][BigInt] Implement support for "|"
Caio Lima
Reported 2018-06-02 06:50:26 PDT
...
Attachments
Draft Patch (21.65 KB, patch)
2018-09-26 04:39 PDT, Caio Lima
no flags
WIP - RFC Patch (47.67 KB, patch)
2018-10-01 07:46 PDT, Caio Lima
no flags
WIP - RFC Patch (47.85 KB, patch)
2018-10-01 08:08 PDT, Caio Lima
no flags
Benchmarks (94.96 KB, text/plain)
2018-10-01 08:23 PDT, Caio Lima
no flags
Patch (53.15 KB, patch)
2018-10-03 14:53 PDT, Caio Lima
no flags
Patch (50.68 KB, patch)
2018-10-04 08:22 PDT, Caio Lima
no flags
Caio Lima
Comment 1 2018-09-26 04:39:45 PDT
Created attachment 350857 [details] Draft Patch This patch doesn't compile without patch available in https://bugs.webkit.org/show_bug.cgi?id=186228.
Caio Lima
Comment 2 2018-10-01 07:46:44 PDT
Created attachment 351245 [details] WIP - RFC Patch
EWS Watchlist
Comment 3 2018-10-01 07:48:38 PDT
Attachment 351245 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caio Lima
Comment 4 2018-10-01 08:08:28 PDT
Created attachment 351248 [details] WIP - RFC Patch Hey @littledan, @dinfuehr, @caitp and @xan, could any you take a look, please?
EWS Watchlist
Comment 5 2018-10-01 08:11:32 PDT
Attachment 351248 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caio Lima
Comment 6 2018-10-01 08:23:43 PDT
Created attachment 351249 [details] Benchmarks Based on this outcome, the change is perf neutral.
Caitlin Potter (:caitp)
Comment 7 2018-10-01 09:42:14 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736 > + if (opcodeID == op_bitand || opcodeID == op_bitor) { Doesn't it make sense to keep using ArithProfile? It seems like it should be much more compact, and provides more details for non-BigInt operations. We could probably add an extra bit to ObservedType to represent BigInts, which should allow them to be more useful for the BigInt case, too.
Caio Lima
Comment 8 2018-10-01 10:39:30 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736 >> + if (opcodeID == op_bitand || opcodeID == op_bitor) { > > Doesn't it make sense to keep using ArithProfile? It seems like it should be much more compact, and provides more details for non-BigInt operations. We could probably add an extra bit to ObservedType to represent BigInts, which should allow them to be more useful for the BigInt case, too. The use case of ValueProfile instead of ArithProfile is that these details are actually not important to bitwise operations right now. ArithProfile is used to check overflow, negative zero, operand types and other stuff, and bitwise operation doesn't have these behaviours. Actually, even though we emit ArithProfile for these operations, we are not even profiling it. With the introduction of BigInt we need to know the outcome type of the operation. In the case of "op_bitor" and "op_bitand", ValueProfile seems to do the job, since the only possible outcome is Int32 | BigInt. Does it make sense?
Caitlin Potter (:caitp)
Comment 9 2018-10-01 11:01:02 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736 >>> + if (opcodeID == op_bitand || opcodeID == op_bitor) { >> >> Doesn't it make sense to keep using ArithProfile? It seems like it should be much more compact, and provides more details for non-BigInt operations. We could probably add an extra bit to ObservedType to represent BigInts, which should allow them to be more useful for the BigInt case, too. > > The use case of ValueProfile instead of ArithProfile is that these details are actually not important to bitwise operations right now. ArithProfile is used to check overflow, negative zero, operand types and other stuff, and bitwise operation doesn't have these behaviours. Actually, even though we emit ArithProfile for these operations, we are not even profiling it. With the introduction of BigInt we need to know the outcome type of the operation. In the case of "op_bitor" and "op_bitand", ValueProfile seems to do the job, since the only possible outcome is Int32 | BigInt. > Does it make sense? I understand what ValueProfile does, but I think we can get the same from ArithProfile with some slight adjustments to ObservedType, and it should be more compact. So it sounds like you don't want to use it just because you don't want to record things like -0 or overflow, even if the ObservedType portion is the stuff we actually care about?
Caio Lima
Comment 10 2018-10-01 11:50:03 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review >>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736 >>>> + if (opcodeID == op_bitand || opcodeID == op_bitor) { >>> >>> Doesn't it make sense to keep using ArithProfile? It seems like it should be much more compact, and provides more details for non-BigInt operations. We could probably add an extra bit to ObservedType to represent BigInts, which should allow them to be more useful for the BigInt case, too. >> >> The use case of ValueProfile instead of ArithProfile is that these details are actually not important to bitwise operations right now. ArithProfile is used to check overflow, negative zero, operand types and other stuff, and bitwise operation doesn't have these behaviours. Actually, even though we emit ArithProfile for these operations, we are not even profiling it. With the introduction of BigInt we need to know the outcome type of the operation. In the case of "op_bitor" and "op_bitand", ValueProfile seems to do the job, since the only possible outcome is Int32 | BigInt. >> Does it make sense? > > I understand what ValueProfile does, but I think we can get the same from ArithProfile with some slight adjustments to ObservedType, and it should be more compact. So it sounds like you don't want to use it just because you don't want to record things like -0 or overflow, even if the ObservedType portion is the stuff we actually care about? Right now, I think makes more sense using ValueProfile in this case for 2 reasons: 1. op_bitand and op_bitor can't have -0 or overflow as outcome, AFIK. In such case, even if we record such things, it is not going to happen. 2. We don't need to change ArithProfile infrastructure to handle bitwise operations, since we already have ValueProfile to do what we want. Maybe we will need to adjust ObservedType to handle BigInt cases in other operations like "+", "-" or "*", but for now, I think using NonNumber would be sufficient to implement simple BigInt specialisation for those operations. We could also use ArithProfile and NonNumber observation for op_bit(and|or) as well, just like op_add does when deciding to specialise for Number or String, and no changes are necessary. However this sounds like misuse of ArithProfile, IMHO. In this case, I think ValueProfile fits better for this case, but ArithProfile is more memory efficient. Anyway, I can change to ArithProfile if you still think it is better.
Caitlin Potter (:caitp)
Comment 11 2018-10-01 11:52:43 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review >>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736 >>>>> + if (opcodeID == op_bitand || opcodeID == op_bitor) { >>>> >>>> Doesn't it make sense to keep using ArithProfile? It seems like it should be much more compact, and provides more details for non-BigInt operations. We could probably add an extra bit to ObservedType to represent BigInts, which should allow them to be more useful for the BigInt case, too. >>> >>> The use case of ValueProfile instead of ArithProfile is that these details are actually not important to bitwise operations right now. ArithProfile is used to check overflow, negative zero, operand types and other stuff, and bitwise operation doesn't have these behaviours. Actually, even though we emit ArithProfile for these operations, we are not even profiling it. With the introduction of BigInt we need to know the outcome type of the operation. In the case of "op_bitor" and "op_bitand", ValueProfile seems to do the job, since the only possible outcome is Int32 | BigInt. >>> Does it make sense? >> >> I understand what ValueProfile does, but I think we can get the same from ArithProfile with some slight adjustments to ObservedType, and it should be more compact. So it sounds like you don't want to use it just because you don't want to record things like -0 or overflow, even if the ObservedType portion is the stuff we actually care about? > > Right now, I think makes more sense using ValueProfile in this case for 2 reasons: > > 1. op_bitand and op_bitor can't have -0 or overflow as outcome, AFIK. In such case, even if we record such things, it is not going to happen. > 2. We don't need to change ArithProfile infrastructure to handle bitwise operations, since we already have ValueProfile to do what we want. > > Maybe we will need to adjust ObservedType to handle BigInt cases in other operations like "+", "-" or "*", but for now, I think using NonNumber would be sufficient to implement simple BigInt specialisation for those operations. > We could also use ArithProfile and NonNumber observation for op_bit(and|or) as well, just like op_add does when deciding to specialise for Number or String, and no changes are necessary. However this sounds like misuse of ArithProfile, IMHO. > In this case, I think ValueProfile fits better for this case, but ArithProfile is more memory efficient. > Anyway, I can change to ArithProfile if you still think it is better. It's OK with me to keep it using ValueProfile for now, don't worry about it.
Yusuke Suzuki
Comment 12 2018-10-02 06:18:18 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:129 > + CRASH(); DFG_CRASH is preferable. > Source/JavaScriptCore/dfg/DFGOperations.cpp:341 > +inline EncodedJSValue bitwiseOp(ExecState* exec, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2, std::function<JSBigInt*(JSBigInt*, JSBigInt*)> bigIntOp, std::function<int32_t(int32_t, int32_t)> int32Op) Use `template<typename BigIntOperation>` and `BigIntOperation&& bigIntOp` instead of `std::function<>`. std::function could cause unnecessary heap allocation. And even if std::function<> is required (to store it somewhere), use WTF::Function<> instead. => do not use std::function<> at any time :P > Source/JavaScriptCore/dfg/DFGOperations.cpp:371 > + auto bigIntOp = [vm] (JSBigInt* left, JSBigInt* right) -> JSBigInt* { Instead of capturing VM here, taking `VM& vm` parameter and passing VM in the caller function (bitwiseOp) is cleaner. > Source/JavaScriptCore/dfg/DFGOperations.cpp:385 > + auto bigIntOp = [vm] (JSBigInt* left, JSBigInt* right) -> JSBigInt* { Ditto. > Source/JavaScriptCore/runtime/JSBigInt.cpp:390 > + return absoluteAddOne(vm, result, SignOption::Signed); Nice.
Caio Lima
Comment 13 2018-10-03 06:45:43 PDT
Comment on attachment 351248 [details] WIP - RFC Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351248&action=review Thank you very much for the review and comments @Yusuke and @Caitlin >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:129 >> + CRASH(); > > DFG_CRASH is preferable. Done. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:341 >> +inline EncodedJSValue bitwiseOp(ExecState* exec, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2, std::function<JSBigInt*(JSBigInt*, JSBigInt*)> bigIntOp, std::function<int32_t(int32_t, int32_t)> int32Op) > > Use `template<typename BigIntOperation>` and `BigIntOperation&& bigIntOp` instead of `std::function<>`. > std::function could cause unnecessary heap allocation. > And even if std::function<> is required (to store it somewhere), use WTF::Function<> instead. => do not use std::function<> at any time :P Thx for letting me know that. I'm also going to remove std::function usage into JSBigInt. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:371 >> + auto bigIntOp = [vm] (JSBigInt* left, JSBigInt* right) -> JSBigInt* { > > Instead of capturing VM here, taking `VM& vm` parameter and passing VM in the caller function (bitwiseOp) is cleaner. Done. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:385 >> + auto bigIntOp = [vm] (JSBigInt* left, JSBigInt* right) -> JSBigInt* { > > Ditto. Done.
Caio Lima
Comment 14 2018-10-03 14:53:15 PDT
EWS Watchlist
Comment 15 2018-10-03 14:56:14 PDT
Attachment 351548 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Caio Lima
Comment 16 2018-10-03 14:56:48 PDT
(In reply to Caio Lima from comment #14) > Created attachment 351548 [details] > Patch I'm asking for review before committing because of 2 reasons: 1. I changed AbstractInterpreter to setNodeType(SpecBigInt) when "node->binaryUseKind() == BigIntUse". This way we are able to remove redundant checks when specialising operations for BigInt. 2. I changed JSBigInt::absoluteBitwiseOp to use template instead of std::function.
Caio Lima
Comment 17 2018-10-03 21:36:12 PDT
Comment on attachment 351548 [details] Patch Canceling the review because I’m investigating a regression into Microbenchmark/string-repeat-arith
Caio Lima
Comment 18 2018-10-04 08:22:51 PDT
Caio Lima
Comment 19 2018-10-04 08:25:39 PDT
Changes from previous version: 1. Removed the usage of template bitwiseOp, because it was causing a regression of 50% into Microbenchmarks/string-repeat-arith.js. Uploading Benchmark results soon.
EWS Watchlist
Comment 20 2018-10-04 09:39:41 PDT
Attachment 351596 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1750: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yusuke Suzuki
Comment 21 2018-10-06 15:25:41 PDT
Comment on attachment 351596 [details] Patch r=me
Caio Lima
Comment 22 2018-10-06 16:54:18 PDT
Comment on attachment 351596 [details] Patch Thank you very much for the review!
WebKit Commit Bot
Comment 23 2018-10-06 17:21:07 PDT
Comment on attachment 351596 [details] Patch Clearing flags on attachment: 351596 Committed r236901: <https://trac.webkit.org/changeset/236901>
WebKit Commit Bot
Comment 24 2018-10-06 17:21:09 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 25 2018-10-06 17:22:29 PDT
Note You need to log in before you can comment on or make changes to this bug.