Skip to content

Conversation

@twilson63
Copy link
Contributor

No description provided.

Copy link
Collaborator

@TillaTheHun0 TillaTheHun0 left a comment

Choose a reason for hiding this comment

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

LGTM. Nicely done.

I could see a case for caching full SU results, instead of separates bits of the result and reduce calls to the SUs (the current PR will dupe calls to the SUs), but then we'd need to worry about the potential for more cache evictions, which could cause either thrashing or high memory pressure, depending on configuration.

Without further exploration, the approach in the PR -- resulting in dupe calls -- is most likely the safest bet.

@TillaTheHun0
Copy link
Collaborator

TillaTheHun0 commented Jan 18, 2025

Another approach, probably most ideal, would be to combine the caches into a single shared cache, key'd off process id and whose value would contain both the owner and module.

No dup calls to the SUs, higher cache hit rate, and deterministic doubling of memory usage, by the cache, which is fine since the cache is already limited to 1MM entries -> ~20MB max size.

@twilson63 twilson63 merged commit be93939 into main Jan 20, 2025
2 checks passed
@twilson63 twilson63 deleted the twilson63/feat-ur-route-from-module branch January 20, 2025 03:46
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