Skip to content

feat: Make NormalizedPath interning cache configurable#343

Merged
nielsenko merged 2 commits intoserverpod:mainfrom
nielsenko:fix-make-interning-configurable
Mar 10, 2026
Merged

feat: Make NormalizedPath interning cache configurable#343
nielsenko merged 2 commits intoserverpod:mainfrom
nielsenko:fix-make-interning-configurable

Conversation

@nielsenko
Copy link
Collaborator

@nielsenko nielsenko commented Mar 10, 2026

Introduce abstract Cache<K, V> interface with LruCache and NoCache implementations. NormalizedPath.interned can now be swapped to tune caching strategy for different workloads.

Fixes #342, fixes #118

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced configurable caching strategies for route normalization: choose between LRU caching for performance or opt out of caching for high-cardinality workloads.
  • Tests

    • Added comprehensive tests validating caching behavior and path normalization across different strategies.

Introduce abstract Cache<K, V> interface with LruCache and NoCache
implementations. NormalizedPath.interned can now be swapped to tune
caching strategy for different workloads.

Fixes serverpod#342, fixes serverpod#118
@nielsenko nielsenko self-assigned this Mar 10, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: fc897641-743a-4d76-9345-e3deda403b17

📥 Commits

Reviewing files that changed from the base of the PR and between 3925db6 and f1044f8.

📒 Files selected for processing (7)
  • .github/workflows/ci.yaml
  • packages/relic_core/lib/relic_core.dart
  • packages/relic_core/lib/src/router/cache.dart
  • packages/relic_core/lib/src/router/lru_cache.dart
  • packages/relic_core/lib/src/router/no_cache.dart
  • packages/relic_core/lib/src/router/normalized_path.dart
  • packages/relic_core/test/router/no_cache_test.dart

📝 Walkthrough

Walkthrough

This PR introduces a pluggable caching abstraction for the NormalizedPath interning mechanism. It defines a Cache interface, implements a NoCache no-op strategy, updates LruCache to conform to the interface, and refactors NormalizedPath to use Cache as the interned field type. The changes allow runtime substitution of caching implementations to address high-cardinality path performance cliffs.

Changes

Cohort / File(s) Summary
CI Configuration
.github/workflows/ci.yaml
Updated dart-lang/setup-dart action from v1.3 to v1.
Cache Abstraction
packages/relic_core/lib/src/router/cache.dart, packages/relic_core/lib/src/router/no_cache.dart, packages/relic_core/lib/src/router/lru_cache.dart
Added abstract Cache<K, V> interface with bracket operators and length getter. Implemented NoCache<K, V> as a no-op cache (always returns null, length 0). Updated LruCache<K, V> to implement Cache interface.
Path Interning Integration
packages/relic_core/lib/src/router/normalized_path.dart
Typed the static interned field as Cache<String, NormalizedPath> (previously var). Maintained LruCache<String, NormalizedPath>(10000) as default. Extended documentation to describe configurable caching via NoCache or custom implementations.
Public API Exports
packages/relic_core/lib/relic_core.dart
Added public exports for cache.dart and no_cache.dart to expose the new abstractions.
Test Suite
packages/relic_core/test/router/no_cache_test.dart
Added comprehensive test coverage for NoCache behavior, NormalizedPath with disabled interning, and LruCache eviction semantics with small capacity.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • feat: Router class #52: Introduces cache interface infrastructure that this PR extends by creating pluggable implementations (NoCache, LruCache conformance) for NormalizedPath interning.
  • feat!: Custom CacheControl header per file #167: Related cache abstraction work that updates cache-based type usages and per-file cache behavior alongside router caching types.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes one out-of-scope change: updating the Dart setup action version in CI/CD workflow (.github/workflows/ci.yaml), which is unrelated to the caching objectives. Remove the CI/CD workflow version update from this PR and submit it as a separate change to keep scope focused on the caching interface and implementations.
Description check ❓ Inconclusive The description is minimal but covers the key changes (Cache interface with implementations) and links to relevant issues; however, it lacks detail on breaking changes, testing, and other template sections. Expand description to address template sections: document breaking changes status, confirm tests were added, and provide more context on implementation rationale if applicable.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main objective of the PR: making the NormalizedPath interning cache configurable.
Linked Issues check ✅ Passed The PR implements core coding requirements from both issues: introduces configurable caching via Cache<K, V> interface, provides LruCache and NoCache implementations, and exposes NormalizedPath.interned for tuning cache strategies.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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 and usage tips.

@nielsenko
Copy link
Collaborator Author

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@codecov
Copy link

codecov bot commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.21%. Comparing base (3925db6) to head (f1044f8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/relic_core/lib/src/router/no_cache.dart 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   91.20%   91.21%   +0.01%     
==========================================
  Files          97       98       +1     
  Lines        3844     3849       +5     
  Branches     1960     1964       +4     
==========================================
+ Hits         3506     3511       +5     
  Misses        338      338              
Flag Coverage Δ
relic_core 90.93% <83.33%> (+0.01%) ⬆️
relic_io 93.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nielsenko
Copy link
Collaborator Author

Please disregard coverage drop. Dart doesn't actually call an empty ctor. It only exist to mark it const.

image

@nielsenko
Copy link
Collaborator Author

While this makes caching configurable, we should investigate if we can make no-interning even cheaper. Dart side caches cannot be shared across isolates, which makes them less useful in multi-isolate deployments.

@CodeRabbit Please create a follow-up issue.

@nielsenko nielsenko marked this pull request as ready for review March 10, 2026 16:22
@nielsenko nielsenko requested a review from a team March 10, 2026 16:23
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo serverpod/relic \
  --title "Investigate cheaper no-interning and multi-isolate cache sharing for NormalizedPath" \
  --assignee nielsenko \
  --body "## Background

PR `#343` makes the \`NormalizedPath\` interning cache configurable (via the new \`Cache<K, V>\` interface, \`LruCache\`, and \`NoCache\` implementations). While this addresses the performance cliff identified in `#342` and `#118`, a follow-up investigation is needed.

## Problem

- The current approach (even with \`NoCache\`) may not be as cheap as possible when interning is disabled. There may be room to make the no-interning path even lighter.
- Dart-side caches (including \`LruCache\`) **cannot be shared across isolates**. This significantly limits their effectiveness in multi-isolate deployments, where each isolate maintains its own separate cache with no shared benefit.

## Tasks

- [ ] Investigate whether the no-interning (\`NoCache\`) code path can be made even cheaper (e.g., avoiding unnecessary allocations or lookups entirely).
- [ ] Investigate options for cache sharing or coordination across isolates (e.g., using \`SendPort\`/\`ReceivePort\`, native code, or alternative architectural approaches).
- [ ] Document the multi-isolate limitation clearly in the public API if no sharing solution is feasible.

## References

- PR: https://github.com/serverpod/relic/pull/343
- Comment: https://github.com/serverpod/relic/pull/343#issuecomment-\$(gh pr view 343 --repo serverpod/relic --json comments --jq '.comments[-1].id' 2>/dev/null || echo 'see-pr-343')
- Fixes `#342`, `#118`

## Requested by

`@nielsenko`"

Length of output: 45


@nielsenko: Done! I've created the follow-up issue at #344 — assigned to you and covering:

  • Investigating whether the NoCache path can be made even cheaper (e.g., avoiding unnecessary allocations/lookups entirely).
  • Exploring options for cache sharing or coordination across isolates.
  • Documenting the multi-isolate limitation in the public API if no sharing solution is found.

🧠 Learnings used
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: lib/src/handler/handler.dart:59-67
Timestamp: 2025-04-25T07:39:38.915Z
Learning: Nielsenko prefers using switch statements with pattern matching over if statements when working with sealed classes in Dart, as they provide exhaustiveness checking at compile time and can be more concise.

Copy link
Collaborator

@marcelomendoncasoares marcelomendoncasoares left a comment

Choose a reason for hiding this comment

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

LGTM!

@nielsenko nielsenko merged commit b11216b into serverpod:main Mar 10, 2026
30 of 31 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.

Performance cliff on high-cardinality dynamic paths due to NormalizedPath interning Improve interning of NormalizedPath

2 participants