Re: Corrupt name-rev output
- From
René Scharfe <l.s.r@web.de>
- Date
- Apr 22, 2022, 18:44 UTC
- Message-ID
- <340c8810-d912-7b18-d46e-a9d43f20216a@web.de>
- In-Reply-To
- <779eb30b-fdb9-81fb-5d43-c8d388c5cb5a@web.de>
Am 21.04.22 um 19:55 schrieb René Scharfe:
Show 5 quoted lines
> Am 21.04.22 um 04:11 schrieb Elijah Newren: > >> Reverting 2d53975488 fixes the problem. > > That's a good band-aid.
Or perhaps it's all we need. I can't replicate the original reduction of peak memory usage for the Chromium repo anymore. In fact, the very next commit, 079f970971 (name-rev: sort tip names before applying, 2020-02-05), reduced the number of times free(3) is called there from 44245 to 5, and 3656f84278 (name-rev: prefer shorter names over following merges, 2021-12-04) brought that number down to zero.
I can't reproduce the issue with the hardenedBSD repo, by the way, but e.g. with 'git name-rev 58b82150da' in the Linux repo.
--- >8 --- Subject: [PATCH] Revert "name-rev: release unused name strings"
This reverts commit 2d53975488df195e1431c3f90bfb5b60018d5bf6.
3656f84278 (name-rev: prefer shorter names over following merges, 2021-12-04) broke the assumption of 2d53975488 (name-rev: release unused name strings, 2020-02-04) that a better name for a child is a better name for all of its ancestors as well, because it added a penalty for generation > 0. This leads to strings being free(3)'d that are still needed.
079f970971 (name-rev: sort tip names before applying, 2020-02-05) already reduced the number of free(3) calls for the use case that motivated the original patch (name-rev --all in the Chromium repository) from ca. 44000 to 5, and 3656f84278 eliminated even those few. So this revert won't affect name-rev's performance on that particular repo.
Reported-by: Thomas Hurst <tom@hur.st> Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/name-rev.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c index c59b5699fe..02ea9d1633 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -18,7 +18,7 @@ #define CUTOFF_DATE_SLOP 86400 struct rev_name { - char *tip_name; + const char *tip_name; timestamp_t taggerdate; int generation; int distance; @@ -84,7 +84,7 @@ static int commit_is_before_cutoff(struct commit *commit) static int is_valid_rev_name(const struct rev_name *name) { - return name && (name->generation || name->tip_name); + return name && name->tip_name; } static struct rev_name *get_commit_rev_name(const struct commit *commit) @@ -146,20 +146,9 @@ static struct rev_name *create_or_update_name(struct commit *commit, { struct rev_name *name = commit_rev_name_at(&rev_names, commit); - if (is_valid_rev_name(name)) { - if (!is_better_name(name, taggerdate, generation, distance, from_tag)) - return NULL; - - /* - * This string might still be shared with ancestors - * (generation > 0). We can release it here regardless, - * because the new name that has just won will be better - * for them as well, so name_rev() will replace these - * stale pointers when it processes the parents. - */ - if (!name->generation) - free(name->tip_name); - } + if (is_valid_rev_name(name) && + !is_better_name(name, taggerdate, generation, distance, from_tag)) + return NULL; name->taggerdate = taggerdate; name->generation = generation; -- 2.35.3