[PATCH 1/2] revision: clear the topo-walk flags in reset_revision_walk

[PATCH 1/2] revision: clear the topo-walk flags in reset_revision_walk

[Mike Hommey]

Not doing so can lead to wrong topo-walks when using the revision walk API
consecutively.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

As mentioned in 20191108025007.bphr7ynvskeoe6tb@glandium.org, it feels
like maybe TOPO_WALK_EXPLORED and TOPO_WALK_INDEGREE should be in
ALL_REV_FLAGS too?

diff --git a/revision.c b/revision.c
index 0e39b2b8a5..765a56ae33 100644
--- a/revision.c
+++ b/revision.c
@@ -3098,7 +3098,7 @@ static void set_children(struct rev_info *revs)
 
 void reset_revision_walk(void)
 {
-	clear_object_flags(SEEN | ADDED | SHOWN);
+	clear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);
 }
 
 static int mark_uninteresting(const struct object_id *oid,
-- 
2.24.0.dirty

[PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk

[Mike Hommey]

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.


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));
-- 
2.24.0.dirty

Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk

[Derrick Stolee]

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

Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk

[Junio C Hamano]

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.

Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk

[Derrick Stolee]

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

Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk

[Junio C Hamano]

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.

Re: [PATCH 1/2] revision: clear the topo-walk flags in reset_revision_walk

[Derrick Stolee]

On 11/22/2019 3:37 AM, Mike Hommey wrote:
> Not doing so can lead to wrong topo-walks when using the revision walk API
> consecutively.
> 
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  revision.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> As mentioned in 20191108025007.bphr7ynvskeoe6tb@glandium.org, it feels
> like maybe TOPO_WALK_EXPLORED and TOPO_WALK_INDEGREE should be in
> ALL_REV_FLAGS too?
> 
> diff --git a/revision.c b/revision.c
> index 0e39b2b8a5..765a56ae33 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3098,7 +3098,7 @@ static void set_children(struct rev_info *revs)
>  
>  void reset_revision_walk(void)
>  {
> -	clear_object_flags(SEEN | ADDED | SHOWN);
> +	clear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);

Looks good. Thanks for catching and fixing this!

-Stolee