[Mesa-dev] [PATCH v3] gallium/util: Break recursion in pipe_resource_reference

[Michel Dänzer]

On 14/06/17 05:42 PM, Nicolai Hähnle wrote:
> On 14.06.2017 10:05, Michel Dänzer wrote:
>> On 14/06/17 04:42 PM, Nicolai Hähnle wrote:
>>> On 14.06.2017 05:39, Michel Dänzer wrote:
>>>> From: Michel Dänzer <michel.daenzer at amd.com>
>>>>
>>>> diff --git a/src/gallium/auxiliary/util/u_inlines.h
>>>> b/src/gallium/auxiliary/util/u_inlines.h
>>>> index 6a3d5043cf..08326da03d 100644
>>>> --- a/src/gallium/auxiliary/util/u_inlines.h
>>>> +++ b/src/gallium/auxiliary/util/u_inlines.h
>>>> @@ -137,8 +137,19 @@ pipe_resource_reference(struct pipe_resource
>>>> **ptr, struct pipe_resource *tex)
>>>>         if (pipe_reference_described(&(*ptr)->reference,
>>>> &tex->reference,
>>>>                                  
>>>> (debug_reference_descriptor)debug_describe_resource)) {
>>>> -      pipe_resource_reference(&old_tex->next, NULL);
>>>> +      struct pipe_resource *next = old_tex->next;
>>>> +
>>>>          old_tex->screen->resource_destroy(old_tex->screen, old_tex);
>>>> +
>>>> +      /* Avoid recursion, which would prevent inlining this
>>>> function */
>>>> +      while (next) {
>>>> +         old_tex = next;
>>>> +         next = old_tex->next;
>>>> +         old_tex->next = NULL;
>>>
>>> Is this really necessary? I don't think so, but if it is, it's probably
>>> necessary above as well.
>>
>> The old_tex->next = NULL assignment is necessary here to complement the
>> pipe_reference_described call below it. It's not necessary above,
>> because the ->next chain only needs to be unreferenced when the "master"
>> resource is destroyed.
>>
>> The behaviour is basically the same as with the recursive
>> pipe_resource_reference(&old_tex->next, NULL) call before: the next
>> pointers are left untouched unless the "master" resource is destroyed,
>> in which case the next pointers are set to NULL and the reference counts
>> of the corresponding sub-resources decremented (and the sub-resource
>> destroyed if it goes to 0). The only slight change is that the next
>> member of the "master" resource isn't set to NULL, but that doesn't
>> matter because it's getting destroyed anyway.
> 
> Well, if (*ptr)->next is _not_ destroyed, then the new code will set
> (*ptr)->next->next to NULL anyway, while the old code would not have
> modified it.

You're right. I'm not sure it makes a difference in practice, but let's
be conservative, as that allows making the patch (and the generated
code) even smaller:


> Come to think of it, the new code should really break out of the loop
> once pipe_reference_described returns false. Maybe:
> 
> do {
>    struct pipe_resource *next = old_tex->next;
>    old_tex->screen->resource_destroy(old_tex->screen, old_tex);
>    old_tex = next;
> } while (old_tex && pipe_reference_described(&old_tex->reference, NULL,
> ...));
> 
> How does that sound?

Great! I took your suggestion almost verbatim (no need to test old_tex
separately before calling pipe_reference_described) for v4, thanks.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer