Add a ParseNodeVisitor for the JS frontend

RESOLVED FIXED in Firefox 66



8 months ago
7 months ago


(Reporter: jorendorff, Assigned: khyperia)


Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)



(1 attachment, 1 obsolete attachment)

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.
Posted file Bug 1512428 - JS AST visitor. (obsolete) —
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.


- 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.
Attachment #9029805 - Attachment is obsolete: true
Assignee: nobody → khyperia
Blocks: 1513040
Pushed by
Create a ParseNodeVisitor and use it for constant folding. r=jwalden
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Resolution: FIXED → ---
Target Milestone: mozilla66 → ---
Backout by
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 I didn't bother filing a bug since this was backed out.
This issue is reproducible by accessing the 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
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.



Flags: needinfo?(khyperia)
Pushed by
Create a ParseNodeVisitor and use it for constant folding. r=jwalden,jorendorff
Closed: 7 months ago7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.