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

fix: simplify preloader logic #284

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 2, 2023

After writing a doc commit into @hannahhoward's #277 to clarify the preloader Loader() flow, I noticed a possible wasteful write @

// write to parent and cache and return
return loadTo(
r,
[]linking.BlockWriteOpener{cs.cacheLinkSystem.StorageWriteOpener, cs.parentLinkSystem.StorageWriteOpener},
linkCtx,
link)

At that point, the Loader() call has failed to find the block in the parent or cache linksystems and it's not in the preload queue, so it's a new block that hasn't been encountered before and we load it from the "Fetcher" (bitswap) directly, bypassing any messy preload semantics. But after we have it, we write it both to the cache and to the parent. But, once it's in the parent, we shouldn't need it in the cache anymore. It's up to the user of the preloader to ensure that the parent linksystem is also able to cache blocks for duplicate-block traversals, and it already does for us—we have a messy 3-way CAR + Stream output attached to this as both the parent and cache, so the double write is entirely wasteful.

That then exposes some other simplifications for the code, which is very nice. 17 additions and 40 deletions.

@rvagg rvagg requested a review from hannahhoward June 2, 2023 04:13
@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #284 (4ef872b) into fix/missing-block-multiple-protocols (2b3656b) will decrease coverage by 1.50%.
The diff coverage is 73.33%.

Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                            @@
##           fix/missing-block-multiple-protocols     #284      +/-   ##
========================================================================
- Coverage                                 73.07%   71.57%   -1.50%     
========================================================================
  Files                                        70       69       -1     
  Lines                                      6651     6068     -583     
========================================================================
- Hits                                       4860     4343     -517     
+ Misses                                     1547     1486      -61     
+ Partials                                    244      239       -5     
Impacted Files Coverage Δ
.../retriever/bitswaphelpers/preloadcachingstorage.go 78.45% <73.33%> (-1.55%) ⬇️

... and 19 files with indirect coverage changes

Base automatically changed from fix/missing-block-multiple-protocols to main June 2, 2023 15:59
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM

@hannahhoward hannahhoward merged commit 7a1b0fb into main Jun 5, 2023
16 checks passed
@hannahhoward hannahhoward deleted the rvagg/simplify-preloader branch June 5, 2023 18:42
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.

None yet

3 participants