Bug 186229 – [ESNext][BigInt] Implement support for "|"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Caio Lima]

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]

Created attachment 351245 [details]
WIP - RFC Patch

[EWS Watchlist]

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]

Created attachment 351248 [details]
WIP - RFC Patch

Hey @littledan, @dinfuehr, @caitp and @xan, could any you take a look, please?

[EWS Watchlist]

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]

Created attachment 351249 [details]
Benchmarks

Based on this outcome, the change is perf neutral.

[Caitlin Potter (:caitp)]

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 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 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 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 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 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 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]

Created attachment 351548 [details]
Patch

[EWS Watchlist]

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]

(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 on attachment 351548 [details]
Patch

Canceling the review because I’m investigating a regression into Microbenchmark/string-repeat-arith

[Caio Lima]

Created attachment 351596 [details]
Patch

[Caio Lima]

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]

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 on attachment 351596 [details]
Patch

r=me

[Caio Lima]

Comment on attachment 351596 [details]
Patch

Thank you very much for the review!

[WebKit Commit Bot]

Comment on attachment 351596 [details]
Patch

Clearing flags on attachment: 351596

Committed r236901: <https://trac.webkit.org/changeset/236901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45070445>