Bug 186229

Created attachment 350857 [details] Draft Patch This patch doesn't compile without patch available in https://bugs.webkit.org/show_bug.cgi?id=186228.

Created attachment 351245 [details] WIP - RFC Patch

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.

Created attachment 351248 [details] WIP - RFC Patch Hey @littledan, @dinfuehr, @caitp and @xan, could any you take a look, please?

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.

Created attachment 351249 [details] Benchmarks Based on this outcome, the change is perf neutral.

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.

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?

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?

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.

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.

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.

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.

Created attachment 351548 [details] Patch

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.

(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.

Comment on attachment 351548 [details] Patch Canceling the review because I’m investigating a regression into Microbenchmark/string-repeat-arith

Created attachment 351596 [details] Patch

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.

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.

Comment on attachment 351596 [details] Patch r=me

Comment on attachment 351596 [details] Patch Thank you very much for the review!

Comment on attachment 351596 [details] Patch Clearing flags on attachment: 351596 Committed r236901: <https://trac.webkit.org/changeset/236901>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45070445>