Bug 186229 - [ESNext][BigInt] Implement support for "|"
Summary: [ESNext][BigInt] Implement support for "|"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: InRadar
Depends on: 186228
Blocks: 179001
  Show dependency treegraph
 
Reported: 2018-06-02 06:50 PDT by Caio Lima
Modified: 2018-10-06 17:22 PDT (History)
15 users (show)

See Also:


Attachments
Draft Patch (21.65 KB, patch)
2018-09-26 04:39 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - RFC Patch (47.67 KB, patch)
2018-10-01 07:46 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - RFC Patch (47.85 KB, patch)
2018-10-01 08:08 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks (94.96 KB, text/plain)
2018-10-01 08:23 PDT, Caio Lima
no flags Details
Patch (53.15 KB, patch)
2018-10-03 14:53 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (50.68 KB, patch)
2018-10-04 08:22 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-06-02 06:50:26 PDT
...
Comment 1 Caio Lima 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.
Comment 2 Caio Lima 2018-10-01 07:46:44 PDT
Created attachment 351245 [details]
WIP - RFC Patch
Comment 3 Build Bot 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.
Comment 4 Caio Lima 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?
Comment 5 Build Bot 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.
Comment 6 Caio Lima 2018-10-01 08:23:43 PDT
Created attachment 351249 [details]
Benchmarks

Based on this outcome, the change is perf neutral.
Comment 7 Caitlin Potter (:caitp) 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.
Comment 8 Caio Lima 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?
Comment 9 Caitlin Potter (:caitp) 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?
Comment 10 Caio Lima 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.
Comment 11 Caitlin Potter (:caitp) 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.
Comment 12 Yusuke Suzuki 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.
Comment 13 Caio Lima 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.
Comment 14 Caio Lima 2018-10-03 14:53:15 PDT
Created attachment 351548 [details]
Patch
Comment 15 Build Bot 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.
Comment 16 Caio Lima 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.
Comment 17 Caio Lima 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
Comment 18 Caio Lima 2018-10-04 08:22:51 PDT
Created attachment 351596 [details]
Patch
Comment 19 Caio Lima 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.
Comment 20 Build Bot 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.
Comment 21 Yusuke Suzuki 2018-10-06 15:25:41 PDT
Comment on attachment 351596 [details]
Patch

r=me
Comment 22 Caio Lima 2018-10-06 16:54:18 PDT
Comment on attachment 351596 [details]
Patch

Thank you very much for the review!
Comment 23 WebKit Commit Bot 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>
Comment 24 WebKit Commit Bot 2018-10-06 17:21:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2018-10-06 17:22:29 PDT
<rdar://problem/45070445>