Generate single await-push command per buffer for all local chunks#324
Generate single await-push command per buffer for all local chunks#324
Conversation
|
Check-perf-impact results: (241536968c1afd627708e72c171c89f6) 🚀 Significant speedup (<0.80x) in some microbenchmark results: benchmark intrusive graph dependency handling with N nodes - 100 / creating nodes Relative execution time per category: (mean of relative medians)
|
Pull Request Test Coverage Report for Build 16268225707Details
💛 - Coveralls |
GagaLP
left a comment
GagaLP
left a comment
There was a problem hiding this comment.
Other than the small comment i made this looks good.
src/command_graph_generator.cc
Outdated
| if(!box.empty() && !wcs.is_fresh()) { missing_parts_boxes.push_back(box); } | ||
| } | ||
|
|
||
| // There is data we don't yet have locally. Generate an await push command for it. |
There was a problem hiding this comment.
For me this comment now seems a bit out of place since we don't generate the await push command in this if.
fknorr
left a comment
fknorr
left a comment
There was a problem hiding this comment.
Seems like the correct thing to do!
|
|
||
| void add(const box_vector<Dims>& boxes) & { | ||
| m_boxes.reserve(m_boxes.size() + boxes.size()); | ||
| for(const auto& b : boxes) { |
There was a problem hiding this comment.
Maybe have a comment here that we use a loop rather than insert(end) to skip over empty boxes, something which add(region) does not need to do.
[only if my comment below does not apply]
| auto& required_boxes = per_buffer_required_boxes[bid]; // allow default-insert | ||
| required_boxes.add(missing_parts_boxes); |
There was a problem hiding this comment.
Couldn't this just be moved into the loop in L401? Then we don't need missing_parts_boxes (or region_builder::add(box_vector) for that matter)
GagaLP
force-pushed
the
generate-single-await-push
branch
from
9a70a5b to
12fb858
Compare
This brings await-pushes in line with pushes, where we already compute the union of all regions required by remote chunks executed on the same node.
PeterTh
force-pushed
the
generate-single-await-push
branch
from
12fb858 to
0df4997
Compare
|
(Fixup'ed Gabriel's comment improvements before expected merge) |
PeterTh
force-pushed
the
generate-single-await-push
branch
from
0df4997 to
f9a8656
Compare
|
Check-perf-impact results: (ae6918621b46271c2f10d6eb978fe95d) ✔️ No significant performance change in the microbenchmark set. You are good to go! Relative execution time per category: (mean of relative medians)
|
5 participants
This brings await-pushes in line with pushes, where we already compute the union of all regions required by remote chunks executed on the same node.