{"thread":{"id":"52308","subject":"[PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk","startedAt":"2019-11-22T08:54:31Z","lastAt":"2019-12-04T19:47:30Z","messageCount":7,"participants":["Mike Hommey","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"386809","messageId":"20191122083704.29267-2-mh@glandium.org","threadId":"52308","inReplyTo":"20191122083704.29267-1-mh@glandium.org","subject":"[PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-11-22T08:37:04Z","receivedAt":"2019-11-22T08:54:31Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"init_topo_walk doesn't reuse an existing topo_walk_info, and currently\nleaks the one that might exist on the current rev_info if it was already\nused for a topo walk beforehand.\n\nSigned-off-by: Mike Hommey \n---\n revision.c | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\n\nThe FREE_AND_NULL() -> xmalloc dance could be avoided, but I figured the\nfunction ought to be reused in the future to clear the rev_info. I was\nthinking to add a call in reset_revision_walk instead, but\nreset_revision_walk doesn't take an argument at all currently.\n\n\ndiff --git a/revision.c b/revision.c\nindex 765a56ae33..7e23c5ed08 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3211,10 +3211,26 @@ static void compute_indegrees_to_depth(struct rev_info *revs,\n \t\tindegree_walk_step(revs);\n }\n \n+static void reset_topo_walk(struct rev_info *revs)\n+{\n+\tstruct topo_walk_info *info = revs->topo_walk_info;\n+\n+\tclear_prio_queue(&info->explore_queue);\n+\tclear_prio_queue(&info->indegree_queue);\n+\tclear_prio_queue(&info->topo_queue);\n+\tclear_indegree_slab(&info->indegree);\n+\tclear_author_date_slab(&info->author_date);\n+\n+\tFREE_AND_NULL(revs->topo_walk_info);\n+}\n+\n static void init_topo_walk(struct rev_info *revs)\n {\n \tstruct topo_walk_info *info;\n \tstruct commit_list *list;\n+\tif (revs->topo_walk_info)\n+\t\treset_topo_walk(revs);\n+\n \trevs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));\n \tinfo = revs->topo_walk_info;\n \tmemset(info, 0, sizeof(struct topo_walk_info));\n-- \n2.24.0.dirty\n\n"},{"id":"386810","messageId":"20191122083704.29267-1-mh@glandium.org","threadId":"52308","inReplyTo":null,"subject":"[PATCH 1/2] revision: clear the topo-walk flags in reset_revision_walk","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-11-22T08:37:03Z","receivedAt":"2019-11-22T08:54:35Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Not doing so can lead to wrong topo-walks when using the revision walk API\nconsecutively.\n\nSigned-off-by: Mike Hommey \n---\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nAs mentioned in 20191108025007.bphr7ynvskeoe6tb@glandium.org, it feels\nlike maybe TOPO_WALK_EXPLORED and TOPO_WALK_INDEGREE should be in\nALL_REV_FLAGS too?\n\ndiff --git a/revision.c b/revision.c\nindex 0e39b2b8a5..765a56ae33 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3098,7 +3098,7 @@ static void set_children(struct rev_info *revs)\n \n void reset_revision_walk(void)\n {\n-\tclear_object_flags(SEEN | ADDED | SHOWN);\n+\tclear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);\n }\n \n static int mark_uninteresting(const struct object_id *oid,\n-- \n2.24.0.dirty\n\n"},{"id":"387156","messageId":"34a0ae8e-a0f3-e263-e386-2fbbc49f0d07@gmail.com","threadId":"52308","inReplyTo":"20191122083704.29267-1-mh@glandium.org","subject":"Re: [PATCH 1/2] revision: clear the topo-walk flags in reset_revision_walk","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-11-27T14:22:20Z","receivedAt":"2019-11-27T14:22:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/22/2019 3:37 AM, Mike Hommey wrote:\n> Not doing so can lead to wrong topo-walks when using the revision walk API\n> consecutively.\n> \n> Signed-off-by: Mike Hommey \n> ---\n> revision.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> As mentioned in 20191108025007.bphr7ynvskeoe6tb@glandium.org, it feels\n> like maybe TOPO_WALK_EXPLORED and TOPO_WALK_INDEGREE should be in\n> ALL_REV_FLAGS too?\n> \n> diff --git a/revision.c b/revision.c\n> index 0e39b2b8a5..765a56ae33 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -3098,7 +3098,7 @@ static void set_children(struct rev_info *revs)\n> \n> void reset_revision_walk(void)\n> {\n> -\tclear_object_flags(SEEN | ADDED | SHOWN);\n> +\tclear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);\n\nLooks good. Thanks for catching and fixing this!\n\n-Stolee\n"},{"id":"387157","messageId":"46c47a21-d0e1-296f-5563-ae29597f995c@gmail.com","threadId":"52308","inReplyTo":"20191122083704.29267-2-mh@glandium.org","subject":"Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-11-27T14:25:09Z","receivedAt":"2019-11-27T14:25:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/22/2019 3:37 AM, Mike Hommey wrote:> diff --git a/revision.c b/revision.c\n> index 765a56ae33..7e23c5ed08 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -3211,10 +3211,26 @@ static void compute_indegrees_to_depth(struct rev_info *revs,\n> \t\tindegree_walk_step(revs);\n> }\n> \n> +static void reset_topo_walk(struct rev_info *revs)\n> +{\n> +\tstruct topo_walk_info *info = revs->topo_walk_info;\n> +\n> +\tclear_prio_queue(&info->explore_queue);\n> +\tclear_prio_queue(&info->indegree_queue);\n> +\tclear_prio_queue(&info->topo_queue);\n> +\tclear_indegree_slab(&info->indegree);\n\nIn general I like this change. I'm happy that this was split into a\nmethod instead of crammed into the block of the \"if\" below.\n\n> +\tclear_author_date_slab(&info->author_date);\n\nThe only issue I have is that the author_date slab should not be\ncleared. That is used by more than the topo-walk AND the values for\nauthor dates will not change between subsequent revision walks. Just\ndrop that line and we should be good to go!\n\n> +\n> +\tFREE_AND_NULL(revs->topo_walk_info);\n> +}\n> +\n> static void init_topo_walk(struct rev_info *revs)\n> {\n> \tstruct topo_walk_info *info;\n> \tstruct commit_list *list;\n> +\tif (revs->topo_walk_info)\n> +\t\treset_topo_walk(revs);\n> +\n> \trevs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));\n> \tinfo = revs->topo_walk_info;\n> \tmemset(info, 0, sizeof(struct topo_walk_info));\n\nThanks,\n-Stolee\n"},{"id":"387377","messageId":"xmqqsgm4yq0m.fsf@gitster-ct.c.googlers.com","threadId":"52308","inReplyTo":"46c47a21-d0e1-296f-5563-ae29597f995c@gmail.com","subject":"Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-01T16:22:33Z","receivedAt":"2019-12-01T16:22:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> In general I like this change. I'm happy that this was split into a\n> method instead of crammed into the block of the \"if\" below.\n>\n>> +\tclear_author_date_slab(&info->author_date);\n>\n> The only issue I have is that the author_date slab should not be\n> cleared. That is used by more than the topo-walk AND the values for\n> author dates will not change between subsequent revision walks. Just\n> drop that line and we should be good to go!\n\nHmph, isn't this merely a performance thing, or would a slab that\nwas once cleared never repopulate upon its second use (i.e.\naffecting correctness)?\n\nThanks.\n"},{"id":"387503","messageId":"ba4095d7-e142-c9a4-ea41-04789be84d0b@gmail.com","threadId":"52308","inReplyTo":"xmqqsgm4yq0m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-04T18:55:20Z","receivedAt":"2019-12-04T18:55:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/1/2019 11:22 AM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> In general I like this change. I'm happy that this was split into a\n>> method instead of crammed into the block of the \"if\" below.\n>>\n>>> +\tclear_author_date_slab(&info->author_date);\n>>\n>> The only issue I have is that the author_date slab should not be\n>> cleared. That is used by more than the topo-walk AND the values for\n>> author dates will not change between subsequent revision walks. Just\n>> drop that line and we should be good to go!\n> \n> Hmph, isn't this merely a performance thing, or would a slab that\n> was once cleared never repopulate upon its second use (i.e.\n> affecting correctness)?\n\nYes, this is only a performance thing. If you think it is safest to\nclear it here, then it can stay.\n\n-Stolee\n"},{"id":"387507","messageId":"xmqq7e3b50vo.fsf@gitster-ct.c.googlers.com","threadId":"52308","inReplyTo":"ba4095d7-e142-c9a4-ea41-04789be84d0b@gmail.com","subject":"Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-04T19:47:23Z","receivedAt":"2019-12-04T19:47:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 12/1/2019 11:22 AM, Junio C Hamano wrote:\n>> Derrick Stolee writes:\n>> \n>>> In general I like this change. I'm happy that this was split into a\n>>> method instead of crammed into the block of the \"if\" below.\n>>>\n>>>> +\tclear_author_date_slab(&info->author_date);\n>>>\n>>> The only issue I have is that the author_date slab should not be\n>>> cleared. That is used by more than the topo-walk AND the values for\n>>> author dates will not change between subsequent revision walks. Just\n>>> drop that line and we should be good to go!\n>> \n>> Hmph, isn't this merely a performance thing, or would a slab that\n>> was once cleared never repopulate upon its second use (i.e.\n>> affecting correctness)?\n>\n> Yes, this is only a performance thing. If you think it is safest to\n> clear it here, then it can stay.\n\nLet's keep what is already in 'next' ;-) and possibly follow-up with\na separate patch to remove this line, justfying the change as\nperformance improvement.\n\nThanks.\n"}]}