| Message ID | 6f1a2547-ad0d-942e-b4a0-c614da1973fd@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: improve memory usage | expand |
René Scharfe <l.s.r@web.de> writes: > Subject: Re: [PATCH 11/10] name-rev: sort tip names before applying "before applying" what? > name_ref() is called for each ref and checks if its a better name for s/its/it's/ > the referenced commit. If that's the case it remembers it and checks if s/case/&,/ > a name based on it is better for its ancestors as well. This in done in > the the order for_each_ref() imposes on us. s/the the/the/ > That might not be optimal. If bad names happen to be encountered first > (as defined by is_better_name()), names derived from them may spread to > a lot of commits, only to be replaced by better names later. Setting > better names first can avoid that. > > is_better_name() prefers tags, short distances and old references. The > distance is a measure that we need to calculate for each candidate > commit, but the other two properties are not dependent on the > relationships of commits. Sorting the refs by them should yield better > performance than the essentially random order we currently use. Good insight. > And applying older references first should also help to reduce rework > due to the fact that older commits have less ancestors than newer ones. "older" in the sense of committer/tagger timestamp. I wonder it would further help if the commit-graph is available and give us generation number. > So add all details of names to the tip table first, then sort them > to prefer tags and older references and then apply them in this order. > Here's the performance as measures by hyperfine for the Linux repo > before: > > Benchmark #1: ./git -C ../linux/ name-rev --all > Time (mean ± σ): 851.1 ms ± 4.5 ms [User: 806.7 ms, System: 44.4 ms] > Range (min … max): 845.9 ms … 859.5 ms 10 runs > > ... and with this patch: > > Benchmark #1: ./git -C ../linux/ name-rev --all > Time (mean ± σ): 736.2 ms ± 8.7 ms [User: 688.4 ms, System: 47.5 ms] > Range (min … max): 726.0 ms … 755.2 ms 10 runs > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > builtin/name-rev.c | 60 +++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 52 insertions(+), 8 deletions(-) > > diff --git a/builtin/name-rev.c b/builtin/name-rev.c > index 23a639ff30..a9dcd25e46 100644 > --- a/builtin/name-rev.c > +++ b/builtin/name-rev.c > @@ -247,6 +247,10 @@ static struct tip_table { > struct tip_table_entry { > struct object_id oid; > const char *refname; > + struct commit *commit; > + timestamp_t taggerdate; > + unsigned int from_tag:1; > + unsigned int deref:1; > } *table; > int nr; > int alloc; > @@ -254,13 +258,18 @@ static struct tip_table { > } tip_table; > > static void add_to_tip_table(const struct object_id *oid, const char *refname, > - int shorten_unambiguous) > + int shorten_unambiguous, struct commit *commit, > + timestamp_t taggerdate, int from_tag, int deref) > { > refname = name_ref_abbrev(refname, shorten_unambiguous); > > ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc); > oidcpy(&tip_table.table[tip_table.nr].oid, oid); > tip_table.table[tip_table.nr].refname = xstrdup(refname); > + tip_table.table[tip_table.nr].commit = commit; > + tip_table.table[tip_table.nr].taggerdate = taggerdate; > + tip_table.table[tip_table.nr].from_tag = from_tag; > + tip_table.table[tip_table.nr].deref = deref; > tip_table.nr++; > tip_table.sorted = 0; > } > @@ -271,12 +280,30 @@ static int tipcmp(const void *a_, const void *b_) > return oidcmp(&a->oid, &b->oid); > } > > +static int cmp_by_tag_and_age(const void *a_, const void *b_) > +{ > + const struct tip_table_entry *a = a_, *b = b_; > + int cmp; > + > + /* Prefer tags. */ > + cmp = b->from_tag - a->from_tag; We end up with negative -1 if b is not from tag and a is from tag, even though the from_tag field is unsigned, so this is not wrong per-se but feels a bit subtle. > + if (cmp) > + return cmp; > + > + /* Older is better. */ > + if (a->taggerdate < b->taggerdate) > + return -1; We are here if both are from tag or neither is from tag. Mental note: let's make sure that add_to_tip_table() is always called with usable timestamp even for a commit that is not from tag. > + return a->taggerdate != b->taggerdate; OK, we know a is either of the same age as or younger than b at this point, so we would return 1 if a is not the same age as b. > static int name_ref(const char *path, const struct object_id *oid, int flags, void *cb_data) > { > struct object *o = parse_object(the_repository, oid); > struct name_ref_data *data = cb_data; > int can_abbreviate_output = data->tags_only && data->name_only; > int deref = 0; > + int from_tag = 0; > + struct commit *commit = NULL; > timestamp_t taggerdate = TIME_MAX; > > if (data->tags_only && !starts_with(path, "refs/tags/")) > @@ -325,8 +352,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo > return 0; > } > > - add_to_tip_table(oid, path, can_abbreviate_output); > - > while (o && o->type == OBJ_TAG) { > struct tag *t = (struct tag *) o; > if (!t->tagged) > @@ -336,17 +361,35 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo > taggerdate = t->date; > } > if (o && o->type == OBJ_COMMIT) { > - struct commit *commit = (struct commit *)o; > - int from_tag = starts_with(path, "refs/tags/"); > - > + commit = (struct commit *)o; > + from_tag = starts_with(path, "refs/tags/"); > if (taggerdate == TIME_MAX) > taggerdate = commit->date; OK, so a lightweight tag gets the commit date, while an annotated tag gets the tagger date (while peeling in the loop just above this part). I never make an annotated tag long after creating the commit the tag refers to myself (i.e. on the day I tag a release or a release candidate, I know that the commit I am creating is what I will tag even before creating the commit, and I tag the commit soon after that), but I can imagine that in some other workflows the maintainers may be tagging long after the commit was made, and more importantly, the interval of time between comitting and tagging might be different from release to release. I wonder if it mimicks the topological ordering better if always compare the timestamps of underlying commits. > - path = name_ref_abbrev(path, can_abbreviate_output); > - name_rev(commit, path, taggerdate, from_tag, deref); > } > + > + add_to_tip_table(oid, path, can_abbreviate_output, commit, taggerdate, > + from_tag, deref); > return 0; > } > > +static void name_tips(void) > +{ > + int i; > + > + /* > + * Try to set better names first, so that worse ones spread > + * less. > + */ > + QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age); > + for (i = 0; i < tip_table.nr; i++) { > + struct tip_table_entry *e = &tip_table.table[i]; > + if (e->commit) { Sorry, I am confused. I didn't see anything that clears tip_table.table[i].commit in the code. > + name_rev(e->commit, e->refname, e->taggerdate, > + e->from_tag, e->deref); > + } > + } > +} > + > static const unsigned char *nth_tip_table_ent(size_t ix, void *table_) > { > struct tip_table_entry *table = table_; > @@ -559,6 +602,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) > cutoff = TIME_MIN; > } > for_each_ref(name_ref, &data); > + name_tips(); > > if (transform_stdin) { > char buffer[2048]; > -- > 2.25.0
Am 05.02.20 um 19:23 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Subject: Re: [PATCH 11/10] name-rev: sort tip names before applying > > "before applying" what? The tip name. "Using" or "attaching to commits" might be better than "applying" here. >> name_ref() is called for each ref and checks if its a better name for > > s/its/it's/ > >> the referenced commit. If that's the case it remembers it and checks if > > s/case/&,/ > >> a name based on it is better for its ancestors as well. This in done in >> the the order for_each_ref() imposes on us. > > s/the the/the/ Right. I was just too excited.. >> And applying older references first should also help to reduce rework >> due to the fact that older commits have less ancestors than newer ones. > > "older" in the sense of committer/tagger timestamp. I wonder it > would further help if the commit-graph is available and give us > generation number. To sort by generation number instead of by timestamp? That would at the very least help in repos whose timestamps are wonky (imported from timeless version control system, or commits from a computer with a broken clock). >> So add all details of names to the tip table first, then sort them >> to prefer tags and older references and then apply them in this order. >> Here's the performance as measures by hyperfine for the Linux repo >> before: >> >> Benchmark #1: ./git -C ../linux/ name-rev --all >> Time (mean ± σ): 851.1 ms ± 4.5 ms [User: 806.7 ms, System: 44.4 ms] >> Range (min … max): 845.9 ms … 859.5 ms 10 runs >> >> ... and with this patch: >> >> Benchmark #1: ./git -C ../linux/ name-rev --all >> Time (mean ± σ): 736.2 ms ± 8.7 ms [User: 688.4 ms, System: 47.5 ms] >> Range (min … max): 726.0 ms … 755.2 ms 10 runs >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> builtin/name-rev.c | 60 +++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 52 insertions(+), 8 deletions(-) >> >> diff --git a/builtin/name-rev.c b/builtin/name-rev.c >> index 23a639ff30..a9dcd25e46 100644 >> --- a/builtin/name-rev.c >> +++ b/builtin/name-rev.c >> @@ -247,6 +247,10 @@ static struct tip_table { >> struct tip_table_entry { >> struct object_id oid; >> const char *refname; >> + struct commit *commit; >> + timestamp_t taggerdate; >> + unsigned int from_tag:1; >> + unsigned int deref:1; >> } *table; >> int nr; >> int alloc; >> @@ -254,13 +258,18 @@ static struct tip_table { >> } tip_table; >> >> static void add_to_tip_table(const struct object_id *oid, const char *refname, >> - int shorten_unambiguous) >> + int shorten_unambiguous, struct commit *commit, >> + timestamp_t taggerdate, int from_tag, int deref) >> { >> refname = name_ref_abbrev(refname, shorten_unambiguous); >> >> ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc); >> oidcpy(&tip_table.table[tip_table.nr].oid, oid); >> tip_table.table[tip_table.nr].refname = xstrdup(refname); >> + tip_table.table[tip_table.nr].commit = commit; >> + tip_table.table[tip_table.nr].taggerdate = taggerdate; >> + tip_table.table[tip_table.nr].from_tag = from_tag; >> + tip_table.table[tip_table.nr].deref = deref; >> tip_table.nr++; >> tip_table.sorted = 0; >> } >> @@ -271,12 +280,30 @@ static int tipcmp(const void *a_, const void *b_) >> return oidcmp(&a->oid, &b->oid); >> } >> >> +static int cmp_by_tag_and_age(const void *a_, const void *b_) >> +{ >> + const struct tip_table_entry *a = a_, *b = b_; >> + int cmp; >> + >> + /* Prefer tags. */ >> + cmp = b->from_tag - a->from_tag; > > We end up with negative -1 if b is not from tag and a is from tag, > even though the from_tag field is unsigned, so this is not wrong > per-se but feels a bit subtle. Integer promotion -- I admit I looked it up just before sending the patch to be sure C does the right thing here. Comparing explicitly would be more readable. > >> + if (cmp) >> + return cmp; >> + >> + /* Older is better. */ >> + if (a->taggerdate < b->taggerdate) >> + return -1; > > We are here if both are from tag or neither is from tag. Mental > note: let's make sure that add_to_tip_table() is always called with > usable timestamp even for a commit that is not from tag. > >> + return a->taggerdate != b->taggerdate; > > OK, we know a is either of the same age as or younger than b at this > point, so we would return 1 if a is not the same age as b. > >> static int name_ref(const char *path, const struct object_id *oid, int flags, void *cb_data) >> { >> struct object *o = parse_object(the_repository, oid); >> struct name_ref_data *data = cb_data; >> int can_abbreviate_output = data->tags_only && data->name_only; >> int deref = 0; >> + int from_tag = 0; >> + struct commit *commit = NULL; Please remember this line. >> timestamp_t taggerdate = TIME_MAX; >> >> if (data->tags_only && !starts_with(path, "refs/tags/")) >> @@ -325,8 +352,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo >> return 0; >> } >> >> - add_to_tip_table(oid, path, can_abbreviate_output); >> - >> while (o && o->type == OBJ_TAG) { >> struct tag *t = (struct tag *) o; >> if (!t->tagged) >> @@ -336,17 +361,35 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo >> taggerdate = t->date; >> } >> if (o && o->type == OBJ_COMMIT) { >> - struct commit *commit = (struct commit *)o; >> - int from_tag = starts_with(path, "refs/tags/"); >> - >> + commit = (struct commit *)o; >> + from_tag = starts_with(path, "refs/tags/"); >> if (taggerdate == TIME_MAX) >> taggerdate = commit->date; > > OK, so a lightweight tag gets the commit date, while an annotated > tag gets the tagger date (while peeling in the loop just above this > part). > > I never make an annotated tag long after creating the commit the tag > refers to myself (i.e. on the day I tag a release or a release > candidate, I know that the commit I am creating is what I will tag > even before creating the commit, and I tag the commit soon after > that), but I can imagine that in some other workflows the > maintainers may be tagging long after the commit was made, and more > importantly, the interval of time between comitting and tagging > might be different from release to release. I wonder if it mimicks > the topological ordering better if always compare the timestamps of > underlying commits. I also don't understand why lightweight tags are preferred over tag objects both by having from_tag set and by having an older timestamp. In my mind an annotated tag should have more weight since it's harder to create. > >> - path = name_ref_abbrev(path, can_abbreviate_output); >> - name_rev(commit, path, taggerdate, from_tag, deref); >> } >> + >> + add_to_tip_table(oid, path, can_abbreviate_output, commit, taggerdate, >> + from_tag, deref); >> return 0; >> } >> >> +static void name_tips(void) >> +{ >> + int i; >> + >> + /* >> + * Try to set better names first, so that worse ones spread >> + * less. >> + */ >> + QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age); >> + for (i = 0; i < tip_table.nr; i++) { >> + struct tip_table_entry *e = &tip_table.table[i]; >> + if (e->commit) { > > Sorry, I am confused. I didn't see anything that clears > tip_table.table[i].commit in the code. The line we remembered initializes commit to NULL. It stays NULL for refs that don't resolve to commits. > >> + name_rev(e->commit, e->refname, e->taggerdate, >> + e->from_tag, e->deref); >> + } >> + } >> +} >> + >> static const unsigned char *nth_tip_table_ent(size_t ix, void *table_) >> { >> struct tip_table_entry *table = table_; >> @@ -559,6 +602,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) >> cutoff = TIME_MIN; >> } >> for_each_ref(name_ref, &data); >> + name_tips(); >> >> if (transform_stdin) { >> char buffer[2048]; >> -- >> 2.25.0
diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 23a639ff30..a9dcd25e46 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -247,6 +247,10 @@ static struct tip_table { struct tip_table_entry { struct object_id oid; const char *refname; + struct commit *commit; + timestamp_t taggerdate; + unsigned int from_tag:1; + unsigned int deref:1; } *table; int nr; int alloc; @@ -254,13 +258,18 @@ static struct tip_table { } tip_table; static void add_to_tip_table(const struct object_id *oid, const char *refname, - int shorten_unambiguous) + int shorten_unambiguous, struct commit *commit, + timestamp_t taggerdate, int from_tag, int deref) { refname = name_ref_abbrev(refname, shorten_unambiguous); ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc); oidcpy(&tip_table.table[tip_table.nr].oid, oid); tip_table.table[tip_table.nr].refname = xstrdup(refname); + tip_table.table[tip_table.nr].commit = commit; + tip_table.table[tip_table.nr].taggerdate = taggerdate; + tip_table.table[tip_table.nr].from_tag = from_tag; + tip_table.table[tip_table.nr].deref = deref; tip_table.nr++; tip_table.sorted = 0; } @@ -271,12 +280,30 @@ static int tipcmp(const void *a_, const void *b_) return oidcmp(&a->oid, &b->oid); } +static int cmp_by_tag_and_age(const void *a_, const void *b_) +{ + const struct tip_table_entry *a = a_, *b = b_; + int cmp; + + /* Prefer tags. */ + cmp = b->from_tag - a->from_tag; + if (cmp) + return cmp; + + /* Older is better. */ + if (a->taggerdate < b->taggerdate) + return -1; + return a->taggerdate != b->taggerdate; +} + static int name_ref(const char *path, const struct object_id *oid, int flags, void *cb_data) { struct object *o = parse_object(the_repository, oid); struct name_ref_data *data = cb_data; int can_abbreviate_output = data->tags_only && data->name_only; int deref = 0; + int from_tag = 0; + struct commit *commit = NULL; timestamp_t taggerdate = TIME_MAX; if (data->tags_only && !starts_with(path, "refs/tags/")) @@ -325,8 +352,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo return 0; } - add_to_tip_table(oid, path, can_abbreviate_output); - while (o && o->type == OBJ_TAG) { struct tag *t = (struct tag *) o; if (!t->tagged) @@ -336,17 +361,35 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo taggerdate = t->date; } if (o && o->type == OBJ_COMMIT) { - struct commit *commit = (struct commit *)o; - int from_tag = starts_with(path, "refs/tags/"); - + commit = (struct commit *)o; + from_tag = starts_with(path, "refs/tags/"); if (taggerdate == TIME_MAX) taggerdate = commit->date; - path = name_ref_abbrev(path, can_abbreviate_output); - name_rev(commit, path, taggerdate, from_tag, deref); } + + add_to_tip_table(oid, path, can_abbreviate_output, commit, taggerdate, + from_tag, deref); return 0; } +static void name_tips(void) +{ + int i; + + /* + * Try to set better names first, so that worse ones spread + * less. + */ + QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age); + for (i = 0; i < tip_table.nr; i++) { + struct tip_table_entry *e = &tip_table.table[i]; + if (e->commit) { + name_rev(e->commit, e->refname, e->taggerdate, + e->from_tag, e->deref); + } + } +} + static const unsigned char *nth_tip_table_ent(size_t ix, void *table_) { struct tip_table_entry *table = table_; @@ -559,6 +602,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) cutoff = TIME_MIN; } for_each_ref(name_ref, &data); + name_tips(); if (transform_stdin) { char buffer[2048];
name_ref() is called for each ref and checks if its a better name for the referenced commit. If that's the case it remembers it and checks if a name based on it is better for its ancestors as well. This in done in the the order for_each_ref() imposes on us. That might not be optimal. If bad names happen to be encountered first (as defined by is_better_name()), names derived from them may spread to a lot of commits, only to be replaced by better names later. Setting better names first can avoid that. is_better_name() prefers tags, short distances and old references. The distance is a measure that we need to calculate for each candidate commit, but the other two properties are not dependent on the relationships of commits. Sorting the refs by them should yield better performance than the essentially random order we currently use. And applying older references first should also help to reduce rework due to the fact that older commits have less ancestors than newer ones. So add all details of names to the tip table first, then sort them to prefer tags and older references and then apply them in this order. Here's the performance as measures by hyperfine for the Linux repo before: Benchmark #1: ./git -C ../linux/ name-rev --all Time (mean ± σ): 851.1 ms ± 4.5 ms [User: 806.7 ms, System: 44.4 ms] Range (min … max): 845.9 ms … 859.5 ms 10 runs ... and with this patch: Benchmark #1: ./git -C ../linux/ name-rev --all Time (mean ± σ): 736.2 ms ± 8.7 ms [User: 688.4 ms, System: 47.5 ms] Range (min … max): 726.0 ms … 755.2 ms 10 runs Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/name-rev.c | 60 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 8 deletions(-) -- 2.25.0