chore: forbid the locking Id constructors with a clippy lint#8617
chore: forbid the locking Id constructors with a clippy lint#8617miniex wants to merge 1 commit into
Conversation
`Id::new`/`Id::new_static` intern into a globally locked table on every call. A disallowed-methods lint now forbids them: static ids use a `CachedId` static, and the few dynamic-string sites keep `Id::new` behind `#[expect]`. `CachedId` interns via `Id::new_static`. Refs: vortex-data#8380 Signed-off-by: Han Damin <miniex@daminstudio.net>
b418f23 to
f58aa0b
Compare
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
168.9 µs | 205.7 µs | -17.89% |
| ❌ | Simulation | slice_empty_vortex |
339.4 ns | 397.8 ns | -14.66% |
| ⚡ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
26.9 µs | 15.7 µs | +71.06% |
| ⚡ | Simulation | chunked_varbinview_canonical_into[(100, 100)] |
259.5 µs | 224.3 µs | +15.69% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(100, 100)] |
306.8 µs | 271.9 µs | +12.82% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
273.6 ns | 244.4 ns | +11.93% |
| ⚡ | Simulation | eq_i64_constant |
318.9 µs | 288.2 µs | +10.68% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing miniex:perf/disallow-id-new-lint (f58aa0b) with develop (733ab9e)
Footnotes
-
4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Summary
Follow-up to #8614. A
clippydisallowed-methods lint now forbidsId::newandId::new_staticworkspace-wide, since both intern into a globally locked table (ThreadedRodeo) on every call. Static identifiers must use aCachedIdstatic instead; the sites that intern a genuinely dynamic string (proto/flatbuffer deserialization, arrow extension lookups, Python read contexts, tests) keepId::newbehind an#[expect(clippy::disallowed_methods, reason = "...")].CachedIditself now interns throughId::new_static, so it is the single sanctioned path for static ids.This is the enforcement half of #8380; #8614 did the migration. It is stacked on #8614, so it stays a draft until that merges; the shared migration commit drops out once this is rebased onto a merged #8614.
Refs: #8380
Testing
cargo clippy --all-targetsis clean across the edited crates (vortex-array,vortex-layout,vortex-file,vortex-ipc,vortex-zstd,vortex-python,vortex-duckdb,vortex-session), withdisallowed_methodsandunfulfilled_lint_expectationsboth at zero;cargo +nightly fmt --all --checkclean.vortex-cudacould not be built locally (no CUDA toolchain).I'm Korean, so sorry if any wording reads a little awkward.