[async] Add customized container for async state to node map #2070
Conversation
There was a problem hiding this comment.
Sorry about the delayed reply. The interface part looks great! I was actually expecting NewStateToNodesMap to be a light-weight wrapper of the original llvm::SmallVector<std::pair<AsyncState, llvm::SmallSet<Node *, 8>>, 4> structure at the first step, and then we discuss how to implement these APIs efficiently on our own, to replace the llvm containers.
Sorry about not stating the plan clearly, but the current implementation of NewStateToNodesMap seems to be on the right track already! I think it's perfectly fine to split this migration into multiple PRs. Let me know when you are free for a chat on the design of the data structure internals, and I'll be happy to contribute to performance tuning!
(It's also nice that many edge operations are now much simpler with the new API!)
k-ye
force-pushed
the
refactor
branch
3 times, most recently
from
0506546 to
0544814
Compare
There was a problem hiding this comment.
I was actually expecting NewStateToNodesMap to be a light-weight wrapper.
Yeah, I thought about that. But I was too lazy to create a fully customized iterator type with a proper abstraction, and just borrowed the underlying container's. If we just wrap the existing container, we probably still need to deal with a large amount of refactoring due to the iterator type change..
Let me know when you are free for a chat on the design of the data structure internals, and I'll be happy to contribute to performance tuning!
Awesome! I think this PR should lay a good foundation for switching to mask, or other more efficient implementation. Note that once we make the switch and add a sort stage, many methods here should just assume that data_ is already sorted, instead of calling maybe_sort() on their own.
| void StateToNodesMap::insert_edge(const AsyncState &as, Node *n) { | ||
| TI_ASSERT(!sorted_); | ||
| const Edge e = std::make_pair(as, n); | ||
| if (has_edge(e)) |
There was a problem hiding this comment.
this could be problematic if has_edge is supposed to be used only after sorting.
There was a problem hiding this comment.
Right, hopefully, once we have the mask, most has_edge queries can quickly return false.
There was a problem hiding this comment.
Thanks! I took a pass and now I have a rough feeling on the use cases and the current internals of the customized container. A few things to discuss tomorrow:
- We may need to codesign the SFG optimizations and the customized container for performance and simplicity. For example, is
disconnect_all(for node deletion) really necessary? It seems that if we makerebuild_graphfast, we can simply remove the node and then rebuild the graph once we need to delete nodes. Then we no longer need to support theremove_nodefunction inStateToNodesMap. - How to deal with
LatestStateReaders - High-performance implementation of has_edge (probably need to tune on site after this PR)
- ...
| } | ||
| void StateToNodesMap::clear() { | ||
| sorted_ = false; | ||
| // It seems that once we are beyond the small buffer threshold, there's no |
There was a problem hiding this comment.
I think this is fine :-) IIRC this is only used on the initial_node_ - if that is the case we can directly re-initialize the whole container.
| void StateToNodesMap::insert_edge(const AsyncState &as, Node *n) { | ||
| TI_ASSERT(!sorted_); | ||
| const Edge e = std::make_pair(as, n); | ||
| if (has_edge(e)) |
There was a problem hiding this comment.
Right, hopefully, once we have the mask, most has_edge queries can quickly return false.
| // This should be used only after |data_| is sorted. | ||
| // 1) disconnect_with | ||
| // 2) fuse_range | ||
| // So adding the mask shouldn't affect this implementation. |
There was a problem hiding this comment.
This function doesn't seem to be called very frequently. We can likely use some correct but slow solution.
| bool StateToNodesMap::replace_node_in_edge(const AsyncState &as, | ||
| Node *old_nd, | ||
| Node *new_nd) { | ||
| maybe_sort(); | ||
|
|
||
| const Edge oe = std::make_pair(as, old_nd); | ||
| auto iter = std::lower_bound(data_.begin(), data_.end(), oe); | ||
| if (!matches(iter, oe)) { | ||
| return false; | ||
| } | ||
| if (has_edge(as, new_nd)) { | ||
| data_.erase(iter); | ||
| return true; | ||
| } | ||
| iter->second = new_nd; | ||
|
|
||
| // TODO: Is getting the range an overkill? Just sort the entire data range? | ||
| auto rn = (*this)[as]; | ||
| TI_ASSERT(rn.begin() <= iter && iter < rn.end()); | ||
| std::sort(rn.begin(), rn.end()); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
If this is troublesome, we may consider not supporting this API at all.
Add a customized
StateToNodesMapdata structure for fast SFG edge insertion and lookup.This PR probably would make the performance even worse for now, because deduping during the edge insertion is slow..
[Click here for the format server]