Bug 175470 – Support the with keyword in DFG

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Robin Morisset]

Support the which keyword in DFG

[Robin Morisset]

Created attachment 317932 [details]
Patch

[Build Bot]

Attachment 317932 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5433:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 317932 [details]
Patch

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

New failing tests:
stress/eval-func-decl-in-eval-within-with-scope.js.default
stress/get-from-scope-dynamic-onto-proxy.js.dfg-eager
stress/global-lexical-variable-with-statement.js.dfg-eager-no-cjit-validate
stress/eval-func-decl-in-eval-within-with-scope.js.dfg-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit-b3o1
mozilla-tests.yaml/ecma/ExecutionContexts/10.2.2-2.js.mozilla-ftl-eager-no-cjit-validate-phases
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-small-pool
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-no-inline-validate
stress/lexical-let-and-with-statement.js.ftl-eager-no-cjit-b3o1
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.dfg-maximal-flush-validate-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/eval-and-with.js.layout-dfg-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-validate-sampling-profiler
stress/eval-func-decl-in-eval-within-with-scope.js.no-llint
mozilla-tests.yaml/ecma/ExecutionContexts/10.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases
stress/constant-closure-var-with-dynamic-invalidation.js.dfg-maximal-flush-validate-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-no-put-stack-validate
stress/get-from-scope-dynamic-onto-proxy.js.dfg-eager-no-cjit-validate
stress/constant-closure-var-with-dynamic-invalidation.js.no-cjit-validate-phases
stress/global-lexical-variable-with-statement.js.ftl-eager-no-cjit
stress/const-and-with-statement.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.no-ftl
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit
stress/const-and-with-statement.js.ftl-eager-no-cjit
stress/constant-closure-var-with-dynamic-invalidation.js.ftl-no-cjit-validate-sampling-profiler
stress/eval-func-decl-in-eval-within-with-scope.js.no-cjit-collect-continuously
stress/global-lexical-variable-with-statement.js.dfg-eager
stress/global-lexical-variable-with-statement.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-with-scope.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/eval-and-with.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.dfg-eager-no-cjit-validate
stress/const-and-with-statement.js.dfg-eager-no-cjit-validate
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager-no-cjit-b3o1
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager
stress/global-lexical-variable-with-statement.js.ftl-eager
stress/constant-closure-var-with-dynamic-invalidation.js.dfg-eager-no-cjit-validate
stress/lexical-let-and-with-statement.js.dfg-eager-no-cjit-validate
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-no-cjit-no-inline-validate
stress/get-from-scope-dynamic-onto-proxy.js.ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager
stress/lexical-let-and-with-statement.js.ftl-eager-no-cjit

[Robin Morisset]

Created attachment 317933 [details]
Patch

[Saam Barati]

Comment on attachment 317933 [details]
Patch

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

LGTM.
It would be nice to add some tests that specifically target DFG compilation.
Does it pass other JSC tests?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2069
> +        forNode(node).setType(m_graph, SpecObjectOther);

We could do something like this here to be more specific:
forNode(node).setType(m_graph, m_codeBlock->globalObjectFor(node->origin.semanitc)->withScopeStructure())

[Saam Barati]

Comment on attachment 317933 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1721
> +            // Child2 is always the current scope, which is a kind of cell

Style nit: I'd say, "Child2 is always the current scope, which is guaranteed to be an object"

[Robin Morisset]

Created attachment 317944 [details]
Patch with Saam's suggestions

[Saam Barati]

Comment on attachment 317944 [details]
Patch with Saam's suggestions

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

r=me with:
- a few comments below
- please add some tests that specifically stress `with` in the DFG

> Source/JavaScriptCore/ChangeLog:3
> +        Support the 'which' keyword in DFG

typo: 'with' not 'which'

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2066
> +        // FIXME: Conservative

Please remove this FIXME or create a bug for it and link to the bugzilla here. I'd vote for just removing it. I think it's ok to be conservative here.

[Robin Morisset]

Created attachment 318054 [details]
Patch with test and fixing a bug

[Saam Barati]

Comment on attachment 318054 [details]
Patch with test and fixing a bug

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

r=me still

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2068
> +        // We cannot use the more precise withScopeStructure() here because it is a LazyProperty and may not yet be allocated.

Style nit: No need for this comment IMO. If you kept, I also wouldn't say "cannot", I'd just say, "we don't", since we could fix how withScopeStructure is allocated during JSGlobalObject creation.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2069
> +        forNode(node).setType(m_graph, SpecObject);

You want SpecObjectOther here since that's more precise than SpecObject

> JSTests/stress/with.js:1
> +for (var i = 0; i < 10000; ++i) {

You need to re-run prepare-Changelog so it can modify the Changelog entry inside JSTests/

[Robin Morisset]

Created attachment 318076 [details]
Patch with fixed Changelog, comment and setType

[Robin Morisset]

Created attachment 318077 [details]
Patch with fixed Changelog, comment and setType

[Robin Morisset]

I just re-uploaded the patch, because I realized my comment in the Changelog was wrong: there were quite a few tests of with already.

[WebKit Commit Bot]

Comment on attachment 318077 [details]
Patch with fixed Changelog, comment and setType

Clearing flags on attachment: 318077

Committed r220724: <http://trac.webkit.org/changeset/220724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 318077 [details]
Patch with fixed Changelog, comment and setType

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2066
> +        clobberWorld(node->origin.semantic, clobberLimit);

If pushing a with scope clobbers the world, shouldn't popping one (SkipScope) do the same?

[Robin Morisset]

I added that line to be conservative, I am not sure if it is needed. I can see two reasons why pushing a with scope could be more dangerous that popping one:
- it allocates memory for the new scope
- it calls toObject() on its argument, which can emit an exception.
Not sure if these justify clobbering the world or if something more limited would be better.

[Saam Barati]

(In reply to Robin Morisset from comment #17)
> I added that line to be conservative, I am not sure if it is needed. I can
> see two reasons why pushing a with scope could be more dangerous that
> popping one:
> - it allocates memory for the new scope
> - it calls toObject() on its argument, which can emit an exception.
> Not sure if these justify clobbering the world or if something more limited
> would be better.

I just audited the toObject code following the call tree of JSValue::toObject(.), and it would be safe to remove the clobbersWorld here. Just allocating does not mandate clobberWorld. Just throwing an exception due to OOM or some other type checks is also not enough to mandate clobberWorld. If I read the code correctly, that's all toObject() can do, so I think we're safe not clobbering the world here.

[Babak Shafiei]

<rdar://problem/33886937>