Skip to content

chore: forbid the locking Id constructors with a clippy lint#8617

Merged
robert3005 merged 1 commit into
vortex-data:developfrom
miniex:perf/disallow-id-new-lint
Jun 30, 2026
Merged

chore: forbid the locking Id constructors with a clippy lint#8617
robert3005 merged 1 commit into
vortex-data:developfrom
miniex:perf/disallow-id-new-lint

Conversation

@miniex

@miniex miniex commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #8614. A clippy disallowed-methods lint now forbids Id::new and Id::new_static workspace-wide, since both intern into a globally locked table (ThreadedRodeo) on every call. Static identifiers must use a CachedId static instead; the sites that intern a genuinely dynamic string (proto/flatbuffer deserialization, arrow extension lookups, Python read contexts, tests) keep Id::new behind an #[expect(clippy::disallowed_methods, reason = "...")]. CachedId itself now interns through Id::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-targets is clean across the edited crates (vortex-array, vortex-layout, vortex-file, vortex-ipc, vortex-zstd, vortex-python, vortex-duckdb, vortex-session), with disallowed_methods and unfulfilled_lint_expectations both at zero; cargo +nightly fmt --all --check clean. vortex-cuda could not be built locally (no CUDA toolchain).


I'm Korean, so sorry if any wording reads a little awkward.

@miniex miniex marked this pull request as ready for review June 29, 2026 11:08
@miniex miniex requested a review from a team June 29, 2026 11:08
@miniex miniex force-pushed the perf/disallow-id-new-lint branch from b418f23 to f58aa0b Compare June 29, 2026 11:09
@codspeed-hq

codspeed-hq Bot commented Jun 29, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 1593 untouched benchmarks
⏩ 4 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation slice_empty_vortex 339.4 ns 397.8 ns -14.66%
Simulation bitwise_not_vortex_buffer_mut[128] 273.6 ns 244.4 ns +11.93%

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 (74b8c39) with develop (4a90e13)

Open in CodSpeed

Footnotes

  1. 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.

@robert3005 robert3005 added the changelog/chore A trivial change label Jun 30, 2026
@robert3005

Copy link
Copy Markdown
Contributor

I think you need to rebase and fix

error: use of a disallowed method `vortex_session::registry::Id::new`
   --> vortex-python-cuda/src/lib.rs:191:23
    |
191 |     let encoding_id = ArrayId::new(&metadata.encoding_id);
    |                       ^^^^^^^^^^^^
    |
    = note: Interning a static id on every call grabs the interner lock (#8380). Use a `CachedId` static for static ids; for a dynamic string, annotate the call with `#[expect(clippy::disallowed_methods)]`.
    = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#disallowed_methods
    = note: `-D clippy::disallowed-methods` implied by `-D clippy::all`
    = help: to override `-D clippy::all` add `#[allow(clippy::disallowed_methods)]`

@miniex miniex force-pushed the perf/disallow-id-new-lint branch from f58aa0b to 301c6d9 Compare June 30, 2026 11:46
`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>
@miniex miniex force-pushed the perf/disallow-id-new-lint branch from 301c6d9 to 74b8c39 Compare June 30, 2026 11:48
@miniex miniex requested a review from robert3005 June 30, 2026 11:48
@robert3005 robert3005 merged commit 3077131 into vortex-data:develop Jun 30, 2026
72 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants