Skip to content

Conversation

@joadnacer
Copy link
Contributor

@joadnacer joadnacer commented Oct 4, 2023

Metron benchmarks on M1 mac, testing time to write a slice to buffer then read it. Number corresponds to input size in bytes. RingBuffer capacity > 500.

  • masterRead = read() function, which still exists following proposed changes
  • newRead = readFirst()/readLast(), newly implemented.
  • masterWrite = master writeSlice() function
  • newWrite = modified writeSlice() function

Can see a slight loss in performance for small read/write sizes, mostly for the new read methods, however old read method still exists so not an issue. New write method is strictly an improvement. Huge improvements for large read/write sizes. Have not benchmarked writing data with a length greater than RingBuffer capacity but new writeSlice() method will be extremely efficient here as it skips data that will get overwritten (excluding first write). 23x performance for new impl with 500 bytes length write/read

-----------------------------------------------------------
Benchmark                               Time       Iterations
-----------------------------------------------------------
Length = 1
ringbuffer/masterWriteMasterRead       1.10 ns     1145553870
ringbuffer/masterWriteNewRead          1.19 ns     1184928558
ringbuffer/newWriteNewRead             1.17 ns     1185958676
ringbuffer/newWriteMasterRead          1.12 ns     1237117497
Length = 5
ringbuffer/masterWriteMasterRead       4.83 ns      294598897
ringbuffer/masterWriteNewRead          3.57 ns      393993845
ringbuffer/newWriteNewRead             5.33 ns      262691990
ringbuffer/newWriteMasterRead          4.10 ns      328091998
Length = 25
ringbuffer/masterWriteMasterRead       14.4 ns       87942575
ringbuffer/masterWriteNewRead          7.21 ns      197825287
ringbuffer/newWriteNewRead             5.92 ns      236331394
ringbuffer/newWriteMasterRead          11.1 ns      125616070
Length = 100
ringbuffer/masterWriteMasterRead        121 ns       10656980
ringbuffer/masterWriteNewRead          71.5 ns       19619597
ringbuffer/newWriteNewRead             9.94 ns      142748448
ringbuffer/newWriteMasterRead          65.1 ns       21449213
Length = 500
ringbuffer/masterWriteMasterRead        642 ns        2182755
ringbuffer/masterWriteNewRead           335 ns        4215511
ringbuffer/newWriteNewRead             28.4 ns       50308287
ringbuffer/newWriteMasterRead           338 ns        4150044

Could eek out some more performance improvements by forcing capacity to a power of two to enable for bitwise AND masking but fairly negligible - not allowing for writing a slice of length > max buffer length would also improve write performance slightly due to removal of branch and one @min call

also still very easy to use buffer to write a slice byte by byte manually as with previous writeSlice implementation via manual use of write() function in loop

@joadnacer joadnacer changed the title std.RingBuffer: Implement multi-byte read/write std.RingBuffer: Implement mem.copy read/write Oct 4, 2023
@joadnacer joadnacer force-pushed the ringbuffer-optim branch 5 times, most recently from c5d1657 to ccb0286 Compare October 4, 2023 22:57
@joadnacer
Copy link
Contributor Author

failed build due to fmt, fixed now so should build all archs

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

@joadnacer joadnacer force-pushed the ringbuffer-optim branch 3 times, most recently from e4c472a to 7204b79 Compare October 9, 2023 07:55
@joadnacer joadnacer requested a review from andrewrk October 10, 2023 09:57

const Allocator = @import("std").mem.Allocator;
const assert = @import("std").debug.assert;
const copy = @import("std").mem.copy;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zig/lib/std/mem.zig

Lines 195 to 197 in b3aaf85

/// Deprecated: use `@memcpy` if the arguments do not overlap, or
/// `copyForwards` if they do.
pub const copy = copyForwards;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified to support both @memcpy and copyForwards via separate methods, no longer using deprecated copy

@andrewrk
Copy link
Member

Looks good, thank you!

@andrewrk andrewrk enabled auto-merge October 22, 2023 02:36
@andrewrk andrewrk merged commit d8c0679 into ziglang:master Oct 22, 2023
@joadnacer joadnacer deleted the ringbuffer-optim branch October 22, 2023 17:26
dweiller added a commit to dweiller/zig that referenced this pull request Nov 3, 2023
This reverts a bug introduced in ziglang#17400 when decompressing a RLE block
into a ring buffer.
dweiller added a commit to dweiller/zig that referenced this pull request Nov 3, 2023
This reverts a change introduced in ziglang#17400 causing a bug when
decompressing an RLE block into a ring buffer.

RLE blocks contain only a single byte of data to copy into the output,
so attempting to copy a slice causes buffer overruns and incorrect
decompression.
andrewrk pushed a commit that referenced this pull request Nov 4, 2023
This reverts a change introduced in #17400 causing a bug when
decompressing an RLE block into a ring buffer.

RLE blocks contain only a single byte of data to copy into the output,
so attempting to copy a slice causes buffer overruns and incorrect
decompression.
kubkon pushed a commit that referenced this pull request Nov 4, 2023
This reverts a change introduced in #17400 causing a bug when
decompressing an RLE block into a ring buffer.

RLE blocks contain only a single byte of data to copy into the output,
so attempting to copy a slice causes buffer overruns and incorrect
decompression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants