From 07f7d55a346f0eb73a358736ce065f8c08b46452 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:43:37 +0100 Subject: [PATCH 1/9] commit: avoid allocation in clear_commit_marks_many() Pass the entries of the commit array directly to clear_commit_marks_1() instead of adding them to a commit_list first. The function clears the commit and any first parent without allocation; only higher numbered parents are added to a list for later treatment. This change extends that optimization to clear_commit_marks_many(). Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commit.c b/commit.c index cab8d4455b..82667514bd 100644 --- a/commit.c +++ b/commit.c @@ -547,7 +547,7 @@ void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark) struct commit_list *list = NULL; while (nr--) { - commit_list_insert(*commit, &list); + clear_commit_marks_1(&list, *commit, mark); commit++; } while (list) From abc035126a58829cb7265b3ab2f2ca30312aa3b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:44:03 +0100 Subject: [PATCH 2/9] commit: use clear_commit_marks_many() in remove_redundant() Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- commit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/commit.c b/commit.c index 82667514bd..9edc12f338 100644 --- a/commit.c +++ b/commit.c @@ -929,8 +929,7 @@ static int remove_redundant(struct commit **array, int cnt) if (work[j]->object.flags & PARENT1) redundant[filled_index[j]] = 1; clear_commit_marks(array[i], all_flags); - for (j = 0; j < filled; j++) - clear_commit_marks(work[j], all_flags); + clear_commit_marks_many(filled, work, all_flags); free_commit_list(common); } From 5dee6d6f28370b613ccf945b1ba8cecb0b44c172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:44:12 +0100 Subject: [PATCH 3/9] ref-filter: use clear_commit_marks_many() in do_merge_filter() Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- ref-filter.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index e728b15b3a..1d0d77c30d 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1961,8 +1961,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata) free_array_item(item); } - for (i = 0; i < old_nr; i++) - clear_commit_marks(to_clear[i], ALL_REV_FLAGS); + clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS); clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS); free(to_clear); } From 4ad315fc99d906364c9a5d75a319752cf4056ae5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:44:58 +0100 Subject: [PATCH 4/9] object: add clear_commit_marks_all() Add a function for clearing the commit marks of all in-core commit objects. It's similar to clear_object_flags(), but more precise, since it leaves the other object types alone. It still has to iterate through them, though. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- object.c | 11 +++++++++++ object.h | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/object.c b/object.c index b9a4a0e501..0afdfd19b7 100644 --- a/object.c +++ b/object.c @@ -434,3 +434,14 @@ void clear_object_flags(unsigned flags) obj->flags &= ~flags; } } + +void clear_commit_marks_all(unsigned int flags) +{ + int i; + + for (i = 0; i < obj_hash_size; i++) { + struct object *obj = obj_hash[i]; + if (obj && obj->type == OBJ_COMMIT) + obj->flags &= ~flags; + } +} diff --git a/object.h b/object.h index df8abe96f7..1111f64dd9 100644 --- a/object.h +++ b/object.h @@ -148,4 +148,9 @@ void object_array_clear(struct object_array *array); void clear_object_flags(unsigned flags); +/* + * Clear the specified object flags from all in-core commit objects. + */ +extern void clear_commit_marks_all(unsigned int flags); + #endif /* OBJECT_H */ From 148f14ab5e066bb95a9f0bc00380ca98369555dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:45:36 +0100 Subject: [PATCH 5/9] bisect: avoid using the rev_info flag leak_pending The leak_pending flag is so awkward to use that multiple comments had to be added around each occurrence. We only use it for remembering the commits whose marks we have to clear after checking if all of the good ones are ancestors of the bad one. This is easy, though: We need to do that for the bad and good commits, of course. Let check_good_are_ancestors_of_bad() create and own the array of bad and good commits, and use it to clear the commit marks as well. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- bisect.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/bisect.c b/bisect.c index 3756f127b0..4e78495bc2 100644 --- a/bisect.c +++ b/bisect.c @@ -784,11 +784,9 @@ static void handle_skipped_merge_base(const struct object_id *mb) * - If one is "skipped", we can't know but we should warn. * - If we don't know, we should check it out and ask the user to test. */ -static void check_merge_bases(int no_checkout) +static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) { struct commit_list *result; - int rev_nr; - struct commit **rev = get_bad_and_good_commits(&rev_nr); result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1); @@ -806,34 +804,21 @@ static void check_merge_bases(int no_checkout) } } - free(rev); free_commit_list(result); } -static int check_ancestors(const char *prefix) +static int check_ancestors(int rev_nr, struct commit **rev, const char *prefix) { struct rev_info revs; - struct object_array pending_copy; int res; bisect_rev_setup(&revs, prefix, "^%s", "%s", 0); - /* Save pending objects, so they can be cleaned up later. */ - pending_copy = revs.pending; - revs.leak_pending = 1; - - /* - * bisect_common calls prepare_revision_walk right away, which - * (together with .leak_pending = 1) makes us the sole owner of - * the list of pending objects. - */ bisect_common(&revs); res = (revs.commits != NULL); /* Clean up objects used, as they will be reused. */ - clear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS); - - object_array_clear(&pending_copy); + clear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS); return res; } @@ -850,7 +835,8 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout) { char *filename = git_pathdup("BISECT_ANCESTORS_OK"); struct stat st; - int fd; + int fd, rev_nr; + struct commit **rev; if (!current_bad_oid) die(_("a %s revision is needed"), term_bad); @@ -864,8 +850,10 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout) goto done; /* Check if all good revs are ancestor of the bad rev. */ - if (check_ancestors(prefix)) - check_merge_bases(no_checkout); + rev = get_bad_and_good_commits(&rev_nr); + if (check_ancestors(rev_nr, rev, prefix)) + check_merge_bases(rev_nr, rev, no_checkout); + free(rev); /* Create file BISECT_ANCESTORS_OK. */ fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); From 63647391e6c306363dcb61d160c84cc601912efb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:46:14 +0100 Subject: [PATCH 6/9] bundle: avoid using the rev_info flag leak_pending The leak_pending flag is so awkward to use that multiple comments had to be added around each occurrence. We use it for remembering the prerequisites for the bundle. That is easy, though: We have the ref_list named "prerequisites" in the header for just that purpose. Use this original list of prerequisites to check if all of them are present and to clear their commit marks afterward. The two new loops are intentionally kept similar to the first one in the function. Calling parse_object() a second time is expected be quick and successful in each case -- any errors should have been handled in the first round. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- bundle.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/bundle.c b/bundle.c index c092d5d68f..0d07202f8c 100644 --- a/bundle.c +++ b/bundle.c @@ -134,7 +134,6 @@ int verify_bundle(struct bundle_header *header, int verbose) struct ref_list *p = &header->prerequisites; struct rev_info revs; const char *argv[] = {NULL, "--all", NULL}; - struct object_array refs; struct commit *commit; int i, ret = 0, req_nr; const char *message = _("Repository lacks these prerequisite commits:"); @@ -157,14 +156,6 @@ int verify_bundle(struct bundle_header *header, int verbose) req_nr = revs.pending.nr; setup_revisions(2, argv, &revs, NULL); - /* Save pending objects, so they can be cleaned up later. */ - refs = revs.pending; - revs.leak_pending = 1; - - /* - * prepare_revision_walk (together with .leak_pending = 1) makes us - * the sole owner of the list of pending objects. - */ if (prepare_revision_walk(&revs)) die(_("revision walk setup failed")); @@ -173,18 +164,24 @@ int verify_bundle(struct bundle_header *header, int verbose) if (commit->object.flags & PREREQ_MARK) i--; - for (i = 0; i < req_nr; i++) - if (!(refs.objects[i].item->flags & SHOWN)) { - if (++ret == 1) - error("%s", message); - error("%s %s", oid_to_hex(&refs.objects[i].item->oid), - refs.objects[i].name); - } + for (i = 0; i < p->nr; i++) { + struct ref_list_entry *e = p->list + i; + struct object *o = parse_object(&e->oid); + assert(o); /* otherwise we'd have returned early */ + if (o->flags & SHOWN) + continue; + if (++ret == 1) + error("%s", message); + error("%s %s", oid_to_hex(&e->oid), e->name); + } /* Clean up objects used, as they will be reused. */ - clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS); - - object_array_clear(&refs); + for (i = 0; i < p->nr; i++) { + struct ref_list_entry *e = p->list + i; + commit = lookup_commit_reference_gently(&e->oid, 1); + if (commit) + clear_commit_marks(commit, ALL_REV_FLAGS); + } if (verbose) { struct ref_list *r; From a9a03fa0d7292758b1c5038c0acb9466c9ac36da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:47:04 +0100 Subject: [PATCH 7/9] checkout: avoid using the rev_info flag leak_pending The leak_pending flag is so awkward to use that multiple comments had to be added around each occurrence. We only use it for remembering the commits whose marks we have to clear after checking if the old HEAD is detached. This is easy, though: We need to do that for the old commit, the new one -- and for all refs. Don't bother tracking exactly which commits need their flags cleared, just nuke all we have in-core. This change is safe because refs can point at anything, so other program parts can't depend on any kept flags anyway. And since all refs are loaded we have to basically deal with all commits anyway, so performance should not be negatively impacted. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/checkout.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index fc4f8fd2ea..4f61ff0aa0 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -786,7 +786,6 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new) { struct rev_info revs; struct object *object = &old->object; - struct object_array refs; init_revisions(&revs, NULL); setup_revisions(0, NULL, &revs, NULL); @@ -797,14 +796,6 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new) for_each_ref(add_pending_uninteresting_ref, &revs); add_pending_oid(&revs, "HEAD", &new->object.oid, UNINTERESTING); - /* Save pending objects, so they can be cleaned up later. */ - refs = revs.pending; - revs.leak_pending = 1; - - /* - * prepare_revision_walk (together with .leak_pending = 1) makes us - * the sole owner of the list of pending objects. - */ if (prepare_revision_walk(&revs)) die(_("internal error in revision walk")); if (!(old->object.flags & UNINTERESTING)) @@ -813,9 +804,7 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new) describe_detached_head(_("Previous HEAD position was"), old); /* Clean up objects used, as they will be reused. */ - clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS); - - object_array_clear(&refs); + clear_commit_marks_all(ALL_REV_FLAGS); } static int switch_branches(const struct checkout_opts *opts, From f1230fb5fc77a47fd7964bb3724db352aea4a476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:47:51 +0100 Subject: [PATCH 8/9] revision: remove the unused flag leak_pending Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- revision.c | 3 +-- revision.h | 12 ------------ 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/revision.c b/revision.c index d167223e69..8b14142bb1 100644 --- a/revision.c +++ b/revision.c @@ -2850,8 +2850,7 @@ int prepare_revision_walk(struct rev_info *revs) } } } - if (!revs->leak_pending) - object_array_clear(&old_pending); + object_array_clear(&old_pending); /* Signal whether we need per-parent treesame decoration */ if (revs->simplify_merges || diff --git a/revision.h b/revision.h index 54761200ad..27be70e75c 100644 --- a/revision.h +++ b/revision.h @@ -150,18 +150,6 @@ struct rev_info { date_mode_explicit:1, preserve_subject:1; unsigned int disable_stdin:1; - /* - * Set `leak_pending` to prevent `prepare_revision_walk()` from clearing - * the array of pending objects (`pending`). It will still forget about - * the array and its entries, so they really are leaked. This can be - * useful if the `struct object_array` `pending` is copied before - * calling `prepare_revision_walk()`. By setting `leak_pending`, you - * effectively claim ownership of the old array, so you should most - * likely call `object_array_clear(&pending_copy)` once you are done. - * Observe that this is about ownership of the array and its entries, - * not the commits referenced by those entries. - */ - unsigned int leak_pending:1; /* --show-linear-break */ unsigned int track_linear:1, track_first_time:1, From 6fcec2f9aeeac6329ecf2f7084173f5b4346588b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 25 Dec 2017 18:48:34 +0100 Subject: [PATCH 9/9] commit: remove unused function clear_commit_marks_for_object_array() Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- commit.c | 14 -------------- commit.h | 1 - 2 files changed, 15 deletions(-) diff --git a/commit.c b/commit.c index 9edc12f338..ff51c9f34a 100644 --- a/commit.c +++ b/commit.c @@ -559,20 +559,6 @@ void clear_commit_marks(struct commit *commit, unsigned int mark) clear_commit_marks_many(1, &commit, mark); } -void clear_commit_marks_for_object_array(struct object_array *a, unsigned mark) -{ - struct object *object; - struct commit *commit; - unsigned int i; - - for (i = 0; i < a->nr; i++) { - object = a->objects[i].item; - commit = lookup_commit_reference_gently(&object->oid, 1); - if (commit) - clear_commit_marks(commit, mark); - } -} - struct commit *pop_commit(struct commit_list **stack) { struct commit_list *top = *stack; diff --git a/commit.h b/commit.h index 99a3fea68d..bdf14f0a72 100644 --- a/commit.h +++ b/commit.h @@ -219,7 +219,6 @@ struct commit *pop_commit(struct commit_list **stack); void clear_commit_marks(struct commit *commit, unsigned int mark); void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark); -void clear_commit_marks_for_object_array(struct object_array *a, unsigned mark); enum rev_sort_order {