Add a ParseNodeVisitor for the JS frontend

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: jorendorff, Assigned: khyperia)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 months ago
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

6 months ago
Posted file Bug 1512428 - JS AST visitor. (obsolete) —
(Reporter)

Comment 2

6 months 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)

Updated

5 months ago
Attachment #9029805 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
(Reporter)

Updated

5 months ago
Blocks: 1513040

Comment 4

5 months ago
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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5278b8d5dfe9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla66 → ---

Comment 7

5 months ago
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
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 months 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 months 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 months 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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/719babda112c
Status: REOPENED → RESOLVED
Last Resolved: 5 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.