Bug 174352 – [FTL] Arguments elimination is suppressed by unreachable blocks

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 315240 [details]
Patch

WIP

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 315410 [details]
Patch

[Saam Barati]

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.

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

Created attachment 315546 [details]
Patch

[Yusuke Suzuki]

Created attachment 315547 [details]
Patch

[Filip Pizlo]

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?

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 315598 [details]
Patch for landing after stabilizing is done

Patch for landing after stabilizing is done

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

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?

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 315619 [details]
Patch for landing after stabilizing is done

Patch for landing

[Yusuke Suzuki]

Committed r219727: <http://trac.webkit.org/changeset/219727>