Refactor some visitor internals #4447
Conversation
asl
added
the
core
ir/visitor.cpp
Outdated
| @@ -311,25 +429,23 @@ const IR::Node *Modifier::apply_visitor(const IR::Node *n, const char *name) { | |||
| if (ctxt) ctxt->child_name = name; | |||
| if (n) { | |||
| PushContext local(ctxt, n); | |||
| if (visited->busy(n)) { | |||
| auto [busy, done] = visited->start(n, visitDagOnce); | |||
There was a problem hiding this comment.
In the original code, start really was only used when starting processing on a node. Now it has the concept of "get status and start if not already started". Perhaps a name like update / update_status / start_or_status?
There was a problem hiding this comment.
It was mixed thing actually. There was "implicit" start in Inspector and slightly different thing in Modifier/ Transform. I thought about try_start. But if there are other options, then I'm all for it :)
|
@grg Renamed the function. Also it returns more clear status now, not just pair of bools |
Also fix the subtle bug with iterator invalidation during insert / emplace
…ilar to ChangeTracker
…ore clear status
| void visitAgain() const { *visitCurrentOnce = false; } | ||
| // FIXME: It would be better named visitCurrentOnce() / visitCurrenAgain() | ||
| virtual void visitOnce() const { BUG("do not know how to handle request"); } | ||
| virtual void visitAgain() const { BUG("do not know how to handle request"); } |
There was a problem hiding this comment.
Maybe delete these here -- make them only in Inspector/Modifier/Transform, and final s well (no need for virtual)
| }; | ||
| typedef std::unordered_map<const IR::Node *, info_t> visited_t; | ||
| std::shared_ptr<visited_t> visited; | ||
| std::shared_ptr<Tracker> visited; |
There was a problem hiding this comment.
The point of having this be an info_t instead of a trscker is tha an Inspector can't change nodes, so only needs to store 2 bits per node visited and not an additional pointer. If you change it to an hvec_map instead of an unordered_map, it would might be possible to make it even more compact.
There was a problem hiding this comment.
See next PRs for further improvements here :) And its 'Tracker', not 'ChangeTracker'. We indeed store only two bools. Though due to alignment requirements it's a bit more depending on the platform.
There was a problem hiding this comment.
And for now I cannot use it due to #4389 as it is not a drop-in replacement for unordered_map
Set the done flag to flase when revisiting a node in Tracker::try_start. Fixes a small regression introduced in #4447.
Set the done flag to false when revisiting a node in Tracker::try_start. Fixes a small regression introduced in #4447.
Reset the done/visit_in_progress flag when revisiting a node in Visitor::Tracker::try_start or Visitor::ChangeTracker::try_start. For inspectors, fixes a small regression introduced in #4447. For Modifiers/Transforms, fixes a bug that hadn't been previously encountered. (Bug revealed by test program.)
Reset the done/visit_in_progress flag when revisiting a node in Visitor::Tracker::try_start or Visitor::ChangeTracker::try_start. For inspectors, fixes a small regression introduced in #4447. For Modifiers/Transforms, fixes a bug that hadn't been previously encountered. (Bug revealed by test program.)
This is a first step towards #4418
The particular set of changes includes:
Inspector's visiting tracking to be similar toChangeTrackerofModifier/Transform. Albeit very similar, there are important differences here and thereInspector::apply_visitor. It was not exposed due to waystd::unordered_mapis implemented in libc++ / libstdc++. Long story short: iterator was kept across possible insertion. It will be invalidated in case of rehashvisitOnce/visitAgain. Previously we had a pointer to a field inside map's value stored inVisitor. So we risked a lot to obtain some dangling pointer to freed memory. It worked due to guarantees we had fromstd::unordered_map, but would stop to work with maps with less strong guarantees. And in general, it looks like an escaping pointer complicating code clarity.Visitor::Contextvia rearranging fields to reduce alignment padding wasteAs a result we are having ~5% speedup on
P4CParserUnroll.switch_20160512testcase:gtestp4c-baseline --gtest_filter=P4CParserUnroll.switch_20160512gtestp4c --gtest_filter=P4CParserUnroll.switch_20160512(benchmarking by hyperfine with 10 iterations and warm-up)