Thread: move hash_any to utils/hash/hashfn.c
I just noticed (once again) that we have hash_any() declared in src/access/hash.h (the header file for the hash index AM) rather than somewhere in utils/, for no good reason other than perhaps there was no better place when it was introduced. This means that some files that seem to just need hash_any end up pointlessly #including the whole AM world upon themselves. Would anybody object too hard if I move hash_any() and friends to src/backend/utils/hash/hashfn.c and the declarations to src/include/utils/hashutils.h? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-01-25 16:35:18 -0300, Alvaro Herrera wrote: > I just noticed (once again) that we have hash_any() declared in > src/access/hash.h (the header file for the hash index AM) rather than > somewhere in utils/, for no good reason other than perhaps there was no > better place when it was introduced. This means that some files that > seem to just need hash_any end up pointlessly #including the whole AM > world upon themselves. > > Would anybody object too hard if I move hash_any() and friends to > src/backend/utils/hash/hashfn.c and the declarations to > src/include/utils/hashutils.h? I hate the current split quite a bit, albeit for somewhat different reasons. We make things like tag_hash, uint32_hash unnecessarily more expensive by adding an entirely useless external function call. And some of these can be fairly hot (e.g. for syscache). So yea, let's move this stuff together. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2019-01-25 16:35:18 -0300, Alvaro Herrera wrote:
>> Would anybody object too hard if I move hash_any() and friends to
>> src/backend/utils/hash/hashfn.c and the declarations to
>> src/include/utils/hashutils.h?
> I hate the current split quite a bit, albeit for somewhat different
> reasons. We make things like tag_hash, uint32_hash unnecessarily more
> expensive by adding an entirely useless external function call. And
> some of these can be fairly hot (e.g. for syscache). So yea, let's
> move this stuff together.
+1
regards, tom lane
Hello
On 2019-Jan-25, Andres Freund wrote:
> I hate the current split quite a bit, albeit for somewhat different
> reasons. We make things like tag_hash, uint32_hash unnecessarily more
> expensive by adding an entirely useless external function call. And
> some of these can be fairly hot (e.g. for syscache). So yea, let's
> move this stuff together.
Here's a patch, but I haven't done anything that would change tag_hash
et al. Looking at the new hashfn.s, it looks like this:
tag_hash:
.LFB42:
.loc 1 681 0
.cfi_startproc
.LVL310:
.loc 1 682 0
call hash_any
.LVL311:
.loc 1 684 0
rep ret
.cfi_endproc
In master, it looks like this instead:
tag_hash:
.LFB96:
.loc 1 53 0
.cfi_startproc
.LVL5:
subq $8, %rsp
.cfi_def_cfa_offset 16
.loc 1 54 0
call hash_any@PLT
.LVL6:
.loc 1 56 0
addq $8, %rsp
.cfi_def_cfa_offset 8
ret
.cfi_endproc
I don't know if the change is significant. Obviously we lost the
subq/addq (probably nothing to be excited about) but there's also the
@PLT in the call ... I don't know what that means.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 2019-Jan-25, Alvaro Herrera wrote: > Would anybody object too hard if I move hash_any() and friends to > src/backend/utils/hash/hashfn.c and the declarations to > src/include/utils/hashutils.h? Pushed this. I ended up adding an #include of utils/hashutils.h to access/hash.h, so that any third-party code using hash_any() from the latter continues to build unbroken. I don't think this is terribly problematic -- the only issue is that those projects won't become updated to use the leaner file, but I don't think we care too much about that. If I do get pushback about this change I can definitely remove that line and watch the fireworks. Anyway, let's see what *else* breaks now ... -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services