Skip to content

Eng 1510 get rid of client data - #19

Merged
Michelprogram merged 10 commits into
mainfrom
dorian/ENG-1510-get-rid-of-client-data
Oct 9, 2026
Merged

Michelprogram merged 10 commits into
mainfrom
dorian/ENG-1510-get-rid-of-client-data

Conversation

@Michelprogram

Copy link
Copy Markdown
Collaborator

Fixes ENG-1510

@linear-code

linear-code Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
ENG-1510 Get rid of set_client_data & get_client_data

libsqlite3-sys does not publish a version that contain both client_data.
Using last version from Github involve upgrade version of diesel, sqlxite, diesel_migration

Review in Linear

@Michelprogram
Michelprogram changed the base branch from main to dorian/ENG-1507-move-reglite September 29, 2026 17:11
@Michelprogram
Michelprogram added this pull request to stack #20 September 29, 2026 17:11
Comment thread crates/sqlite-ffi/src/client_data.rs
Comment thread sqlite-compress/src/state.rs Outdated
Base automatically changed from dorian/ENG-1507-move-reglite to main October 5, 2026 12:08
@Michelprogram
Michelprogram force-pushed the dorian/ENG-1510-get-rid-of-client-data branch from c50445c to 3406f9a Compare October 5, 2026 12:08
@Michelprogram

Copy link
Copy Markdown
Collaborator Author

Thats not really what I was talking about, I was essentially trying to see if we could have one DictCache for all the connections where the inner items would be Arc<EncoderDictionary<'static>> so we can quickly clone them for a compression. We could also basically drop the LRU or make it way bigger.

so i did the shared process-wide dict pool for all connections an i replaced the LRU with a HashMap (cap 64) on overflow we clear. That's simpler than LRU acceptable miss cost for our size and Per-connection state is only current_ids via client data

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The global cache can reuse dictionaries across unrelated databases, ignore compression levels, and grow beyond its intended limit.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
What changed in this PR

Moves prepared Zstandard dictionaries from per-connection LRU caches into a shared process-wide pool.

Changes:

  • Adds global encoder and decoder caches.
  • Retains only current dictionary IDs per connection.
  • Serializes the crate’s tests.
File Description
.mise/​tasks/​sqlite-compress/​test Runs tests serially.
sqlite-compress/​src/​cache/​connection.rs Removes prepared dictionaries from connection state.
sqlite-compress/​src/​cache/​lru.rs Deletes the per-connection LRU implementation.
sqlite-compress/​src/​cache/​mod.rs Registers the new pool module.
sqlite-compress/​src/​cache/​pool.rs Implements global prepared-dictionary storage.
sqlite-compress/​src/​cache/​prepared.rs Routes dictionary operations through the global pool.
sqlite-compress/​src/​state.rs Removes formatting whitespace.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread sqlite-compress/src/cache/pool.rs
Comment thread sqlite-compress/src/cache/pool.rs Outdated
Comment thread sqlite-compress/src/cache/pool.rs Outdated
@Michelprogram
Michelprogram requested a review from Sytten October 7, 2026 06:41
Entry::Vacant(e) => {
if at_cap {
let key = e.into_key();
map.clear();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clears the whole map at cap, you sure it is what we want?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep the downside of this is that you need to fill it again with a lot of cache misses but i don't think we'll be doing clear that much. i mean the cost of having an LRU instead is acceptable

@Michelprogram
Michelprogram requested a review from Sytten October 8, 2026 07:49
Sytten
Sytten previously approved these changes Oct 8, 2026
@Michelprogram
Michelprogram merged commit 4c4f6e3 into main Oct 9, 2026
2 of 8 checks passed
@Michelprogram
Michelprogram deleted the dorian/ENG-1510-get-rid-of-client-data branch October 9, 2026 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants