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

Snapshotter with caching #3028

Closed
wants to merge 1 commit into from
Closed

Conversation

apostasie
Copy link
Contributor

This is a proof of concept / draft for #3027

Average, very un-scientific measurements fornerdctl images:

On 500 identical images at 270MB:

  • without cache: 5.2 seconds
  • with cache: 3.3 seconds

On 500 identical images (buysbox, one layer, 4.174MB):

  • without cache: 5.0 seconds
  • with cache: 4.8 seconds

For about 250 containers (nerdctl ps -as) started from the same image (the big one above), results are in the same ballpark: about 50% gain with cache.

This PR also consolidates code and reduce duplication I did introduce...

Let me know if this is something we should consider.

If yes, we can iterate on design / code location and possibly simplify the signature of imgutil.SnapshotServiceWithCache for eg.

@apostasie
Copy link
Contributor Author

May have an impact on #809 and #325 when a lot of layers are shared.

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
@AkihiroSuda AkihiroSuda requested a review from ktock May 22, 2024 04:41
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone May 22, 2024
}

func (snap *CachingSnapshotter) Stat(ctx context.Context, key string) (snapshots.Info, error) {
if stat, ok := snap.statCache[key]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

snapshots.Info.Updated might be updated. I think this can't be cached.

Copy link
Contributor Author

@apostasie apostasie May 22, 2024

Choose a reason for hiding this comment

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

Hey @ktock ! Thanks for looking at this.

The snapshotter here is NOT meant to do caching across successive calls.

This is meant solely to be used when calling things like image list and walking through the resulting collection of images, and NEVER for two successive, separate in time, calls on the same resource.

Furthermore, there is no persistent caching mechanism - same reason.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarification. At least it should be documented that this should be used only for short term usage.

But why does it wrap the entire snapshotter API whereas it only patches Stat and Usage? I think this doesn't need to be a wrapper for the entire snapshotter and can be implemented at the caller.

Copy link
Contributor Author

@apostasie apostasie May 22, 2024

Choose a reason for hiding this comment

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

You welcome! Yes, I will document.

As for the other question: it does implement the Snapshotter interface so it can easily be swapped in / out in place of the containerd snapshotter.
Of course we can just implement a smaller struct, but that would change the signature.

I am fine with either approach, so, let me know what do you folks prefer here.

Copy link
Member

Choose a reason for hiding this comment

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

Of course we can just implement a smaller struct, but that would change the signature.

SGTM

@apostasie
Copy link
Contributor Author

@ktock I am closing this in favor of #3029 which is no longer a draft and has a complete, cleaner version and caching optimizations that can benefit any method dealing with bulks of objects (ps, images).

@apostasie apostasie closed this May 24, 2024
@AkihiroSuda AkihiroSuda removed this from the v2.0.0 milestone Jun 2, 2024
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