| Message ID | 20191122083704.29267-2-mh@glandium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] revision: clear the topo-walk flags in reset_revision_walk | expand |
On 11/22/2019 3:37 AM, Mike Hommey wrote:> diff --git a/revision.c b/revision.c > index 765a56ae33..7e23c5ed08 100644 > --- a/revision.c > +++ b/revision.c > @@ -3211,10 +3211,26 @@ static void compute_indegrees_to_depth(struct rev_info *revs, > indegree_walk_step(revs); > } > > +static void reset_topo_walk(struct rev_info *revs) > +{ > + struct topo_walk_info *info = revs->topo_walk_info; > + > + clear_prio_queue(&info->explore_queue); > + clear_prio_queue(&info->indegree_queue); > + clear_prio_queue(&info->topo_queue); > + clear_indegree_slab(&info->indegree); In general I like this change. I'm happy that this was split into a method instead of crammed into the block of the "if" below. > + clear_author_date_slab(&info->author_date); The only issue I have is that the author_date slab should not be cleared. That is used by more than the topo-walk AND the values for author dates will not change between subsequent revision walks. Just drop that line and we should be good to go! > + > + FREE_AND_NULL(revs->topo_walk_info); > +} > + > static void init_topo_walk(struct rev_info *revs) > { > struct topo_walk_info *info; > struct commit_list *list; > + if (revs->topo_walk_info) > + reset_topo_walk(revs); > + > revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info)); > info = revs->topo_walk_info; > memset(info, 0, sizeof(struct topo_walk_info)); Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > In general I like this change. I'm happy that this was split into a > method instead of crammed into the block of the "if" below. > >> + clear_author_date_slab(&info->author_date); > > The only issue I have is that the author_date slab should not be > cleared. That is used by more than the topo-walk AND the values for > author dates will not change between subsequent revision walks. Just > drop that line and we should be good to go! Hmph, isn't this merely a performance thing, or would a slab that was once cleared never repopulate upon its second use (i.e. affecting correctness)? Thanks.
On 12/1/2019 11:22 AM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> In general I like this change. I'm happy that this was split into a >> method instead of crammed into the block of the "if" below. >> >>> + clear_author_date_slab(&info->author_date); >> >> The only issue I have is that the author_date slab should not be >> cleared. That is used by more than the topo-walk AND the values for >> author dates will not change between subsequent revision walks. Just >> drop that line and we should be good to go! > > Hmph, isn't this merely a performance thing, or would a slab that > was once cleared never repopulate upon its second use (i.e. > affecting correctness)? Yes, this is only a performance thing. If you think it is safest to clear it here, then it can stay. -Stolee
Derrick Stolee <stolee@gmail.com> writes: > On 12/1/2019 11:22 AM, Junio C Hamano wrote: >> Derrick Stolee <stolee@gmail.com> writes: >> >>> In general I like this change. I'm happy that this was split into a >>> method instead of crammed into the block of the "if" below. >>> >>>> + clear_author_date_slab(&info->author_date); >>> >>> The only issue I have is that the author_date slab should not be >>> cleared. That is used by more than the topo-walk AND the values for >>> author dates will not change between subsequent revision walks. Just >>> drop that line and we should be good to go! >> >> Hmph, isn't this merely a performance thing, or would a slab that >> was once cleared never repopulate upon its second use (i.e. >> affecting correctness)? > > Yes, this is only a performance thing. If you think it is safest to > clear it here, then it can stay. Let's keep what is already in 'next' ;-) and possibly follow-up with a separate patch to remove this line, justfying the change as performance improvement. Thanks.
diff --git a/revision.c b/revision.c index 765a56ae33..7e23c5ed08 100644 --- a/revision.c +++ b/revision.c @@ -3211,10 +3211,26 @@ static void compute_indegrees_to_depth(struct rev_info *revs, indegree_walk_step(revs); } +static void reset_topo_walk(struct rev_info *revs) +{ + struct topo_walk_info *info = revs->topo_walk_info; + + clear_prio_queue(&info->explore_queue); + clear_prio_queue(&info->indegree_queue); + clear_prio_queue(&info->topo_queue); + clear_indegree_slab(&info->indegree); + clear_author_date_slab(&info->author_date); + + FREE_AND_NULL(revs->topo_walk_info); +} + static void init_topo_walk(struct rev_info *revs) { struct topo_walk_info *info; struct commit_list *list; + if (revs->topo_walk_info) + reset_topo_walk(revs); + revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info)); info = revs->topo_walk_info; memset(info, 0, sizeof(struct topo_walk_info));
init_topo_walk doesn't reuse an existing topo_walk_info, and currently leaks the one that might exist on the current rev_info if it was already used for a topo walk beforehand. Signed-off-by: Mike Hommey <mh@glandium.org> --- revision.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) The FREE_AND_NULL() -> xmalloc dance could be avoided, but I figured the function ought to be reused in the future to clear the rev_info. I was thinking to add a call in reset_revision_walk instead, but reset_revision_walk doesn't take an argument at all currently.