Bug 170961 - r211670 broke double to int conversion.
Summary: r211670 broke double to int conversion.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-18 12:12 PDT by Mark Lam
Modified: 2017-04-19 15:01 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (12.82 KB, patch)
2017-04-18 13:09 PDT, Mark Lam
utatane.tea: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.92 MB, application/zip)
2017-04-18 15:00 PDT, Build Bot
no flags Details
patch for landing. (14.23 KB, patch)
2017-04-18 15:06 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
Patch (8.01 KB, patch)
2017-04-19 01:32 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-04-18 12:12:45 PDT
This is because operationToInt32SensibleSlow() assumes that left shifts of greater than 31 bits on an 31-bit value will produce a 0.  However, the spec says that "if the value of the right operand is negative or is greater or equal to the number of bits in the promoted left operand, the behavior is undefined."  See http://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators.

This patch fixes this by restoring the check to prevent a shift of greater than 31 bits.
It also consolidates the optimization in operationToInt32SensibleSlow() back into toInt32() so that we don't have 2 copies of the same code with only a slight variation.
Comment 1 Radar WebKit Bug Importer 2017-04-18 12:13:41 PDT
<rdar://problem/31687696>
Comment 2 Yusuke Suzuki 2017-04-18 12:28:07 PDT
(In reply to Mark Lam from comment #0)
> This is because operationToInt32SensibleSlow() assumes that left shifts of
> greater than 31 bits on an 31-bit value will produce a 0.  However, the spec
> says that "if the value of the right operand is negative or is greater or
> equal to the number of bits in the promoted left operand, the behavior is
> undefined."  See
> http://en.cppreference.com/w/cpp/language/
> operator_arithmetic#Bitwise_shift_operators.

Oops! Right.

> 
> This patch fixes this by restoring the check to prevent a shift of greater
> than 31 bits.

Yup, I think adding the following check,

if (exp > 83)
    return 0;

will solve.

> It also consolidates the optimization in operationToInt32SensibleSlow() back
> into toInt32() so that we don't have 2 copies of the same code with only a
> slight variation.

Hm, my concern is that there is no way to emit cvttss2si_rr like thing in an architecture independent way (like, emitting this semantics operation in ARM64).
Comment 3 Mark Lam 2017-04-18 13:09:13 PDT
Created attachment 307406 [details]
proposed patch.
Comment 4 Yusuke Suzuki 2017-04-18 13:39:21 PDT
Comment on attachment 307406 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=307406&action=review

r=me if it does not regress DoubleAsInt32.

> Source/JavaScriptCore/runtime/MathCommon.h:93
> +    if (static_cast<uint32_t>(exp) > 83u)

Nice!
Comment 5 Build Bot 2017-04-18 15:00:22 PDT
Comment on attachment 307406 [details]
proposed patch.

Attachment 307406 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3559103

New failing tests:
http/tests/inspector/network/resource-sizes-network.html
Comment 6 Build Bot 2017-04-18 15:00:23 PDT
Created attachment 307421 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Mark Lam 2017-04-18 15:06:06 PDT
Created attachment 307423 [details]
patch for landing.

I added a microbenchmark to test that the r211670 progression is not negated.  It is not i.e. this patch does not appear to regress performance.  Will land shortly.
Comment 8 Mark Lam 2017-04-18 15:34:43 PDT
Thanks for the review.  Landed in r215482: <http://trac.webkit.org/r215482>.
Comment 9 Yusuke Suzuki 2017-04-19 00:31:08 PDT
(In reply to Mark Lam from comment #8)
> Thanks for the review.  Landed in r215482: <http://trac.webkit.org/r215482>.

Hmmm, it seems that it causes 5% regression in pbkdf2.
https://arewefastyet.com/#machine=29&view=single&suite=kraken&subtest=crypto-pbkdf2

I've just checked out r215482, and compared it with the r215482 + this patch's change version.

Benchmark report for Kraken on sakura-trick.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/int32sensible/Release/bin/jsc
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/int32sensible2/Release/bin/jsc

Collected 30 samples per benchmark/VM, with 30 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times
with 95% confidence intervals in milliseconds.

                                 baseline                  patched

stanford-crypto-pbkdf2       147.383+-4.238      !     161.362+-5.761         ! definitely 1.0948x slower

<arithmetic>                 147.383+-4.238      !     161.362+-5.761         ! definitely 1.0948x slower


So, I suggest sharing the implementation by using the template. I'll upload the patch.
Comment 10 Yusuke Suzuki 2017-04-19 01:32:17 PDT
Reopening to attach new patch.
Comment 11 Yusuke Suzuki 2017-04-19 01:32:18 PDT
Created attachment 307472 [details]
Patch
Comment 12 Mark Lam 2017-04-19 07:44:31 PDT
Comment on attachment 307472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307472&action=review

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:11
> +        and fixes kraken pbkdf2 regression.

Can you add "on linux" after "regression" to clarify where this can be observed?

> Source/JavaScriptCore/runtime/MathCommon.h:36
> +int32_t JIT_OPERATION operationToInt32SensibleSlow(double) WTF_INTERNAL;

You need to revert the DFG and FTL code back to using operationToInt32SensibleSlow() too.

> Source/JavaScriptCore/runtime/MathCommon.h:156
> +    return bits < 0 ? -static_cast<int32_t>(result) : static_cast<int32_t>(result);

Let's make bits uint64_t and only cast it to int64_t here instead for the sign check.  This will reduce the amount of casting in this function.  It also makes it consistent that we do all our bit manipulation on unsigned bits, and only re-apply the sign at the end.
Comment 13 Yusuke Suzuki 2017-04-19 09:08:01 PDT
Comment on attachment 307472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307472&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:11
>> +        and fixes kraken pbkdf2 regression.
> 
> Can you add "on linux" after "regression" to clarify where this can be observed?

Fixed.

>> Source/JavaScriptCore/runtime/MathCommon.h:36
>> +int32_t JIT_OPERATION operationToInt32SensibleSlow(double) WTF_INTERNAL;
> 
> You need to revert the DFG and FTL code back to using operationToInt32SensibleSlow() too.

Oops, right. I moved modified version of MathCommon.[h,cpp] from the old revision and forgot to bring the changes in FTL and DFG.

>> Source/JavaScriptCore/runtime/MathCommon.h:156
>> +    return bits < 0 ? -static_cast<int32_t>(result) : static_cast<int32_t>(result);
> 
> Let's make bits uint64_t and only cast it to int64_t here instead for the sign check.  This will reduce the amount of casting in this function.  It also makes it consistent that we do all our bit manipulation on unsigned bits, and only re-apply the sign at the end.

OK, fixed.
Comment 14 Yusuke Suzuki 2017-04-19 09:50:14 PDT
Committed r215516: <http://trac.webkit.org/changeset/215516>
Comment 15 Yusuke Suzuki 2017-04-19 15:01:55 PDT
OK, perf is recovered.
https://arewefastyet.com/#machine=29&view=single&suite=kraken&subtest=crypto-pbkdf2