perf(pm): parse version manifests from vec buffers#3031
Conversation
There was a problem hiding this comment.
Code Review
This pull request optimizes manifest parsing by transitioning from Bytes to Vec<u8>, allowing simd_json to perform in-place mutation and avoiding unnecessary memory copies. Review feedback identified that simd_json requires specific padding (SIMDJSON_PADDING) to prevent out-of-bounds reads, which was missing in the implementation. Recommendations include resizing the buffer with the necessary padding before parsing and pre-allocating this space during the initial response body read to further optimize performance.
| rayon::spawn(move || { | ||
| let mut parse_buf = bytes.to_vec(); | ||
| let result = simd_json::serde::from_slice::<T>(&mut parse_buf) | ||
| .map_err(|e| anyhow!("JSON parse error: {e}")); |
There was a problem hiding this comment.
The simd_json parser requires the input buffer to have a specific amount of padding (defined by simd_json::SIMDJSON_PADDING) beyond the end of the JSON string to perform safe SIMD reads. Without this padding, the parser may perform out-of-bounds reads, which is undefined behavior. Since simd_json::serde::from_slice mutates the buffer in-place, you should resize the Vec<u8> to include this padding (using spaces to avoid trailing character errors) before parsing.
| rayon::spawn(move || { | |
| let mut parse_buf = bytes.to_vec(); | |
| let result = simd_json::serde::from_slice::<T>(&mut parse_buf) | |
| .map_err(|e| anyhow!("JSON parse error: {e}")); | |
| rayon::spawn(move || { | |
| parse_buf.resize(parse_buf.len() + simd_json::SIMDJSON_PADDING, b' '); | |
| let result = simd_json::serde::from_slice::<T>(&mut parse_buf) | |
| .map_err(|e| anyhow!("JSON parse error: {e}")); |
| simd_json::serde::from_slice::<T>(&mut parse_buf) | ||
| .map_err(|e| anyhow!("JSON parse error: {e}")) |
There was a problem hiding this comment.
Similar to the native path, the wasm32 implementation should also ensure the input buffer has the required simd_json::SIMDJSON_PADDING to avoid potential out-of-bounds reads during parsing.
parse_buf.resize(parse_buf.len() + simd_json::SIMDJSON_PADDING, b' ');
simd_json::serde::from_slice::<T>(&mut parse_buf)
.map_err(|e| anyhow!("JSON parse error: {e}"))| .content_length() | ||
| .and_then(|len| usize::try_from(len).ok()) | ||
| .unwrap_or(0); | ||
| let mut body = Vec::with_capacity(capacity); |
There was a problem hiding this comment.
To optimize the performance of the subsequent JSON parsing, you can pre-allocate the required simd_json::SIMDJSON_PADDING in the Vec. This avoids a potential reallocation when resize is called later in the parsing functions.
| let mut body = Vec::with_capacity(capacity); | |
| let mut body = Vec::with_capacity(capacity + simd_json::SIMDJSON_PADDING); |
bd39706 to
b96d81c
Compare
b96d81c to
604aa30
Compare
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 8.99s | 0.12s | 10.38s | 9.99s | 673M | 313.7K |
| utoo-next | 8.87s | 0.99s | 10.65s | 12.23s | 980M | 122.4K |
| utoo-npm | 8.08s | 0.03s | 10.43s | 12.05s | 989M | 125.9K |
| utoo | 7.82s | 0.06s | 10.42s | 11.87s | 970M | 128.1K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 16.9K | 19.4K | 1.19G | 7M | 1.86G | 1.75G | 1M |
| utoo-next | 128.3K | 91.3K | 1.16G | 6M | 1.71G | 1.70G | 2M |
| utoo-npm | 126.3K | 89.9K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo | 122.1K | 76.8K | 1.16G | 5M | 1.71G | 1.70G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.16s | 0.06s | 3.93s | 1.16s | 506M | 186.9K |
| utoo-next | 2.96s | 0.04s | 5.10s | 1.85s | 609M | 80.9K |
| utoo-npm | 3.10s | 0.03s | 5.17s | 2.21s | 601M | 74.7K |
| utoo | 2.93s | 0.07s | 5.03s | 1.87s | 612M | 86.9K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 10.8K | 4.1K | 203M | 3M | 108M | - | 1M |
| utoo-next | 53.3K | 71.5K | 200M | 3M | 7M | 3M | 2M |
| utoo-npm | 76.8K | 97.3K | 200M | 2M | 7M | 3M | 2M |
| utoo | 52.2K | 74.2K | 200M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.72s | 0.23s | 6.34s | 9.67s | 566M | 200.1K |
| utoo-next | 6.05s | 0.17s | 5.02s | 10.46s | 486M | 64.4K |
| utoo-npm | 5.83s | 0.01s | 5.13s | 10.47s | 465M | 58.8K |
| utoo | 5.83s | 0.15s | 5.10s | 10.32s | 519M | 60.9K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 7.1K | 7.4K | 1019M | 4M | 1.76G | 1.76G | 1M |
| utoo-next | 101.8K | 48.7K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo-npm | 102.7K | 49.3K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo | 97.1K | 54.0K | 989M | 3M | 1.70G | 1.70G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.33s | 0.06s | 0.19s | 2.44s | 136M | 32.4K |
| utoo-next | 2.39s | 0.22s | 0.53s | 3.79s | 79M | 18.1K |
| utoo-npm | 2.41s | 0.11s | 0.48s | 3.82s | 79M | 18.8K |
| utoo | 2.29s | 0.03s | 0.50s | 3.78s | 79M | 18.8K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 249 | 84 | 5M | 46K | 1.91G | 1.75G | 1M |
| utoo-next | 42.4K | 19.4K | 18K | 25K | 1.70G | 1.70G | 2M |
| utoo-npm | 42.0K | 19.6K | 15K | 10K | 1.70G | 1.70G | 2M |
| utoo | 41.8K | 19.4K | 14K | 10K | 1.71G | 1.70G | 2M |
npmmirror.com: no output captured.
Summary
Review-sized split from #3028 / #2948, stacked after #3030.
Optimizes exact-version manifest parsing by keeping the HTTP body as the mutable
Vec<u8>thatsimd_jsonneeds:parse_json_vec_off_runtime;Vec<u8>on native targets;fetch_version_manifest_bytesas a compatibility wrapper;This is the zero-copy hot path piece for exact-version manifest fetches; full manifests still keep immutable
Bytesbecause their raw payload is reused for later version extraction.Size
1 file changed, 75 insertions(+), 11 deletions(-)Validation
cargo fmtcargo check -p utoo-ruboristcargo test -p utoo-ruborist service::manifest::tests::parse_json_vec_off_runtime_consumes_mutable_buffercargo clippy --all-targets -- -D warnings --no-depspack-napistill warns locally becausenext.jsis a symlink in this worktree; clippy exits successfully.