Skip to content

Commit e0ef934

Browse files
committed
termio: replace SegmentedPool with std.MemoryPool
The purpose of SegmentedPool was pointer-stable values for the pty write path, and the std.MemoryPool provides that. SegmentedPool is actually so old it predates a stdlib memory pool! Just noting why I did it in the first place. I also wrote it when I was pretty fucking bad at Zig, so I'm shocked its lasted this long. The write path is hot , so the replacement was benchmarked against the old SegmentedPool plus a rewrite simple Pool I did before realizing... wait... why not just a MemoryPool. Benchmarked using the real 240-byte xev write request. workload old std.MemoryPool depth-1 (keystroke echo) 3.93 ns/op 0.96 ns/op burst (1MiB paste, d=256) 4.27 ns/op 1.00 ns/op cold growth (32 -> 16k) 4.54 ns/op 6.48 ns/op malloc create/destroy 15.9 ns/op (baseline) Cold growth is slower but this is only a cost when the pool grows. Note this also gets rid of the preallocation, which didn't show any measurable performance benefit at all. This has the benefit of shrinking our ThreadData by ~10KB.
1 parent 8eecb8f commit e0ef934

3 files changed

Lines changed: 28 additions & 119 deletions

File tree

src/datastruct/main.zig

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ const blocking_queue = @import("blocking_queue.zig");
55
const cache_table = @import("cache_table.zig");
66
const circ_buf = @import("circ_buf.zig");
77
const intrusive_linked_list = @import("intrusive_linked_list.zig");
8-
const segmented_pool = @import("segmented_pool.zig");
98
const split_tree = @import("split_tree.zig");
109

1110
pub const BlockingQueue = blocking_queue.BlockingQueue;
@@ -14,7 +13,6 @@ pub const CircBuf = circ_buf.CircBuf;
1413
pub const IntrusiveDoublyLinkedList = intrusive_linked_list.DoublyLinkedList;
1514
pub const LimitedAllocator = @import("limited_allocator.zig").LimitedAllocator;
1615
pub const MessageData = @import("message_data.zig").MessageData;
17-
pub const SegmentedPool = segmented_pool.SegmentedPool;
1816
pub const SplitTree = split_tree.SplitTree;
1917

2018
test {

src/datastruct/segmented_pool.zig

Lines changed: 0 additions & 96 deletions
This file was deleted.

src/termio/Exec.zig

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ const shell_integration = @import("shell_integration.zig");
2222
const terminal = @import("../terminal/main.zig");
2323
const termio = @import("../termio.zig");
2424
const Command = @import("../Command.zig");
25-
const SegmentedPool = @import("../datastruct/main.zig").SegmentedPool;
2625
const ptypkg = @import("../pty.zig");
2726
const Pty = ptypkg.Pty;
2827
const EnvMap = std.process.Environ.Map;
@@ -418,8 +417,9 @@ pub fn queueWrite(
418417
// our cached buffers that we can queue to the stream.
419418
var i: usize = 0;
420419
while (i < data.len) {
421-
const req = try exec.write_req_pool.getGrow(alloc);
422-
const buf = try exec.write_buf_pool.getGrow(alloc);
420+
const w = try exec.write_pool.create(alloc);
421+
w.td = exec;
422+
const buf = &w.buf;
423423
const slice = slice: {
424424
// The maximum end index is either the end of our data or
425425
// the end of our buffer, whichever is smaller.
@@ -459,26 +459,25 @@ pub fn queueWrite(
459459
exec.write_stream.queueWrite(
460460
td.loop,
461461
&exec.write_queue,
462-
req,
462+
&w.req,
463463
.{ .slice = slice },
464-
termio.Exec.ThreadData,
465-
exec,
464+
ThreadData.Write,
465+
w,
466466
ttyWrite,
467467
);
468468
}
469469
}
470470

471471
fn ttyWrite(
472-
td_: ?*ThreadData,
472+
w_: ?*ThreadData.Write,
473473
_: *xev.Loop,
474474
_: *xev.Completion,
475475
_: xev.Stream,
476476
_: xev.WriteBuffer,
477477
r: xev.WriteError!usize,
478478
) xev.CallbackAction {
479-
const td = td_.?;
480-
td.write_req_pool.put();
481-
td.write_buf_pool.put();
479+
const w = w_.?;
480+
w.td.write_pool.destroy(w);
482481

483482
const d = r catch |err| {
484483
log.err("write error: {}", .{err});
@@ -492,9 +491,21 @@ fn ttyWrite(
492491

493492
/// The thread local data for the exec implementation.
494493
pub const ThreadData = struct {
495-
// The preallocation size for the write request pool. This should be big
496-
// enough to satisfy most write requests. It must be a power of 2.
497-
const WRITE_REQ_PREALLOC = std.math.pow(usize, 2, 5);
494+
/// The state for a single queued pty write. The write request and
495+
/// the buffer it writes from must both remain pointer-stable until
496+
/// the write completes, so they're pooled together and checked out
497+
/// per write.
498+
pub const Write = struct {
499+
/// Backpointer to the thread data so the write completion
500+
/// callback can put this back into the pool.
501+
td: *ThreadData,
502+
503+
/// The libxev write request.
504+
req: xev.WriteRequest,
505+
506+
/// The buffer for the data being written.
507+
buf: [64]u8,
508+
};
498509

499510
/// Process start time and boolean of whether its already exited.
500511
start: std.Io.Timestamp,
@@ -506,12 +517,9 @@ pub const ThreadData = struct {
506517
/// The process watcher
507518
process: ?xev.Process,
508519

509-
/// This is the pool of available (unused) write requests. If you grab
520+
/// This is the pool of available (unused) write states. If you grab
510521
/// one from the pool, you must put it back when you're done!
511-
write_req_pool: SegmentedPool(xev.WriteRequest, WRITE_REQ_PREALLOC) = .{},
512-
513-
/// The pool of available buffers for writing to the pty.
514-
write_buf_pool: SegmentedPool([64]u8, WRITE_REQ_PREALLOC) = .{},
522+
write_pool: std.heap.MemoryPool(Write) = .empty,
515523

516524
/// The write queue for the data stream.
517525
write_queue: xev.WriteQueue = .{},
@@ -541,11 +549,10 @@ pub const ThreadData = struct {
541549
pub fn deinit(self: *ThreadData, alloc: Allocator) void {
542550
_ = posix.system.close(self.read_thread_pipe);
543551

544-
// Clear our write pools. We know we aren't ever going to do
552+
// Clear our write pool. We know we aren't ever going to do
545553
// any more IO since we stop our data stream below so we can just
546554
// drop this.
547-
self.write_req_pool.deinit(alloc);
548-
self.write_buf_pool.deinit(alloc);
555+
self.write_pool.deinit(alloc);
549556

550557
// Stop our process watcher
551558
if (self.process) |*p| p.deinit();

0 commit comments

Comments
 (0)