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

llbsolver: create single temp lease for exports for performance #4947

Merged
merged 1 commit into from
May 27, 2024

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented May 23, 2024

Currently there is a temp lease creation in GetRemotes() called from exporters, cache-exporting, and provenance creation and gets called for every cache result when walking the graph. 99% time it returns nil or reference to blob that already existed but temp lease is needed for the odd case where it needs to create new blobs to make sure the code does not go to race against containerd GC. Although it looks like creating a lease is just context.WithValue it is actually quite expensive as it is written into containerd boltdb. This improves performance in this case by creating a single temp lease for the whole export phase of a single job so calls inside GetRemotes() don't need to create their own one for safety.

Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -366,11 +366,11 @@ func (h *HistoryQueue) addResource(ctx context.Context, l leases.Lease, desc *co
}
if _, err := h.hContentStore.Info(ctx, desc.Digest); err != nil {
if errdefs.IsNotFound(err) {
ctx, release, err := leaseutil.WithLease(ctx, h.hLeaseManager, leases.WithID("history_migration_"+identity.NewID()), leaseutil.MakeTemporary)
lr, ctx, err := leaseutil.NewLease(ctx, h.hLeaseManager, leases.WithID("history_migration_"+identity.NewID()), leaseutil.MakeTemporary)
Copy link
Member

Choose a reason for hiding this comment

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

Is this ctx intended to be used by the call on 383 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess lease on a context doesn't do anything for a AddResource call. Technically the lease created here is on the right namespace for that leasemanger but if this if statement is not reached I don't think we need to add lease for only the AddResource call.

@dmcgowan
Copy link
Member

Just one question about whether the shadowed ctx variable is intended. From local testing I see a significant performance improvement with this.

@AkihiroSuda AkihiroSuda merged commit 41c868b into moby:master May 27, 2024
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants