Closed Bug 1526031 Opened 8 months ago Closed 8 months ago

NameNode should not have a initOrStmt field

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: khyperia, Assigned: khyperia)

Details

Attachments

(1 file)

That one field gobbles up a surprising percentage of total ParseNode allocations. We should remove it and instead wrap the node in an AssignExpr to hold the initializer.

A follow-up patch should introduce a DeclarationExpr or something like that, so that declarations are their own node, instead of using AssignExpr for both declarations and assignments. (This patch doesn't change much, as DeclarationLists can already contain either AssignExpr (for patterns) or NameNode (for simple declarations)).

I'll file that next bug if that second patch doesn't get tacked onto this bug.

Pushed by ahauck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b29cead870d9
remove initOrStmt field from NameNode. r=jorendorff

Aaah right, dumb mistake by me. disassemble is apparently not defined in all builds.

I'll push an update soon.

Flags: needinfo?(khyperia)
Pushed by ahauck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94a9c4cabe37
remove initOrStmt field from NameNode. r=jorendorff
Attachment #9042266 - Attachment description: bug 1526031 - remove initOrStmt field from NameNode. → Bug 1526031 - remove initOrStmt field from NameNode.

Yoric, there's some really weird things happening with BinAST.

In this failure, there's a difference between the source parser and the binast parser https://treeherder.mozilla.org/logviewer.html#?job_id=228776807&repo=autoland

Got distinct ASTs when parsing jsapi-tests/binast/parser/multipart/unit/getter_setter_scope.js (0x346/0x462):
    BINARY
(StatementList [(Function (ParamsBody [e
                                       (LexicalScope []
                                         (StatementList [(VarStmt [(AssignExpr obj
                                                                               (ObjectExpr [(Colon "x"
                                                                                                   (Function (ParamsBody [(StatementList [(VarStmt [e])])])))
                                                                                            (Colon "x"
                                                                                                   (Function (ParamsBody [y
                                                                                                                          (StatementList [(VarStmt [e])])])))]))])]))]))])

    TEXT
(StatementList [(Function (ParamsBody [e
                                       (LexicalScope []
                                         (StatementList [(VarStmt [(AssignExpr obj
                                                                               (ObjectExpr [(Colon "x"
                                                                                                   (Function (ParamsBody [(LexicalScope []
                                                                                                                            (StatementList [(VarStmt [e])]))])))
                                                                                            (Colon "x"
                                                                                                   (Function (ParamsBody [y
                                                                                                                          (LexicalScope []
                                                                                                                            (StatementList [(VarStmt [e])]))])))]))])]))]))])

Those trees are produced from this source: https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/jsapi-tests/binast/parser/multipart/unit/getter_setter_scope.js

function foo(e) {
    var obj = {
        get x() {
            var e; // This `e` and the function argument `e` should have a distinct scope.
        },
        set x(y) {
            var e; // This `e` and the function argument `e` should have a distinct scope.
        }
    };
}

The difference is the presence/absence of (LexicalScope [] ...) within the innermost getters/setters.

Before this change, both source and binast do not produce the LexicalScope. Somehow (I'm not sure how) this change introduces the new LexicalScope. Reading the JS spec, I am fairly certain it should be there, the lack of a lexicalscope is a bug.

So, I looked into adding the lexicalscope to binast, and discovered some pretty weird generated C++ code. In particular, this LexicalScope (the one in question) dissapears into oblivion, never getting used in a handler_.newLexicalScope: https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/frontend/BinASTParser.cpp#2743

Here's my diagnosis, referring to BinSource.yaml nodes:

EagerMethod: has an inherits: EagerFunctionExpression. EagerFunctionExpression: creates the above LexicalScope in fields: contents: before:, and then consumes it with handler_.newLexicalScope inside its build:. EagerMethod: inherits the fields: contents: before: code, and so creates the LexicalScope struct. However, it does NOT inherit the build: code, and so it does not consume it with handler_.newLexicalScope.

I have pushed an update to this bug's patch to add that consumption code to LexicalScope: build:. My question to you is if this is the right thing to do. (Is build: supposed to be somehow inherited? Is the consumption code supposed to be somewhere else? Did you know about this issue and is actually by design?) - I've also added you as a reviewer on the phab patch so you can sign off on that being correct.

Flags: needinfo?(khyperia) → needinfo?(dteller)

Yes, build: should be inherited. However by now, arai knows this code (and the JS standard) much better than I do, so let me deflect :)

Flags: needinfo?(dteller) → needinfo?(arai.unmht)

(In reply to Ashley Hauck [:khyperia] from comment #7)

Here's my diagnosis, referring to BinSource.yaml nodes:

EagerMethod: has an inherits: EagerFunctionExpression. EagerFunctionExpression: creates the above LexicalScope in fields: contents: before:, and then consumes it with handler_.newLexicalScope inside its build:. EagerMethod: inherits the fields: contents: before: code, and so creates the LexicalScope struct. However, it does NOT inherit the build: code, and so it does not consume it with handler_.newLexicalScope.

I have pushed an update to this bug's patch to add that consumption code to LexicalScope: build:. My question to you is if this is the right thing to do. (Is build: supposed to be somehow inherited? Is the consumption code supposed to be somewhere else? Did you know about this issue and is actually by design?) - I've also added you as a reviewer on the phab patch so you can sign off on that being correct.

yes, build: is inherited only when there's no build: in the derived interface rule,
and given EagerMethod and EagerFunctionExpression have different build: rule, adding lexical scope handling in EagerMethod's build: rule is the correct fix.

Flags: needinfo?(arai.unmht)
Pushed by ahauck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01d931a9140b
remove initOrStmt field from NameNode. r=jorendorff,arai

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=01d931a9140b35f9ea90a6a5a45956510f5ef7f1&selectedJob=230966621

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230966621&repo=autoland&lineNumber=4682

Backout link: https://hg.mozilla.org/integration/autoland/rev/c4d3d41136213af2b11a74d5e2a70aa227bd5d1e

[task 2019-02-28T09:12:35.218Z] 09:12:35 INFO - 3 warnings generated.
[task 2019-02-28T09:12:35.218Z] 09:12:35 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/modules/zlib/src'
[task 2019-02-28T09:12:35.233Z] 09:12:35 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mfbt'
[task 2019-02-28T09:12:35.233Z] 09:12:35 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -m32 -o Unified_cpp_mfbt1.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DIMPL_MFBT -DLZ4LIB_VISIBILITY= -I/builds/worker/workspace/build/src/mfbt -I/builds/worker/workspace/build/src/obj-firefox/mfbt -I/builds/worker/workspace/build/src/mfbt/double-conversion -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fcrash-diagnostics-dir=/builds/worker/artifacts -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer -funwind-tables -Werror -MD -MP -MF .deps/Unified_cpp_mfbt1.o.pp /builds/worker/workspace/build/src/obj-firefox/mfbt/Unified_cpp_mfbt1.cpp
[task 2019-02-28T09:12:35.233Z] 09:12:35 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/mfbt'
[task 2019-02-28T09:12:35.249Z] 09:12:35 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/js/src/frontend'
[task 2019-02-28T09:12:35.251Z] 09:12:35 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -m32 -o BinASTParser.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DJS_HAS_CTYPES -DFFI_BUILDING -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/builds/worker/workspace/build/src/js/src/frontend -I/builds/worker/workspace/build/src/obj-firefox/js/src/frontend -I/builds/worker/workspace/build/src/obj-firefox/js/src -I/builds/worker/workspace/build/src/js/src -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/js/src/js-confdefs.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-noexcept-type -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fcrash-diagnostics-dir=/builds/worker/artifacts -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -Werror=format -Wno-shadow -MD -MP -MF .deps/BinASTParser.o.pp /builds/worker/workspace/build/src/js/src/frontend/BinASTParser.cpp
[task 2019-02-28T09:12:35.251Z] 09:12:35 INFO - In file included from /builds/worker/workspace/build/src/js/src/frontend/BinASTParser.cpp:12:
[task 2019-02-28T09:12:35.251Z] 09:12:35 INFO - In file included from /builds/worker/workspace/build/src/js/src/frontend/BinASTParser.h:17:
[task 2019-02-28T09:12:35.251Z] 09:12:35 INFO - In file included from /builds/worker/workspace/build/src/js/src/frontend/BCEParserHandle.h:11:
[task 2019-02-28T09:12:35.251Z] 09:12:35 INFO - In file included from /builds/worker/workspace/build/src/js/src/frontend/FullParseHandler.h:16:
[task 2019-02-28T09:12:35.252Z] 09:12:35 ERROR - /builds/worker/workspace/build/src/js/src/frontend/ParseNode.h:1622:9: error: use of undeclared identifier 'statement_'
[task 2019-02-28T09:12:35.252Z] 09:12:35 INFO - if (statement_) {
[task 2019-02-28T09:12:35.252Z] 09:12:35 INFO - ^
[task 2019-02-28T09:12:35.253Z] 09:12:35 ERROR - /builds/worker/workspace/build/src/js/src/frontend/ParseNode.h:1623:26: error: use of undeclared identifier 'statement_'
[task 2019-02-28T09:12:35.253Z] 09:12:35 INFO - if (!visitor.visit(statement_)) {
[task 2019-02-28T09:12:35.253Z] 09:12:35 INFO - ^
[task 2019-02-28T09:12:35.253Z] 09:12:35 INFO - 2 errors generated.
[task 2019-02-28T09:12:35.253Z] 09:12:35 INFO - /builds/worker/workspace/build/src/config/rules.mk:805: recipe for target 'BinASTParser.o' failed
[task 2019-02-28T09:12:35.253Z] 09:12:35 ERROR - make[4]: *** [BinASTParser.o] Error 1
[task 2019-02-28T09:12:35.254Z] 09:12:35 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src/frontend'
[task 2019-02-28T09:12:35.254Z] 09:12:35 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'js/src/frontend/target' failed
[task 2019-02-28T09:12:35.254Z] 09:12:35 ERROR - make[3]: *** [js/src/frontend/target] Error 2
[task 2019-02-28T09:12:35.254Z] 09:12:35 INFO - make[3]: *** Waiting for unfinished jobs....
[task 2019-02-28T09:12:35.271Z] 09:12:35 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/js/src/wasm'
[task 2019-02-28T09:12:35.271Z] 09:12:35 INFO - js/src/wasm/Unified_cpp_js_src_wasm1.o
[task 2019-02-28T09:12:35.271Z] 09:12:35 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src/wasm'
[task 2019-02-28T09:12:35.271Z] 09:12:35 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/nspr/pr'
[task 2019-02-28T09:12:35.271Z] 09:12:35 INFO - config/external/nspr/pr/Unified_c_external_nspr_pr1.o

Flags: needinfo?(khyperia)

Oh fun, test-merge race conditions. Looks like a change got in between me testing things out and pushing, sorry about that.

Rebasing now and will recommit soon. (It's kind of weird, though, I got a merge conflict when rebasing just now, but lando was fine with it? hmm.)

Flags: needinfo?(khyperia)
Pushed by ahauck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d10717796a29
remove initOrStmt field from NameNode. r=jorendorff,arai
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.