Skip to content

Account for hybrid cache entry overhead - #104

Merged
mjc merged 3 commits into
nntpp-28-stateful-connection-lifecyclefrom
nntpp-29-hybrid-cache-weight
Sep 11, 2026
Merged

Account for hybrid cache entry overhead#104
mjc merged 3 commits into
nntpp-28-stateful-connection-lifecyclefrom
nntpp-29-hybrid-cache-weight

Conversation

@mjc

@mjc mjc commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • charge hybrid cache entries for key, metadata, and encoded payload framing
  • keep capacity accounting aligned with retained memory rather than payload bytes alone

Branch-added tests

  • A focused weigher fixture compares the exact encoded entry size with the key and in-memory metadata overhead and verifies the hybrid weight includes all components.
  • Existing cache serialization and capacity tests remain green.

Copilot AI lite review requested due to automatic review settings September 5, 2026 21:48
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1c401e2a-f53c-45d6-95de-fabe481846d0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new weigher/test currently uses String::len() instead of capacity(), which can undercount retained heap memory and misalign eviction/capacity accounting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates hybrid cache capacity accounting so entry weights reflect not only article payload bytes, but also key storage, in-memory metadata, and serialized framing overhead—keeping eviction behavior better aligned with actual retained footprint.

Changes:

  • Introduce a shared hybrid_entry_weight used by both production and memory-only hybrid cache builders.
  • Add DiskCachedArticle::encoded_len() to measure serialized value size including metadata and payload framing.
  • Add a focused unit test to validate the new weight calculation behavior.
File summaries
File Description
src/cache/hybrid.rs Switches foyer weighter from payload-only to a composite entry weight and adds a unit test for the new accounting.
src/cache/hybrid_codec.rs Adds encoded_len() and a fixed-size constant to represent serialized entry overhead for weight calculations.
Review details

Suppressed comments (1)

src/cache/hybrid.rs:720

  • This test hard-codes short_key.len() in the expected weight. If the weighter is meant to track retained memory, it should use capacity() (matching the production code) so the assertion remains correct for strings with reserved capacity.
            size_of::<String>()
                + short_key.len()
                + size_of::<DiskCachedArticle>()
                + missing.encoded_len()
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment thread src/cache/hybrid.rs Outdated
@mjc
mjc force-pushed the nntpp-29-hybrid-cache-weight branch from c15f877 to b5db963 Compare September 5, 2026 23:28
@mjc
mjc force-pushed the nntpp-28-stateful-connection-lifecycle branch from d6fbd37 to 4f3244a Compare September 5, 2026 23:28
@mjc
mjc merged commit 29c5697 into nntpp-28-stateful-connection-lifecycle Sep 11, 2026
8 checks passed
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.

2 participants