Conversation
Three tests in countvector.rs and gaugevector.rs used the same "count_vector_test" MMV filename. If run in parallel, tests could interleave and fail. Give each test its own unique MMV filename.
Client::export used Write::write, which is only guaranteed to write some of the buffer, to pre-size the mmap'd MMV file with zeros. A short write would leave the file smaller than mmv_size, silently corrupting everything mapped after it. clippy already flags this as unused_io_amount (deny-by-default). Use write_all instead, which loops until the whole buffer is written or an error occurs. Assisted-by: Cursor:claude-opus-5
Drops four third-party dependencies in favor of std, reducing maintenance surface and modernizing away from older/unmaintained crates. byteorder: replaced by a ReadBytesExt/WriteBytesExt pair in src/byteio. The old code hardcoded LittleEndian, which was technically incorrect: looking at mmv src code, MMV files should use native-endian byte order. Hence the new code will also use native order. regex: existed only to parse PCP_KEY=value lines out of pcp.conf, now a hand-rolled byte-slice parser with the same semantics. nix/kernel32-sys: get_process_id had one #[cfg] arm per platform, both replaced by std::process::id. time: Timer moves from wall-clock Tm/Duration to the monotonic std::time::Instant. Also improves windows arm for osstr_from_bytes which asserted pcp.conf holds valid UTF-8 via from_utf8_unchecked. It now checks and skips the line instead. Assisted-by: Cursor:claude-opus-5
memmap 0.5.x is unmaintained (last release 2016) and had known soundness issues around unmapping memory. memmap2 is the actively maintained successor. memmap 0.5's MmapViewSync let a single mapping be split into independently-owned "views" over disjoint sub-ranges (via split_at), which is how each Metric/Instance got a private handle to just its own value's bytes for later updates. memmap2 has no equivalent - it just hands out one MmapMut for the whole mapping and expects callers to handle everything else themselves. Replaced that with a small MmapView type (in client::metric::private, alongside MMVWriterState): an Arc<Mutex<MmapMut>> plus an offset/len, cheaply cloneable and slice-able. It will now be used to hand out sub-views, with the Mutex taken for the read/write. This is different from the old design in one respect: writes to disjoint sub-views are now serialized through a shared lock rather than being raw pointer writes. This is strictly safer and shouldn't be observable functionally. Client::export() now maps the freshly-created file with memmap2::MmapMut::map_mut and takes the lock once, via MmapView::lock_whole, for the duration of the header/TOC/metric writing. The old code instead took an unsafe aliasing clone of the view and called as_mut_slice on it, so the whole-file cursor and the per-value views were two overlapping mutable handles to one mapping; holding the lock replaces that with a single writer. No API changes for consumers of the crate. Assisted-by: Cursor:claude-opus-5
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Reduces the dependency tree (using
stdinstead) and migrates from deprecated mmap to mmap2 crate. Includes two correctness fixes.Further modernization of rust version/dependencies coming in the future.