Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new internity crate providing compact string interning with both a single-threaded (Lexicon) and concurrent (ThreadedLexicon) implementation, plus a frozen Reader for fast lock-free resolution. The PR also adds extensive tests, benchmarks, and performance documentation to support correctness and compare against other Rust interners.
Changes:
- Added
internitycrate (no-std + alloc baseline, optionalstdfor concurrency, optionalserde) withSymhandles and a sealedReaderabstraction. - Added correctness coverage via integration tests, property tests (Bolero), and concurrency modeling (Loom).
- Added benchmarking + reporting tooling (Criterion, Gungraun/Callgrind, memory footprint harness) and performance/comparison docs.
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds internity to the repo’s primary crates list. |
| CHANGELOG.md | Links the new internity crate changelog from the workspace changelog index. |
| Cargo.toml | Adds workspace dependency entries needed by internity (and competitors used in benches). |
| Cargo.lock | Records resolved dependencies for the new crate and benchmark competitors. |
| .spelling | Adds terminology used by internity docs/code (e.g., CSR, dedup, sharded). |
| crates/internity/Cargo.toml | Defines the new crate’s features, dependencies, benches, and Loom test target. |
| crates/internity/src/lib.rs | Crate root: public API exports, feature gating, and high-level documentation. |
| crates/internity/src/lexicon.rs | Implements the single-threaded interner with dense Sym encoding + freeze to Reader. |
| crates/internity/src/threaded_lexicon.rs | Implements the concurrent sharded interner and freeze-to-reader logic. |
| crates/internity/src/sym.rs | Defines Sym handle representation and encoding/decoding helpers. |
| crates/internity/src/reader.rs | Defines the sealed Reader trait for frozen, thread-safe resolution. |
| crates/internity/src/flat_reader.rs | Frozen reader implementation for Lexicon’s dense layout. |
| crates/internity/src/shard.rs | Shard wrapper using RwLock/upgradable-read for concurrent interning. |
| crates/internity/src/shard_write.rs | Shard write-state: dedup table + CSR offsets + byte buffer. |
| crates/internity/src/shard_reader.rs | Frozen per-shard storage and resolution routine. |
| crates/internity/src/sharded_reader.rs | Frozen reader implementation for ThreadedLexicon (shard + local decode). |
| crates/internity/src/storage.rs | Centralizes unchecked UTF-8 reconstruction for fast resolve paths. |
| crates/internity/src/symbol_map.rs | Adds Sym-keyed map/set support via a specialized hasher. |
| crates/internity/src/serde_impls.rs | Implements serde for Sym and interners (feature-gated). |
| crates/internity/tests/basic.rs | Broad integration tests covering API behavior, freezing, concurrency, serde, etc. |
| crates/internity/tests/bolero_internity.rs | Property tests for dedup/resolve/freezing and raw-handle range checks. |
| crates/internity/tests/loom_internity.rs | Loom model tests for concurrent dedup and snapshot behavior. |
| crates/internity/benches/compare.rs | Criterion wall-clock benchmark suite comparing multiple interners and thread counts. |
| crates/internity/benches/counts/main.rs | Gungraun/Callgrind harness entrypoint (Linux-only runner). |
| crates/internity/benches/counts/linux.rs | Deterministic instruction/cache-count benches for hot paths + competitors. |
| crates/internity/benches/mem.rs | Memory footprint benchmark using a tracking global allocator. |
| crates/internity/scripts/perf_report.rs | Script to run benches and regenerate docs/PERF.md. |
| crates/internity/docs/PERF.md | Generated performance report output committed to the repo. |
| crates/internity/docs/COMPARISON.md | Detailed comparative analysis across the Rust string interner ecosystem. |
| crates/internity/README.md | Crate README content for docs.rs / GitHub browsing. |
| crates/internity/CHANGELOG.md | Per-crate changelog for internity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Version increments look sufficient
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #604 +/- ##
=========================================
Coverage 100.0% 100.0%
=========================================
Files 443 461 +18
Lines 42764 44553 +1789
=========================================
+ Hits 42764 44553 +1789
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
crates/internity/src/de/mod.rs:48
- The reference-style rustdoc links include
ThreadedLexicon, which is not available without thestdfeature. This creates broken intra-doc links inserde-only (no_std) builds. After switching the earlier prose to non-linkedThreadedLexicontext, these reference links can be removed.
//! [`Sym`]: crate::Sym
//! [`Lexicon`]: crate::Lexicon
//! [`LocalLexicon`]: crate::LocalLexicon
//! [`ThreadedLexicon`]: crate::ThreadedLexicon
crates/internity/src/de/deserialize_in.rs:26
- The reference-style links for
ThreadedLexicon/ThreadedLexicon::deserialize_inare broken when thestdfeature is off. Since the prose can avoid linking toThreadedLexicon, these reference links should be removed to keep rustdoc clean inno_stdbuilds.
/// [`Lexicon`]: crate::Lexicon
/// [`LocalLexicon`]: crate::LocalLexicon
/// [`LocalLexicon::deserialize_in`]: crate::LocalLexicon::deserialize_in
/// [`ThreadedLexicon`]: crate::ThreadedLexicon
/// [`ThreadedLexicon::deserialize_in`]: crate::ThreadedLexicon::deserialize_in
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
crates/internity/src/sym.rs:100
- The workspace lints enable
clippy::unwrap_used, so thisexpect(...)triggers a warning (and CI denies warnings). Add an explicit#[expect(clippy::unwrap_used, reason = ...)]documenting that exceeding the u32 capacity is treated as a programming error / documented panic condition.
crates/internity/src/local_lexicon.rs:203 - The workspace lints enable
clippy::unwrap_used, so thisexpect(...)triggers a warning (and CI denies warnings). Add an explicit#[expect(clippy::unwrap_used, reason = ...)]onresolveto document that this is the intentional panicking variant.
#[inline]
#[must_use]
pub fn resolve(&self, sym: Sym) -> &str {
self.try_resolve(sym).expect("internity: Sym does not belong to this interner")
|
Second review pass at The previous custom-hasher serialization issue and duplicated-corpus insertion benchmark are fixed. Remaining/new findings:
Production unsafe remains isolated to |
ralfbiedert
left a comment
There was a problem hiding this comment.
Approved after the second-round review comment above.
|
Thanks for the thorough second pass. Addressed at
CI, Clippy, rustfmt, spellcheck, and doc2readme are green locally. |
|
@ralfbiedert — thanks again for the thorough second-round review. I've pushed changes (rebased onto latest 1. 2. 3. Serializing 4. Same-shard dedup hits on the single upgradable-read slot. Kept the current fill-oriented path per your guidance (the read-first/recheck variant regressed insertion, the crate's primary workload) and documented the tradeoff plus the "freeze before a read-heavy phase" guidance. 6. Loom tests framed as production model-checking. Renamed All tests pass across feature combinations and CI is green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 50 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/internity/docs/DESIGN.md:245
- The
freezedocumentation here says the shared/"snapshot" path walks shards one at a time and is not point-in-time, but the implementation holds read guards on all shards before copying any of them (seeThreadedLexiconInner::build_reader). This is a point-in-time snapshot; the doc should match the code to avoid misleading readers.
The snapshot path acquires read guards on **all** shards up front, before any
blob is copied. While every guard is held no concurrent `intern` can commit (a
miss cannot upgrade to the write lock while a reader is present), so the copied
state is a single **point-in-time snapshot** even when other clones are still
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 50 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/internity_macros/src/lib.rs:531
- The generated
Visitor::visit_seqfor named structs reads the declared fields but never checks for trailing elements, so inputs like[field0, ..., fieldN, extra]are silently accepted. This diverges fromserde::Deserializebehavior and undermines thetrailing_sequence_fields_are_rejectedintegration test expectations. After reading the expected fields, probe for one extraIgnoredAnyelement and returninvalid_lengthif present.
Add compact local and concurrent string interners with frozen, lock-free readers, no-std and Serde support, property tests, Loom models, and Criterion, Gungraun, and memory benchmarks. Add interner-aware deserialization so items can deserialize directly into a chosen LocalLexicon or ThreadedLexicon while converting string fields to Sym handles. Lookup benchmarks use 60,000 distinct identifier-like strings via `INTERNITY_BENCH_CORPUS_SIZE=60000`; memory benchmarks use the default 6,000 strings. Lower is better. | Lookup implementation | Time | |---|---:| | internity (live) | 234.85 us | | internity (frozen) | 214.25 us | | lasso | 236.60 us | | string-interner | 309.74 us | | symbol_table | 574.28 us | | string_cache | 269.66 us | | Memory implementation | Handle | Filled | Lookup | |---|---:|---:|---:| | internity LocalLexicon | 4 B | 172.0 KiB | 96.5 KiB | | internity ThreadedLexicon | 4 B | 181.1 KiB | 98.8 KiB | | lasso | 4 B | 264.2 KiB | 224.2 KiB | | string-interner | 4 B | 204.0 KiB | 204.0 KiB | | symbol_table | 4 B | 241.3 KiB | 241.3 KiB | | string_cache | 8 B | 351.7 KiB | 351.7 KiB | Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: df123253-43b0-4ae3-82c1-6e9f88846881
Add compact single-threaded and concurrent string interners with frozen, lock-free readers, no-std and Serde support, property tests, Loom models, and Criterion, Gungraun, and memory benchmarks.
Lookup benchmarks use 60,000 distinct identifier-like strings via
INTERNITY_BENCH_CORPUS_SIZE=60000; memory benchmarks use the default 6,000 strings. Lower is better.