chore: Extract the ring-ish structure implicit in Fanout #11142
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✔️ Deploy Preview for vector-project ready! 🔨 Explore the source changes: 8dae1a3 🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/61f9cbd1329fdb00080f8904 😎 Browse the preview: https://deploy-preview-11142--vector-project.netlify.app |
github-actions
bot
added
the
domain: core
lukesteensen
reviewed
Feb 1, 2022
There was a problem hiding this comment.
One of the oddities of
Fanoutwas the use of anito index sinks. This was, partially, preserved across polls but was not in general use when looping. It is my understanding thatFanout.iwas ultimately vestigial and any actual indexing was reset each poll.
I think this is true everywhere except poll_ready. Given semantics of poll_ready on a channel-like sender effectively reserving a slot for a new item, it does seem like a place where we were using the i struct member directly for iteration and therefore avoiding double-polling any sinks that had already indicated ready. This seems like something we likely want to keep, unless the semantics of poll_ready mentioned are inaccurate.
I think I'm missing something subtle here potentially. I am aware of some channels reserving extra slots but I didn't realize -- and can't find documentation -- for that being a general consideration. Very possible that I've overlooked something. That said, assuming that it is a general consideration, I'm confused about why only |
Soak Test ResultsBaseline: 3f31469 ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. Changes in throughput with confidence ≥ 90.00%:
Fine details of change detection per experiment.
Fine details of each soak run.
|
One of the oddities of `Fanout` was the use of an `i` to index sinks. This was, partially, preserved across polls but was not in general use when looping. It is my understanding that `Fanout.i` was ultimately vestigial and any actual indexing was reset each poll. I think, as a result, we would also repoll the same sink multiple times when removals happened, which should be rare in practice but was possible. I have extracted the vector and index munging into a `Store` type. We should now no longer poll underlying sinks multiple times and calling code does not have to munge indexes, although it is required to manually advance/reset a 'cursor' because we're changing the shape of an iterator while iterating it. The primary difference here is the use of `swap_remove` instead of `remove`. This saves a shift. I expect no performance change here. I think, ultimately, this is a stepping stone to getting the logic here very explicit so we can start to do broadcasting in a way that is not impeded by slow receivers downstream. REF #10144 REF #10912 Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
|
I guess it comes down to an interpretation of the It doesn't matter in Reading the |
Aaah I see what you're saying now, yes. Yes I have changed the behavior here because I misunderstood what it was we were up to. |
Soak Test ResultsBaseline: 251a7b4 ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. No interesting changes with confidence ≥ 90.00%: Fine details of change detection per experiment.
Fine details of each soak run.
|
|
This is a dead-end, predicated partially on a misunderstanding. Thanks for setting me straight @lukesteensen. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One of the oddities of
Fanoutwas the use of anito index sinks. This was,partially, preserved across polls but was not in general use when looping. It is
my understanding that
Fanout.iwas ultimately vestigial and any actualindexing was reset each poll. I think, as a result, we would also repoll the
same sink multiple times when removals happened, which should be rare in
practice but was possible. I have extracted the vector and index munging into a
Storetype. We should now no longer poll underlying sinks multiple times andcalling code does not have to munge indexes, although it is required to manually
advance/reset a 'cursor' because we're changing the shape of an iterator while
iterating it.
The primary difference here is the use of
swap_removeinstead ofremove. This saves a shift.I expect no performance change here. I think, ultimately, this is a stepping
stone to getting the logic here very explicit so we can start to do broadcasting
in a way that is not impeded by slow receivers downstream. Or, less so.
REF #10144
REF #10912
Signed-off-by: Brian L. Troutwine brian@troutwine.us