perf: write state directly to avoid temporary allocations - #21
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
MeteorsLiu
changed the title
MeteorsLiu
changed the title
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: streaming memfd output
This is a clean, well-reasoned refactor. It replaces the mmap grow-and-retry buffer with a streaming bufio.Writer over a pwrite-backed imageWriter, and adds a shared save(mem, out, root) path so Save/SaveTo stay DRY.
Verified correct:
- Header/size accounting in
writeStateImageis consistent (reserve zero header atoffset, write payload ofn, zero the following header, then publishsize = n + 8last). MatchesreadStateImageand the tests. - The
io.Writershort-write contract inwriter.writeBytes/writeStringis honored (panic onio.ErrShortWrite, converted to error viasafely). - Security: no findings. Offset/size overflow guards hold, the host still reads its result from the host-retained offset (not the guest-writable input header), the unpublished-header invariant survives the new layout, and a failed guest write leaves a zero header that
readStateImagerejects.
Inline comments below are minor/tuning suggestions, none blocking.
Also minor (not inlined): writeStateImage and imageWriter.Write emit the same "sandbox image size overflow" message from two sites, making a failure ambiguous — consider differentiating the two messages.
| if err := unix.Ftruncate(fd, end); err != nil { | ||
| return 0, err | ||
| } | ||
| buffer := bufio.NewWriter(&output) |
There was a problem hiding this comment.
bufio.NewWriter uses the default 4 KB buffer, so the payload flushes to pwrite every 4 KB. Since the encoder emits many tiny writes (per-object tag + varints), buffering is load-bearing here — but for large state images a bigger buffer (e.g. bufio.NewWriterSize(&output, 64<<10)) would materially cut the number of pwrite syscalls at negligible memory cost. Tuning only, not a correctness issue.
| } | ||
| mem, err := unix.Mmap(fd, mapOffset, delta+n+16, unix.PROT_READ|unix.PROT_WRITE, unix.MAP_SHARED) | ||
| if err != nil { | ||
| // An unwritten result must keep its following header unpublished. |
There was a problem hiding this comment.
This comment reads as a prohibition ("must keep ... unpublished"), but the next line actively writes an all-zero header at the following image's offset. The mechanism is proactive zeroing so a reader walking to the next offset sees a zero-length (rejected) image. Consider rewording to describe the action, e.g. "Zero the following image's header so the next result reads as unpublished until it is written."
| n = max(n, 0) | ||
| w.offset += int64(n) | ||
| if err == nil && n != len(p) { | ||
| err = io.ErrShortWrite |
There was a problem hiding this comment.
A short Pwrite (err == nil && n != len(p)) is turned into io.ErrShortWrite and returned immediately, after advancing w.offset by the partial n. For a memfd on tmpfs this effectively never happens, so it's safe in practice — but since imageWriter is a general io.Writer fed through bufio, the more robust choice is to loop on the remainder (p = p[n:]) and only give up when Pwrite returns n == 0, nil. As-is, the partial-write branch is essentially unreachable/untested and would advance the offset past what was persisted. A one-line comment noting the reliance on tmpfs semantics would help if you keep it.
1 participant
No description provided.