Summary: | Support the with keyword in DFG | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robin Morisset <rmorisset> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bshafiei, buildbot, commit-queue, ggaren, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Robin Morisset
2017-08-10 19:30:12 PDT
Created attachment 317932 [details]
Patch
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.
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 Created attachment 317933 [details]
Patch
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()) 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" Created attachment 317944 [details]
Patch with Saam's suggestions
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. Created attachment 318054 [details]
Patch with test and fixing a bug
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/ Created attachment 318076 [details]
Patch with fixed Changelog, comment and setType
Created attachment 318077 [details]
Patch with fixed Changelog, comment and setType
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. 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> All reviewed patches have been landed. Closing bug. 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? 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. (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. |