On 2015-08-12 16:25:17 -0400, Robert Haas wrote:
> On Wed, Aug 12, 2015 at 2:57 PM, Andres Freund <andres@anarazel.de> wrote:
> > The only actual separate patch since then (fastgetattr as inline
> > function) was posted 2015-08-05 and I yesterday suggested to push it
> > today. And it's just replacing two existing macros by inline functions.
>
> I'm a little concerned about that one. Your testing shows that's a
> win for you, but is it a win for everyone? Maybe we should go ahead
> and do it anyway, but I'm not sure.
I don't think it's likely to affect performance in any significant way
in either direction. I mean, the absolute worst case would be that a
formerly in-place fastgetattr() call gets changed into a function call
in the same translation unit. That might or might not have a performance
impact in either direction, but it's not going to be large. The only
reason this has improved performance is imo that it shrank the code size
a bit (increasing cache hit ratio, increase use of the branch prediction
unit etc.).
In all the profiles I've seen where fastgetattr (or rather the in-place
code it has) is responsible for some portion of the runtime it was the
actual looping, the cache misses, et al, not the stack and the indirect
call. It's debatable that it's inline (via macro or inline function) in
the first place.
The advantage is not performance, it's readability. I've several times
re-wrapped fastgetattr() just to understand what it does, because I
otherwise find the code so hard to read. Maybe I just utterly suck at
reading macros...
You might argue that it's nothing we have touched frequently. And you're
right. But I think that's a mistake. We spend far too much time in the
various pieces of code dissembling tuples, and I think at some point
somebody really needs to spend time on this.
Regards,
Andres
