optimize buffer access in the RingBuffer class #355
Conversation
|
I've made some significant changes in PR #355 , where I optimized buffer access in the RingBuffer class. I believe these changes could improve the performance of our project. Could you please take a moment to review my PR? Your feedback would be greatly appreciated. Additionally, I'm planning to apply for GSOC'24 and I'm particularly interested in this project. I believe that your insights would not only help improve my PR but also my application for GSOC'24. Thank you for your time and consideration. Best, |
There was a problem hiding this comment.
Thank you for the pull request!
I have added some comments. I think making the array contiguous should improve performance but it would be best if we had some profiling or other measurement to make sure everything is having the effect we desire.
There are some strange things about these ringbuffer implementations so I appreciate your interest and wish you luck on your application!
| get framesAvailable() { | ||
| return this._framesAvailable; | ||
| } |
There was a problem hiding this comment.
This looks like a duplicate of the function below; was this intended?
| const sourceLength = arraySequence[0].length; | ||
| const dataArray = this._data; | ||
| const channelCount = this._channelCount; | ||
| const length = this._length; |
There was a problem hiding this comment.
Are these const aliases for performance purposes? If so, did you measure the performance difference versus accessing the object members directly?
|
|
||
| // Transfer data from the internal buffer to the |arraySequence| storage. |
There was a problem hiding this comment.
Why remove this comment?
| } | ||
|
|
||
| // For excessive frames, the buffer will be overwritten. |
There was a problem hiding this comment.
I think we need to keep some form of this comment, and actually looking at this a bit more closely we probably need to update the read index if we overflow the buffer.
Some ringbuffers will stop when they get full; this one just keeps writing. Changing that is probably outside the scope of this PR. But we should probably at least keep the comment.
There was a problem hiding this comment.
You're right, keeping some form of the comment is important to acknowledge the discussion about the efficiency of the proposed optimization. Additionally, updating the read index if the buffer overflows is a valid consideration, as it ensures proper behavior when the buffer is full. While changing the behavior of the ring buffer to stop when it gets full might be outside the scope of this PR, updating the comment and considering the overflow scenario are reasonable adjustments. I'll make sure to address both points in the code and update the comment accordingly. Thank you for your input!
There was a problem hiding this comment.
Hi
I had some Approach to updated code:
- After pushing data into the buffer, the write index and frames available are updated as before.
- Additionally, if the frames available exceed the buffer length, the read index is updated to the current write index, effectively wrapping around the buffer.
- The pull method remains unchanged, only updating the read index and frames available.
There was a problem hiding this comment.
This sounds fine to me. I think it may be more common in audio to stop writing when full, probably because that would produce one large discontinuity instead of potentially many small discontinuities, but as long as we are clear about what happens when more data is written than there is space in the buffer either way should be OK.
The pull method has a similar issue of the read index overtaking the write index if we read more than _framesAvailable.
| for (let channel = 0; channel < this._channelCount; ++channel) { | ||
| this._channelData[channel][this._writeIndex] = arraySequence[channel][i]; | ||
| for (let channel = 0; channel < channelCount; ++channel) { | ||
| const index = (this._writeIndex + i) % length; |
There was a problem hiding this comment.
Did you measure that this is more efficient than storing back to the object member? There was a recent change that did the opposite of what is being proposed here so it would be good to quantify which version is faster, and maybe add a comment with the results as well.
Optimize Buffer Access in RingBuffer Class
Issue: #347
Description:
This PR aims to optimize buffer access in the RingBuffer class by utilizing typed array views for more efficient data access. The changes improve performance by avoiding nested array accesses and directly accessing the underlying Float32Array.
Changes:
pushandpull) in the RingBuffer class to utilize typed array views for accessing channel data._dataproperty of the RingBuffer class for improved efficiency.