Closed Bug 1512428 Opened 5 years ago Closed 5 years ago

Add a ParseNodeVisitor for the JS frontend

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

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.
Attached 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.

<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.
Attachment #9029805 - Attachment is obsolete: true
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
Blocks: 1513040
Pushed by ahauck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5278b8d5dfe9
Create a ParseNodeVisitor and use it for constant folding. r=jwalden
https://hg.mozilla.org/mozilla-central/rev/5278b8d5dfe9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Status: RESOLVED → REOPENED
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
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.
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
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)
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
https://hg.mozilla.org/mozilla-central/rev/719babda112c
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: