[pull] canary from vercel:canary - #1359
Merged
Merged
Conversation
Cache components are only compatible with Node.js runtime anyway. So this was basically dead code. Removing it doesn't break any existing functionality.
### What? `turbo-tasks-malloc` keeps a process-wide `ALLOCATED` atomic to answer `memory_usage()`. To optimize performance thread local buffers were maintained but this lead to code size issues. So instead we rely on mimalloc apis saving 1.6M of binary size ### Why not ask the OS? That was the obvious alternative, and it does not work well. The natural per-platform figures are not the same quantity: macOS `phys_footprint` and Windows `PrivateUsage` account for compressed and swapped pages, but Linux `VmRSS` does not — so under swap or zram the Linux number can read flat, or fall, while real consumption climbs. Closing that gap means `/proc/self/smaps_rollup` (`Pss + SwapPss`), which is not the cheap one-line read it looks like, it takes on the order of [100ms per 4GB](ncabatoff/process-exporter#246), and it would contend with other allocations in our process. ### How? `memory_usage()` reads `current_commit` from `mi_process_info`. We fall back to the `ALLOCATED` atomic only when `custom_allocator` is off. ### Behaviour changes **It does not track frees in lock step.** mimalloc reuses and purges pages on its own schedule or when eviction calls `collect` ### Size Rust inlines `#[global_allocator]` methods into every allocation site, so the buffer arithmetic and the branch guarding it were duplicated across the whole binary. Dropping them from the default build is where the win comes from. `libnext_napi_bindings.dylib`, release profile as shipped (thin LTO, `codegen-units = 1`). | Section | canary | this PR | Δ | |---|---:|---:|---:| | `__text` | 64,833,760 | 63,319,776 | **−1,513,984** | | `__eh_frame` | 8,966,904 | 8,748,464 | −218,440 | | `__gcc_except_tab` | 2,234,428 | 2,229,484 | −4,944 | | `__unwind_info` | 1,489,072 | 1,484,644 | −4,428 | | **Total** | | | **−1,741,796 = −1.66 MiB** | ### Performance `cargo bench -p turbo-tasks-malloc --bench allocation`, macOS aarch64, revisions interleaved across 3 rounds so drift hits both arms equally. Figures are the paired per-round deltas. | benchmark | canary | this PR | mean Δ | per-round | |---|---:|---:|---:|---| | `alloc_dealloc` | 6.65 ns | 6.45 ns | **−2.9%** | −3.7% .. −2.4% | | `alloc_realloc_dealloc` | 18.62 ns | 16.76 ns | **−10.0%** | −10.2% .. −9.7% | Removing the counter takes the buffer arithmetic and its guarding branch out of every allocation, and a realloc pays that twice — which is roughly the shape of the result. That said, the absolute numbers are small (a couple of hundred picoseconds on `alloc_dealloc`) and this benchmark is noisy, so treat the direction as the signal rather than the magnitude.
### What? Follow-up to #97761, which was auto-submitted before @bgw's review comments were addressed. Comment-only changes; no behavior change. - Name the nulled-out out-params in the `mi_process_info` call, applying the suggestion as written. - Trim the `memory_usage` doc comment to a brief description plus a link to the mimalloc docs. `mi_process_info` is absent from mimalloc's published doxygen docs, so the link points at the `libmimalloc-sys` rustdoc, which reproduces the header's table describing each figure. It is a plain URL rather than an intra-doc link because `libmimalloc-sys` is an optional dependency — an intra-doc link fails to resolve under `--no-default-features`. - Trim the test's doc comment to the single suggested line. The `#[global_allocator]` suggestion matched the existing code exactly, so nothing to change there. Left the `vec!` in the test alone, per "this is fine because every test in this crate should expect this global allocator". While in the same doc comment, dropped the `[`self::counter`]` link, which was raising a `private_intra_doc_links` warning. ### How? Verified with `cargo fmt --check`, `cargo clippy -p turbo-tasks-malloc --all-targets`, `cargo test -p turbo-tasks-malloc` (6 passed), and `cargo doc` under `RUSTDOCFLAGS="-D warnings"` with both default and `--no-default-features`.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )