| Message ID | 20190919214712.7348-10-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: eliminate recursion | expand |
Am 19.09.19 um 23:47 schrieb SZEDER Gábor: > At the beginning of the recursive name_rev() function it parses the > commit it got as parameter, and returns early if the commit is older > than a cutoff limit. > > Restructure this so the caller parses the commit and checks its date, > and doesn't invoke name_rev() if the commit to be passed as parameter > is older than the cutoff, i.e. both name_ref() before calling > name_rev() and name_rev() itself as it iterates over the parent > commits. > > This makes eliminating the recursion a bit easier to follow, and it > will be moved back to name_rev() after the recursion is eliminated. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > builtin/name-rev.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/builtin/name-rev.c b/builtin/name-rev.c > index 42cea5c881..99643aa4dc 100644 > --- a/builtin/name-rev.c > +++ b/builtin/name-rev.c > @@ -107,11 +107,6 @@ static void name_rev(struct commit *commit, > struct commit_list *parents; > int parent_number = 1; > > - parse_commit(commit); > - > - if (commit->date < cutoff) > - return; > - > if (!create_or_update_name(commit, tip_name, taggerdate, generation, > distance, from_tag)) > return; > @@ -119,6 +114,12 @@ static void name_rev(struct commit *commit, > for (parents = commit->parents; > parents; > parents = parents->next, parent_number++) { > + struct commit *parent = parents->item; > + > + parse_commit(parent); > + if (parent->date < cutoff) > + continue; > + > if (parent_number > 1) { > size_t len; > char *new_name; > @@ -131,11 +132,11 @@ static void name_rev(struct commit *commit, > new_name = xstrfmt("%.*s^%d", (int)len, tip_name, > parent_number); > The check now also skips this allocation for old commits... > - name_rev(parents->item, new_name, taggerdate, 0, > + name_rev(parent, new_name, taggerdate, 0, > distance + MERGE_TRAVERSAL_WEIGHT, > from_tag); > } else { > - name_rev(parents->item, tip_name, taggerdate, > + name_rev(parent, tip_name, taggerdate, > generation + 1, distance + 1, > from_tag); > } > @@ -269,16 +270,18 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo > if (o && o->type == OBJ_COMMIT) { > struct commit *commit = (struct commit *)o; > int from_tag = starts_with(path, "refs/tags/"); > - const char *tip_name; > > if (taggerdate == TIME_MAX) > taggerdate = commit->date; > path = name_ref_abbrev(path, can_abbreviate_output); > - if (deref) > - tip_name = xstrfmt("%s^0", path); > - else > - tip_name = xstrdup(path); > - name_rev(commit, tip_name, taggerdate, 0, 0, from_tag); > + if (commit->date >= cutoff) { > + const char *tip_name; > + if (deref) > + tip_name = xstrfmt("%s^0", path); > + else > + tip_name = xstrdup(path); ... and this allocation here as well. If this improves performance in a meaningful way then perhaps it should be kept at this place? And if it doesn't, then an additional allocation might not hurt much? Just a thought, I still didn't measure.. > + name_rev(commit, tip_name, taggerdate, 0, 0, from_tag); > + } > } > return 0; > } >
diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 42cea5c881..99643aa4dc 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -107,11 +107,6 @@ static void name_rev(struct commit *commit, struct commit_list *parents; int parent_number = 1; - parse_commit(commit); - - if (commit->date < cutoff) - return; - if (!create_or_update_name(commit, tip_name, taggerdate, generation, distance, from_tag)) return; @@ -119,6 +114,12 @@ static void name_rev(struct commit *commit, for (parents = commit->parents; parents; parents = parents->next, parent_number++) { + struct commit *parent = parents->item; + + parse_commit(parent); + if (parent->date < cutoff) + continue; + if (parent_number > 1) { size_t len; char *new_name; @@ -131,11 +132,11 @@ static void name_rev(struct commit *commit, new_name = xstrfmt("%.*s^%d", (int)len, tip_name, parent_number); - name_rev(parents->item, new_name, taggerdate, 0, + name_rev(parent, new_name, taggerdate, 0, distance + MERGE_TRAVERSAL_WEIGHT, from_tag); } else { - name_rev(parents->item, tip_name, taggerdate, + name_rev(parent, tip_name, taggerdate, generation + 1, distance + 1, from_tag); } @@ -269,16 +270,18 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; int from_tag = starts_with(path, "refs/tags/"); - const char *tip_name; if (taggerdate == TIME_MAX) taggerdate = commit->date; path = name_ref_abbrev(path, can_abbreviate_output); - if (deref) - tip_name = xstrfmt("%s^0", path); - else - tip_name = xstrdup(path); - name_rev(commit, tip_name, taggerdate, 0, 0, from_tag); + if (commit->date >= cutoff) { + const char *tip_name; + if (deref) + tip_name = xstrfmt("%s^0", path); + else + tip_name = xstrdup(path); + name_rev(commit, tip_name, taggerdate, 0, 0, from_tag); + } } return 0; }
At the beginning of the recursive name_rev() function it parses the commit it got as parameter, and returns early if the commit is older than a cutoff limit. Restructure this so the caller parses the commit and checks its date, and doesn't invoke name_rev() if the commit to be passed as parameter is older than the cutoff, i.e. both name_ref() before calling name_rev() and name_rev() itself as it iterates over the parent commits. This makes eliminating the recursion a bit easier to follow, and it will be moved back to name_rev() after the recursion is eliminated. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- builtin/name-rev.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)