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

[Nicolai Hähnle]

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>
>>>
>>> It calling itself recursively prevented it from being inlined, resulting
>>> in a copy being generated in every compilation unit referencing it. This
>>> bloated the text segment of the Gallium mega-driver *_dri.so by ~4%,
>>> and might also have impacted performance.
>>
>> Did you update the size measurement?
> 
> Yes, since I measured ~4.3%, I figured it's better to go for
> understatement than exaggeration after all. :)
> 
> 
>>> 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.

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?

Cheers,
Nicolai


> Let me know of you spot another difference in behaviour before and after
> the patch.
> 
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.