Closed
Bug 1512428
Opened 5 years ago
Closed 5 years ago
Add a ParseNodeVisitor for the JS frontend
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jorendorff, Assigned: khyperia)
References
Details
Attachments
(1 file, 1 obsolete file)
Hand-coding the tree traversal every time is honestly a lot more ugly, tricky code in reality than it was in my head. See the patch -- I would have expected the savings to appear only after switching multiple consumers to the new visitor class, but even with just a single user on board it's -279 lines net.
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Comment 2•5 years ago
|
||
Ashley might adopt this patch and push it through. Here's the additional work I would probably do before landing it: - Run it through Parsemark -- in theory the current patch should regress performance, but I haven't tried it. <https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Running_Parsemark> - Ditch all the virtual methods and use the Curiously Recurring Template Pattern, see if that helps. - In a separate changeset, rename all ParseNodeKinds that are expressions to have names that end with Expr, and all statements to end in Stmt. (For now, make an exception for Function and Class, which can occur as either declarations or expressions.) Also Star -> MulExpr. - In a separate changeset, change FOR_EACH_PARSE_NODE_KIND to use a class name (like TernaryNode) instead of an arity enum (like PN_TERNARY) as the second argument. Each class will have to have something like `static constexpr TernaryNode::arity()`, not a big deal. Then move `ParseNodeVisitor::visit_PN_TERNARY_children(ParseNode*& pn)` to `TernaryNode::visitChildren<V>(V&)`, and likewise for the other visit_*_children methods. This should make some of the `unsafe{Kid,Left,Right,Kid2,Kid3,Blah}Reference()` methods unnecessary.
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Attachment #9029805 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
Pushed by ahauck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5278b8d5dfe9 Create a ParseNodeVisitor and use it for constant folding. r=jwalden
Comment 5•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5278b8d5dfe9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 6•5 years ago
|
||
Backed out 3 changesets (Bug 1512428, Bug 1513040) for causing raptor tp6-5 perma failures on Windows 7 opt/pgo Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&searchStr=windows%2C7%2Ctp6-5&revision=bab13130b3f0880709e8c00ce77fec3ddd9eeea2&selectedJob=217671261 failure logs: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217662963&repo=autoland&lineNumber=1255
Flags: needinfo?(khyperia)
Updated•5 years ago
|
Status: RESOLVED → REOPENED
status-firefox66:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla66 → ---
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1e9f0298ad03 Backed out 3 changesets (bug 1512428, bug 1513040) for causing raptor tp6-5 perma failures on Windows 7 opt/pgo
Comment 8•5 years ago
|
||
This also caused a small number of Windows crashes - see https://bit.ly/2ClUDWj. I didn't bother filing a bug since this was backed out.
Comment 9•5 years ago
|
||
This issue is reproducible by accessing the https://apps.facebook.com/1375730929320387 game from the latest Nightly 66.0a1 20181218095120 from Ubuntu 16.04 x32. For every try the game fails to load and the crash occurs. Crash ID: 00f9254d-f375-4518-8432-e31cc0181218
Assignee | ||
Comment 10•5 years ago
|
||
I just pushed an update to phabricator [0] that passes raptor tp6-5 on Win7 opt as well as pgo [1]. Root cause: ParseNodes are represented by a large union of the various node types. The FoldConstants.cpp pass changes the "kind" of ParseNodes it's folding (for example from an AddExpr to a NumberExpr), which effectively changes which branch of the union is active. After swapping the kind, the fields in the union are set to whatever new values have been computed. There's a scary case, though: What if not all fields in the union are set? Then, the unset field would be whatever bit pattern was left behind from the previous ParseNode residing there. This was exactly what was happening [2]. (side note, that code was already present before my patch, and somehow wasn't causing problems). Upon converting to a StringExpr, NameNode::setInitializer(nullptr) was not called, and so the initializer pointer was the value of whatever bits were left there before. Eventually, the initializer gets touched, and segfaults ahoy. The patch fixes all instances of various fields of parsenodes not getting set after `setKind` is called. Someone should review this before merging, but once reviewed, I think this is able to land again. [0] https://phabricator.services.mozilla.com/D13991?vs=45142&id=45671&whitespace=ignore-most#toc [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d56c091849659d10664efc02fc046d6cb1bb2812 [2] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/js/src/frontend/FoldConstants.cpp#449-451
Flags: needinfo?(khyperia)
Comment 11•5 years ago
|
||
Pushed by ahauck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/719babda112c Create a ParseNodeVisitor and use it for constant folding. r=jwalden,jorendorff
Comment 12•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/719babda112c
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•