Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Add CacheEntry type and use it for SymCaches and PpdbCaches #929

Merged
merged 15 commits into from
Dec 5, 2022

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented Dec 1, 2022

This adds a new enum CacheEntry that is intended to eventually replace the current representation of cache entries as (essentially) a pair of (CacheStatus, ByteView) and the myriad different errors. As a first step, we use it in SymCacheActor::fetch and PortablePdbCacheActor::fetch and consequently also in the module_lookup module. The code in the latter becomes slightly simpler, in my opinion, but the big win is that we parse the caches only once and from then on pass around the parsed object instead of using a ByteView that has to be parsed over and over again.

This currently requires a development version of symbolic because some additional traits have to be implemented for PortablePdbCache.

EDIT: The most recent version replaces the custom CacheEntry enum with a Result. The semantics stay exactly the same.

This changes `SymCacheActor::fetch` and `PortablePdbCacheActor::fetch`
so that they return a `CacheEntry` (plus auxiliary information, i.e.
candidates and features).
@loewenheim loewenheim self-assigned this Dec 1, 2022
@loewenheim loewenheim linked an issue Dec 1, 2022 that may be closed by this pull request
@loewenheim loewenheim marked this pull request as ready for review December 5, 2022 14:51
@loewenheim loewenheim requested a review from a team December 5, 2022 14:51
@codecov-commenter
Copy link

Codecov Report

Base: 64.87% // Head: 64.54% // Decreases project coverage by -0.32% ⚠️

Coverage data is based on head (3edaf3a) compared to base (bbaec01).
Patch coverage: 63.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
- Coverage   64.87%   64.54%   -0.33%     
==========================================
  Files          68       68              
  Lines       11797    12012     +215     
==========================================
+ Hits         7653     7753     +100     
- Misses       4144     4259     +115     
Impacted Files Coverage Δ
...s/symbolicator-service/src/services/ppdb_caches.rs 65.44% <47.05%> (-18.20%) ⬇️
...symbolicator-service/src/services/symcaches/mod.rs 81.76% <56.84%> (-11.48%) ⬇️
crates/symbolicator-service/src/cache.rs 90.75% <65.90%> (-3.24%) ⬇️
...ervice/src/services/symbolication/module_lookup.rs 97.24% <88.88%> (-0.44%) ⬇️
...ervice/src/services/symbolication/symbolication.rs 79.12% <100.00%> (+0.75%) ⬆️
...olicator-service/src/services/symbolication/mod.rs 50.90% <0.00%> (-7.28%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@loewenheim loewenheim merged commit 22d830b into master Dec 5, 2022
@loewenheim loewenheim deleted the ref/cache-entry branch December 5, 2022 16:06
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.

Clean up internal Error / Cache propagation
3 participants