Bug 174352

Summary: [FTL] Arguments elimination is suppressed by unreachable blocks
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, fpizlo, keith_miller, mark.lam, msaboff, rniwa, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Patch
none
Patch
fpizlo: review+, buildbot: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch for landing after stabilizing is done
buildbot: commit-queue-
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch for landing after stabilizing is done none

Description Yusuke Suzuki 2017-07-11 03:30:42 PDT
I can see the case like this,


1: CreateClonedArguments

...

10: ForceOSRExit
11: GetById(@1, "length")

In that case, even if 11 is not reachable from FTL code, our arguments elimination phase is suppressed by this, because our DFG constant folding's DFG node clipping is not executed yet.
Comment 1 Yusuke Suzuki 2017-07-11 03:32:25 PDT
Let's see this effect with mathjs

https://github.com/josdejong/mathjs

```
function runMathjs(n)  {
    const a = math.zeros(n, n)
    const b = math.zeros(n, n)
    for (let i = 0; i < n; ++i) {
        for (let j = 0; j < n; ++j) {
            a.set([i, j], Math.random())
            b.set([i, j], Math.random())
        }
    }

    for (var i = 0; i < 30; ++i) {
        math.multiply(a, b)
    }
}
runMathjs(100);
```

This is super slow in JSC because arguments elimination is suppressed in add#AebcRZ.
Comment 2 Yusuke Suzuki 2017-07-12 08:02:09 PDT
Created attachment 315240 [details]
Patch

WIP
Comment 3 Build Bot 2017-07-12 16:06:31 PDT
Comment on attachment 315240 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html
Comment 4 Build Bot 2017-07-12 16:06:32 PDT
Created attachment 315298 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Yusuke Suzuki 2017-07-14 01:09:00 PDT
Created attachment 315410 [details]
Patch
Comment 6 Saam Barati 2017-07-14 11:10:28 PDT
Comment on attachment 315410 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:414
> +                case GetById:
> +                    // If we already perform OSR-exit, GetById can be simply ignored.
> +                    // This can happen if you have a basic block that is not executed yet and references elimination candidates.
> +                    // Since this GetById code is not executed yet, it is always GetById (neither GetByOffset nor GetByIdFlush).
> +                    // We do not need to consider GetByIdFlush here.
> +                    // We focus on ForceOSRExit. It is OK because explicit OSR exit nodes like Unreachable, Throw etc. are terminal
> +                    // nodes. So it does not appear in the middle of a basic block.
> +                    if (isOSRExited)
> +                        break;
> +                    m_graph.doToChildren(node, [&] (Edge edge) { return escape(edge, node); });
> +                    break;

We can't just special case GetById. There are plenty of other operations that might escape an allocation.

Regarding your boolean here, why not just break out of walking over this block when seeing a node that is an exit.
I would just add something like:
case ForceOSRExit:
case CheckBadCell:
case Throw:
case ThrowStaticError:
    break out of for loop.

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:1118
> +                isOSRExited |= node->isPseudoTerminal();

and then I'd just add the same cases to this switch statement, and break out early.

> Source/JavaScriptCore/dfg/DFGNode.h:1313
> +    // As is described in DFGNodeType.h's ForceOSRExit, this is a pseudo-terminal.
> +    // It means that execution should fall out of DFG at this point, but execution
> +    // does continue in the basic block - just in a different compiler.
> +    bool isPseudoTerminal()

I wouldn't add this function. This is nicer just in the arguments elimination phase as part of its switch statements.

> Source/JavaScriptCore/dfg/DFGValidate.cpp:737
> +                case GetById:
> +                    if (isOSRExited)
> +                        break;
> +                    FALLTHROUGH;

ditto here.
Comment 7 Yusuke Suzuki 2017-07-15 08:52:02 PDT
Comment on attachment 315410 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:414
>> +                    break;
> 
> We can't just special case GetById. There are plenty of other operations that might escape an allocation.
> 
> Regarding your boolean here, why not just break out of walking over this block when seeing a node that is an exit.
> I would just add something like:
> case ForceOSRExit:
> case CheckBadCell:
> case Throw:
> case ThrowStaticError:
>     break out of for loop.

I'm a bit worried about that XXXNode(PhantomClonedArguments) node appears in lowering phase.
But since we cannot hoist nodes over ForceOSRExit, the above thing cannot happen.
I'll change this to

if (node->isPseudoTerminal())
    break;

since pseudo terminal nodes may escape (currently there is no such nodes).

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:1118
>> +                isOSRExited |= node->isPseudoTerminal();
> 
> and then I'd just add the same cases to this switch statement, and break out early.

Changed this to

if (node->isPseudoTerminal())
    break;

>> Source/JavaScriptCore/dfg/DFGNode.h:1313
>> +    bool isPseudoTerminal()
> 
> I wouldn't add this function. This is nicer just in the arguments elimination phase as part of its switch statements.

I think this is useful since we use this too in DFGValidate. What do you think of?
Comment 8 Yusuke Suzuki 2017-07-15 08:52:52 PDT
Created attachment 315546 [details]
Patch
Comment 9 Yusuke Suzuki 2017-07-15 09:00:43 PDT
Created attachment 315547 [details]
Patch
Comment 10 Filip Pizlo 2017-07-15 09:41:17 PDT
Comment on attachment 315547 [details]
Patch

Can you add a fixme above the declaration of isPseudoTerminal that says something like that we should consider replacing all users of this with AI-based reachablity?
Comment 11 Build Bot 2017-07-15 13:03:30 PDT
Comment on attachment 315547 [details]
Patch

Attachment 315547 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4126762

New failing tests:
http/tests/canvas/canvas-tainted-after-draw-image.html
Comment 12 Build Bot 2017-07-15 13:03:32 PDT
Created attachment 315564 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 13 Yusuke Suzuki 2017-07-16 02:24:11 PDT
(In reply to Filip Pizlo from comment #10)
> Comment on attachment 315547 [details]
> Patch
> 
> Can you add a fixme above the declaration of isPseudoTerminal that says
> something like that we should consider replacing all users of this with
> AI-based reachablity?

Thanks, added.
Comment 14 Yusuke Suzuki 2017-07-16 02:25:20 PDT
Created attachment 315598 [details]
Patch for landing after stabilizing is done

Patch for landing after stabilizing is done
Comment 15 Build Bot 2017-07-16 03:54:04 PDT
Comment on attachment 315598 [details]
Patch for landing after stabilizing is done

Attachment 315598 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4129991

New failing tests:
http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html
Comment 16 Build Bot 2017-07-16 03:54:22 PDT
Created attachment 315600 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 17 Build Bot 2017-07-16 03:59:04 PDT
Comment on attachment 315598 [details]
Patch for landing after stabilizing is done

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

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
Comment 18 Build Bot 2017-07-16 03:59:05 PDT
Created attachment 315601 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-07-16 04:41:38 PDT
Comment on attachment 315598 [details]
Patch for landing after stabilizing is done

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

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
Comment 20 Build Bot 2017-07-16 04:41:39 PDT
Created attachment 315602 [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 21 Saam Barati 2017-07-16 11:49:45 PDT
Comment on attachment 315598 [details]
Patch for landing after stabilizing is done

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:412
> +                if (node->isPseudoTerminal())

Can you add a test where we “throw arguments” just to ensure that this break is in the correct place?
Comment 22 Yusuke Suzuki 2017-07-16 12:47:29 PDT
Comment on attachment 315598 [details]
Patch for landing after stabilizing is done

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

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:412
>> +                if (node->isPseudoTerminal())
> 
> Can you add a test where we “throw arguments” just to ensure that this break is in the correct place?

Sure!
Comment 23 Yusuke Suzuki 2017-07-16 12:54:53 PDT
Created attachment 315619 [details]
Patch for landing after stabilizing is done

Patch for landing
Comment 24 Yusuke Suzuki 2017-07-21 09:01:12 PDT
Committed r219727: <http://trac.webkit.org/changeset/219727>