| Message ID | 282fbe35a7c9db715a8a805f93f9b465d42885a5.1715836916.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: drop all references to `the_repository` | expand |
Patrick Steinhardt <ps@pks.im> writes: [snip] > diff --git a/refs/debug.c b/refs/debug.c > index 58fb4557ed..27aae42134 100644 > --- a/refs/debug.c > +++ b/refs/debug.c > @@ -33,6 +33,12 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor > return (struct ref_store *)res; > } > > +static void debug_release(struct ref_store *refs) > +{ > + struct debug_ref_store *drefs = (struct debug_ref_store *)refs; We should probably add a trace here, using `trace_printf_key()` > + drefs->refs->be->release(drefs->refs); > +} > + > static int debug_create(struct ref_store *refs, int flags, struct strbuf *err) > { > struct debug_ref_store *drefs = (struct debug_ref_store *)refs; > @@ -427,6 +433,7 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, > struct ref_storage_be refs_be_debug = { > .name = "debug", > .init = NULL, > + .release = debug_release, > .create = debug_create, > > /* > diff --git a/refs/files-backend.c b/refs/files-backend.c > index f766d18d5a..368df075c1 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -149,6 +149,14 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store, > return refs; > } > > +static void files_ref_store_release(struct ref_store *ref_store) > +{ > + struct files_ref_store *refs = files_downcast(ref_store, 0, "release"); > + free_ref_cache(refs->loose); > + free(refs->gitcommondir); > + ref_store_release(refs->packed_ref_store); > +} > + > static void files_reflog_path(struct files_ref_store *refs, > struct strbuf *sb, > const char *refname) > @@ -3284,6 +3292,7 @@ static int files_ref_store_create(struct ref_store *ref_store, > struct ref_storage_be refs_be_files = { > .name = "files", > .init = files_ref_store_init, > + .release = files_ref_store_release, > .create = files_ref_store_create, > .transaction_prepare = files_transaction_prepare, > .transaction_finish = files_transaction_finish, > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index 716513efed..bebceb4aa7 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -252,6 +252,15 @@ static void clear_snapshot(struct packed_ref_store *refs) > } > } > > +static void packed_ref_store_release(struct ref_store *ref_store) > +{ > + struct packed_ref_store *refs = packed_downcast(ref_store, 0, "release"); > + clear_snapshot(refs); > + rollback_lock_file(&refs->lock); > + delete_tempfile(&refs->tempfile); > + free(refs->path); > +} Verified that all the params inside `packed_ref_store` are cleaned up here, nice! > static NORETURN void die_unterminated_line(const char *path, > const char *p, size_t len) > { > @@ -1707,6 +1716,7 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s > struct ref_storage_be refs_be_packed = { > .name = "packed", > .init = packed_ref_store_init, > + .release = packed_ref_store_release, > .create = packed_ref_store_create, > .transaction_prepare = packed_transaction_prepare, > .transaction_finish = packed_transaction_finish, > diff --git a/refs/refs-internal.h b/refs/refs-internal.h > index eb42212764..cc1fe6e633 100644 > --- a/refs/refs-internal.h > +++ b/refs/refs-internal.h > @@ -530,6 +530,11 @@ typedef struct ref_store *ref_store_init_fn(struct repository *repo, > const char *gitdir, > unsigned int flags); > > +/* > + * Release all memory and resources associated with the ref store. > + */ > +typedef void ref_store_release_fn(struct ref_store *refs); > + > typedef int ref_store_create_fn(struct ref_store *refs, > int flags, > struct strbuf *err); > @@ -668,6 +673,7 @@ typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refnam > struct ref_storage_be { > const char *name; > ref_store_init_fn *init; > + ref_store_release_fn *release; > ref_store_create_fn *create; > > ref_transaction_prepare_fn *transaction_prepare; > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index a4bb71cd76..6c262c2193 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -293,6 +293,27 @@ static struct ref_store *reftable_be_init(struct repository *repo, > return &refs->base; > } > > +static void reftable_be_release(struct ref_store *ref_store) > +{ > + struct reftable_ref_store *refs = reftable_be_downcast(ref_store, 0, "release"); > + struct strmap_entry *entry; > + struct hashmap_iter iter; > + > + if (refs->main_stack) { > + reftable_stack_destroy(refs->main_stack); > + refs->main_stack = NULL; > + } > + > + if (refs->worktree_stack) { > + reftable_stack_destroy(refs->worktree_stack); > + refs->main_stack = NULL; This should be `refs->worktree_stack`, right? > + } > + > + strmap_for_each_entry(&refs->worktree_stacks, &iter, entry) > + reftable_stack_destroy(entry->value); > + strmap_clear(&refs->worktree_stacks, 0); > +} > + > static int reftable_be_create(struct ref_store *ref_store, > int flags UNUSED, > struct strbuf *err UNUSED) > @@ -2248,6 +2269,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, > struct ref_storage_be refs_be_reftable = { > .name = "reftable", > .init = reftable_be_init, > + .release = reftable_be_release, > .create = reftable_be_create, > .transaction_prepare = reftable_be_transaction_prepare, > .transaction_finish = reftable_be_transaction_finish, > -- > 2.45.1.190.g19fe900cfc.dirty
Karthik Nayak <karthik.188@gmail.com> writes: >> +static void debug_release(struct ref_store *refs) >> +{ >> + struct debug_ref_store *drefs = (struct debug_ref_store *)refs; > > We should probably add a trace here, using `trace_printf_key()` A totally ignorant question. Should we be adding more traces with trace_* API instead of trace2_* API? If the latter aims to cover superset of use cases the former did, I was hoping that we can eventually deprecate the former, hence this question. Of course We could add a compatiblity layer that emulates trace_* API with a thin wrapper around trace2_* API, but if we do not add new callers, it may still be feasible to directly migrate the callers to use trace2_ API without having to invent such compatibility wrappers.
On Thu, May 16, 2024 at 11:39:36AM -0500, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > [snip] > > > diff --git a/refs/debug.c b/refs/debug.c > > index 58fb4557ed..27aae42134 100644 > > --- a/refs/debug.c > > +++ b/refs/debug.c > > @@ -33,6 +33,12 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor > > return (struct ref_store *)res; > > } > > > > +static void debug_release(struct ref_store *refs) > > +{ > > + struct debug_ref_store *drefs = (struct debug_ref_store *)refs; > > We should probably add a trace here, using `trace_printf_key()` I didn't because we don't have `debug_init()` with a trace, either, and because there is no error code that we could report. But on the other hand it does not hurt to have it here, so let's just add it. [snip] > > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > > index a4bb71cd76..6c262c2193 100644 > > --- a/refs/reftable-backend.c > > +++ b/refs/reftable-backend.c > > @@ -293,6 +293,27 @@ static struct ref_store *reftable_be_init(struct repository *repo, > > return &refs->base; > > } > > > > +static void reftable_be_release(struct ref_store *ref_store) > > +{ > > + struct reftable_ref_store *refs = reftable_be_downcast(ref_store, 0, "release"); > > + struct strmap_entry *entry; > > + struct hashmap_iter iter; > > + > > + if (refs->main_stack) { > > + reftable_stack_destroy(refs->main_stack); > > + refs->main_stack = NULL; > > + } > > + > > + if (refs->worktree_stack) { > > + reftable_stack_destroy(refs->worktree_stack); > > + refs->main_stack = NULL; > > This should be `refs->worktree_stack`, right? Good catch, yes. Patrick
On Thu, May 16, 2024 at 11:01:01AM -0700, Junio C Hamano wrote: > Karthik Nayak <karthik.188@gmail.com> writes: > > >> +static void debug_release(struct ref_store *refs) > >> +{ > >> + struct debug_ref_store *drefs = (struct debug_ref_store *)refs; > > > > We should probably add a trace here, using `trace_printf_key()` > > A totally ignorant question. Should we be adding more traces with > trace_* API instead of trace2_* API? If the latter aims to cover > superset of use cases the former did, I was hoping that we can > eventually deprecate the former, hence this question. Of course We > could add a compatiblity layer that emulates trace_* API with a thin > wrapper around trace2_* API, but if we do not add new callers, it > may still be feasible to directly migrate the callers to use trace2_ > API without having to invent such compatibility wrappers. I cannot really answer this question as I ain't got much of a clue around the tracing APIs. But in this case I think we should indeed add this via `trace_printf_key()` so that we remain consistent with all the other wrappers in the debug store. I'd argue that either all functions here should use trace or trace2, but not a mixture. Patrick
diff --git a/refs.c b/refs.c index ad7987efab..edb0a633d1 100644 --- a/refs.c +++ b/refs.c @@ -2041,6 +2041,12 @@ static struct ref_store *ref_store_init(struct repository *repo, return refs; } +void ref_store_release(struct ref_store *ref_store) +{ + ref_store->be->release(ref_store); + free(ref_store->gitdir); +} + struct ref_store *get_main_ref_store(struct repository *r) { if (r->refs_private) diff --git a/refs.h b/refs.h index e9804e4c22..5ecdfb16dc 100644 --- a/refs.h +++ b/refs.h @@ -118,6 +118,11 @@ int is_branch(const char *refname); int ref_store_create(struct ref_store *refs, int flags, struct strbuf *err); +/* + * Release all memory and resources associated with the ref store. + */ +void ref_store_release(struct ref_store *ref_store); + /* * Return the peeled value of the oid currently being iterated via * for_each_ref(), etc. This is equivalent to calling: diff --git a/refs/debug.c b/refs/debug.c index 58fb4557ed..27aae42134 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -33,6 +33,12 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor return (struct ref_store *)res; } +static void debug_release(struct ref_store *refs) +{ + struct debug_ref_store *drefs = (struct debug_ref_store *)refs; + drefs->refs->be->release(drefs->refs); +} + static int debug_create(struct ref_store *refs, int flags, struct strbuf *err) { struct debug_ref_store *drefs = (struct debug_ref_store *)refs; @@ -427,6 +433,7 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, struct ref_storage_be refs_be_debug = { .name = "debug", .init = NULL, + .release = debug_release, .create = debug_create, /* diff --git a/refs/files-backend.c b/refs/files-backend.c index f766d18d5a..368df075c1 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -149,6 +149,14 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store, return refs; } +static void files_ref_store_release(struct ref_store *ref_store) +{ + struct files_ref_store *refs = files_downcast(ref_store, 0, "release"); + free_ref_cache(refs->loose); + free(refs->gitcommondir); + ref_store_release(refs->packed_ref_store); +} + static void files_reflog_path(struct files_ref_store *refs, struct strbuf *sb, const char *refname) @@ -3284,6 +3292,7 @@ static int files_ref_store_create(struct ref_store *ref_store, struct ref_storage_be refs_be_files = { .name = "files", .init = files_ref_store_init, + .release = files_ref_store_release, .create = files_ref_store_create, .transaction_prepare = files_transaction_prepare, .transaction_finish = files_transaction_finish, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 716513efed..bebceb4aa7 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -252,6 +252,15 @@ static void clear_snapshot(struct packed_ref_store *refs) } } +static void packed_ref_store_release(struct ref_store *ref_store) +{ + struct packed_ref_store *refs = packed_downcast(ref_store, 0, "release"); + clear_snapshot(refs); + rollback_lock_file(&refs->lock); + delete_tempfile(&refs->tempfile); + free(refs->path); +} + static NORETURN void die_unterminated_line(const char *path, const char *p, size_t len) { @@ -1707,6 +1716,7 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s struct ref_storage_be refs_be_packed = { .name = "packed", .init = packed_ref_store_init, + .release = packed_ref_store_release, .create = packed_ref_store_create, .transaction_prepare = packed_transaction_prepare, .transaction_finish = packed_transaction_finish, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index eb42212764..cc1fe6e633 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -530,6 +530,11 @@ typedef struct ref_store *ref_store_init_fn(struct repository *repo, const char *gitdir, unsigned int flags); +/* + * Release all memory and resources associated with the ref store. + */ +typedef void ref_store_release_fn(struct ref_store *refs); + typedef int ref_store_create_fn(struct ref_store *refs, int flags, struct strbuf *err); @@ -668,6 +673,7 @@ typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refnam struct ref_storage_be { const char *name; ref_store_init_fn *init; + ref_store_release_fn *release; ref_store_create_fn *create; ref_transaction_prepare_fn *transaction_prepare; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index a4bb71cd76..6c262c2193 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -293,6 +293,27 @@ static struct ref_store *reftable_be_init(struct repository *repo, return &refs->base; } +static void reftable_be_release(struct ref_store *ref_store) +{ + struct reftable_ref_store *refs = reftable_be_downcast(ref_store, 0, "release"); + struct strmap_entry *entry; + struct hashmap_iter iter; + + if (refs->main_stack) { + reftable_stack_destroy(refs->main_stack); + refs->main_stack = NULL; + } + + if (refs->worktree_stack) { + reftable_stack_destroy(refs->worktree_stack); + refs->main_stack = NULL; + } + + strmap_for_each_entry(&refs->worktree_stacks, &iter, entry) + reftable_stack_destroy(entry->value); + strmap_clear(&refs->worktree_stacks, 0); +} + static int reftable_be_create(struct ref_store *ref_store, int flags UNUSED, struct strbuf *err UNUSED) @@ -2248,6 +2269,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, struct ref_storage_be refs_be_reftable = { .name = "reftable", .init = reftable_be_init, + .release = reftable_be_release, .create = reftable_be_create, .transaction_prepare = reftable_be_transaction_prepare, .transaction_finish = reftable_be_transaction_finish,
Ref storages are typically only initialized once for `the_repository` and then never released. Until now we got away with that without causing memory leaks because `the_repository` stays reachable, and because the ref backend is reachable via `the_repository` its memory basically never leaks. This is about to change though because of the upcoming migration logic, which will create a secondary ref storage. In that case, we will either have to release the old or new ref storage to avoid leaks. Implement a new `release` callback and expose it via a new `ref_storage_release()` function. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs.c | 6 ++++++ refs.h | 5 +++++ refs/debug.c | 7 +++++++ refs/files-backend.c | 9 +++++++++ refs/packed-backend.c | 10 ++++++++++ refs/refs-internal.h | 6 ++++++ refs/reftable-backend.c | 22 ++++++++++++++++++++++ 7 files changed, 65 insertions(+)