Bug 202711 - Post increment/decrement should only call ToNumber once
Summary: Post increment/decrement should only call ToNumber once
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
Keywords: InRadar
: 202710 (view as bug list)
Depends on:
Reported: 2019-10-08 16:05 PDT by Robin Morisset
Modified: 2019-10-21 12:12 PDT (History)
8 users (show)

See Also:

Patch (3.27 KB, patch)
2019-10-09 10:51 PDT, Robin Morisset
mark.lam: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.65 MB, application/zip)
2019-10-10 00:33 PDT, EWS Watchlist
no flags Details
Patch (3.54 KB, patch)
2019-10-14 18:32 PDT, Robin Morisset
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2019-10-15 11:12 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-10-08 16:05:34 PDT
Currently they call it twice, see the following test case:
var o = {};
var counter = 0;
o.valueOf = () => {counter ++; return 42};
if (counter != 1)
    throw "valueOf was executed " + counter + " times during a post-increment instead of once!";

As far as I can tell this is not spec-compliant:
12.4.4 Postfix Increment Operator requires a single call to ToNumeric.
Then there is a single chain of ToNumeric -> ToNumber -> ToPrimitive -> OrdinaryToPrimitive -> valueOf with no reason for the call to be duplicated anywhere.

The problem appears to be from:
static RegisterID* emitPostIncOrDec(BytecodeGenerator& generator, RegisterID* dst, RegisterID* srcDst, Operator oper)
    if (dst == srcDst)
        return generator.emitToNumber(generator.finalDestination(dst), srcDst);
    RefPtr<RegisterID> tmp = generator.emitToNumber(generator.tempDestination(dst), srcDst);
    emitIncOrDec(generator, srcDst, oper);
    return generator.move(dst, tmp.get());
which uses an emitToNumber, but then does an emitIncOrDec on the original value, which itself can lead to a slow path that does ToNumber anew.
Comment 1 Robin Morisset 2019-10-08 16:49:09 PDT
Things are a bit more subtle than I expected: wrapping the testcase above in a function and calling that function suppresses the bug!
It comes back if "o++" is replaced by "var x = o++" so I suspect it is caused by the code:
RegisterID* PostfixNode::emitResolve(BytecodeGenerator& generator, RegisterID* dst)
    if (dst == generator.ignoredResult())
        return PrefixNode::emitResolve(generator, dst);
    RefPtr<RegisterID> oldValue = emitPostIncOrDec(generator, generator.finalDestination(dst), value.get(), m_operator);
also in NodesCodegen.cpp. It visibly replaces "o++;" by "++o;" when the result is not stored anywhere.

The mystery is why it only occurs when wrapped in a function, and not at the top-level.
Comment 2 Robin Morisset 2019-10-09 10:51:35 PDT
Created attachment 380545 [details]
Comment 3 Mark Lam 2019-10-09 10:58:37 PDT
Comment on attachment 380545 [details]

Comment 4 EWS Watchlist 2019-10-10 00:33:17 PDT
Comment on attachment 380545 [details]

Attachment 380545 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13113521

Number of test failures exceeded the failure limit.
Comment 5 EWS Watchlist 2019-10-10 00:33:19 PDT
Created attachment 380608 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 6 Keith Miller 2019-10-10 17:47:00 PDT
Seems like this patch has broken jsc EWS... :(
Comment 7 Alexey Proskuryakov 2019-10-11 00:04:02 PDT
*** Bug 202710 has been marked as a duplicate of this bug. ***
Comment 8 Robin Morisset 2019-10-14 18:32:03 PDT
Created attachment 380943 [details]
Comment 9 EWS Watchlist 2019-10-14 20:48:28 PDT
Comment on attachment 380943 [details]

Attachment 380943 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13131267

New failing tests:
Comment 10 Robin Morisset 2019-10-15 11:12:36 PDT
Created attachment 381002 [details]

Same patch as before, to check that the previous test failure was a flake.
Comment 11 Saam Barati 2019-10-15 11:21:39 PDT
Comment on attachment 381002 [details]

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


> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1791
> +    RefPtr<RegisterID> tmp = generator.emitToNumber(generator.newTemporary(), srcDst);
> +    RefPtr<RegisterID> result = generator.tempDestination(srcDst);
> +    generator.move(result.get(), tmp.get());
> +    emitIncOrDec(generator, result.get(), oper);
> +    generator.move(srcDst, result.get());

why not something like this?
RefPtr<RegisterID> result = generator.emitToNumber(generator.tempDestination(dst), srcDst);
emitIncOrDec(generator, result.get(), oper);
generator.move(srcDst, result.get());
return generator.move(dst, result.get());
Comment 12 Robin Morisset 2019-10-21 11:11:18 PDT
Comment on attachment 381002 [details]

The jsc-mips bot appears stuck, but since it does not show an actual test failure and all other bots are green, I'm trying to land this.
Comment 13 WebKit Commit Bot 2019-10-21 12:06:55 PDT
Comment on attachment 381002 [details]

Clearing flags on attachment: 381002

Committed r251371: <https://trac.webkit.org/changeset/251371>
Comment 14 WebKit Commit Bot 2019-10-21 12:06:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-10-21 12:12:37 PDT