Experiment: store nodes in contiguous Vec instead of HashMap #379
Conversation
This file contains hidden or 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
|
/bench |
|
No functional changes compared to the previous commit
|
/bench |
|
b-ma
reviewed
Oct 25, 2023
| impl NodeCollection { | ||
| pub fn new() -> Self { | ||
| let mut instance = Self { | ||
| nodes: Vec::with_capacity(64), |
There was a problem hiding this comment.
I wonder if we should be more agressive here and grab more memory at startup here, e.g. Vec::with_capacity(2048)
There was a problem hiding this comment.
std::mem::size_of::<RefCell<Node>>() is 160 bytes, so that would mean over 300kB allocated.
That's not terrible of course, but I think most audio graphs really don't need that many nodes.
Whatever the initial size, we do need to think about the performance impact of outgrowing that size.
- for the reallocation, we could perform it on the control thread because it will know when we run out of space
- for the memcpy, we should maybe look at reducing the side of
NodeReducesize_ofNodefor better cache performance in render loop #208 or an amortized growth like https://github.com/jonhoo/griddle
b-ma
reviewed
Oct 25, 2023
src/render/graph.rs
Outdated
| @@ -126,6 +125,7 @@ impl Graph { | |||
| let inputs = vec![AudioRenderQuantum::from(self.alloc.silence()); number_of_inputs]; | |||
| let outputs = vec![AudioRenderQuantum::from(self.alloc.silence()); number_of_outputs]; | |||
|
|
|||
| let index = index.0 as usize; | |||
There was a problem hiding this comment.
Hey, really interesting perf improvements
Just thinking (maybe I'm mistaking, not really sure and maybe that's what you have in mind but...) renaming these to id rather than index could help continue refactoring I think.
Because then we could have two parallel Vec: a Vec<Option<AudioNodeId>> and a Vec<Option<RefCell<Node>>> where the indexes would correspond. Then we could reuse None places in the Vec to prevent the it from growing on indefinitely (would maybe imply a third Vec<Option<usize>> containing the dropped positions for housekeeping)
There was a problem hiding this comment.
Hey thanks for the suggestion. I actually have another solution in mind where the audio graph lets the AudioContext know ( realtime safe) which AudioNodeIds are available again. Would be interesting to benchmark against your solution
|
I will have to evaluate the performance regression between the first and second commit |
Todo: reinstate tests for Graph, add tests for reusing
|
/bench |
|
This could crash the render thread because previously it would drop the AudioParamRenderer alongside the AudioNode
|
/bench |
|
|
Alright, very happy with the results. This is ready for review @b-ma, but take your time. I'm not in a hurry |
b-ma
reviewed
Oct 29, 2023
src/render/graph.rs
Outdated
| self.nodes | ||
| .get_mut(&source.0) | ||
| .unwrap_or_else(|| panic!("cannot connect {:?} to {:?}", source, dest)) | ||
| self.nodes[source.0 .0 as usize] |
There was a problem hiding this comment.
I'm a bit confused with this syntax: source.0 .0 as usize, what does it mean?
There was a problem hiding this comment.
Yeah that looks weird. It is a double tuple unpack for which rustfmt has added an extra space. This appears to be not necessary anymore if we change the config: rust-lang/rustfmt#5745
I will pick that up in a separate PR
Then again I agree the nested tuple is not nice to read. I will change the NodeCollection interface to take AudioNodeId instead of usize for indexing. This will make the code nicer!
There was a problem hiding this comment.
I will change the NodeCollection interface to take AudioNodeId instead of usize for indexing. This will make the code nicer!
+1
|
I think I got it, no special comment on my side, looks very nice, great work! |
b-ma
reviewed
Oct 29, 2023
src/context/concrete_base.rs
Outdated
| /// | ||
| /// It reuses the ids of decommissioned nodes to prevent unbounded growth of the audio graphs node | ||
| /// list (which is stored in a Vec indexed by the AudioNodeId). | ||
| struct AudioNodeIdIssuer { |
There was a problem hiding this comment.
I find audio_node_id_issuer.issue() a bit weird.
Maybe rename to AudioNodeIdProvider? i.e. audio_node_id_provider.get()
...But really no big deal
|
/bench |
|
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.
Don't mind the unreadable code, I'm seeing +-10% performance gains in some benchmarks.
A big todo is to reuse deallocated node slots, or the Vec may grow indefinitely for long running processes