[Go] Dictionary unifier uses unnecessarily expensive getvalFn

[brancz]

Describe the enhancement requested

Our profiling data shows clearly that even though dictionary unification is working brilliantly, there is still plenty of CPU time that is used that is unnecessary. Take this profiling data: https://pprof.me/6681ca5

This shows that ~43% is spent because of convTslice within getvalFn, because the value has to be treated as an interface{} and is then subsequently added to the MemoTable using non-type-safe GetOrInsert.

I see 3 potential ways forward:

1. Somehow optimize this away entirely, though I don't see how.
2. Offer a specific DictionaryUnifier per type (these could be added one by one as they're needed).
3. Move hashing out of the internal package so that people can build their own DictionaryUnifiers.

@zeroshade thoughts?

Component(s)

Go

[brancz]

I think I would tend towards 2 and 3, but don't feel strongly. Either way, I'm already starting to implement a BinaryDictionaryUnifier, which I'm happy to contribute.

[zeroshade]

So, @brancz I think it's been long enough at this point that I'd be fine with updating the Arrow lib to go1.18+ and introducing generics right into the main library (since go1.21 is officially out, the standard would be that we can go up to 1.19 safely). Do you want to try your hand at upgrading the unifier / memo table / etc. to fully leverage generics for the performance improvements?

[brancz]

I have a feeling you must have thought about this much much longer than I have, for me that's a pretty unknown amount of time to spend, and I won't be able to get to something like that for the next 1-2 months.

[zeroshade]

Haha, perfectly fine. It's on my list of things to do, particularly because of the performance potential. In the meantime, I agree with you that both solutions 2 and 3 are viable ideas. I'll definitely take a look at review the PR you added. I guess whichever one of us can manage to get to this first will put up a PR for it 😄

If I do get around to this, I'll make sure to tag you on the PR since I'd be interested to see if you notice any performance improvements in your use cases (might even be interesting to see if you could simplify some of your use cases enough to contribute potential benchmark functions).

[brancz]

If I do get around to this, I'll make sure to tag you on the PR since I'd be interested to see if you notice any performance improvements in your use cases (might even be interesting to see if you could simplify some of your use cases enough to contribute potential benchmark functions).

I wish ... unfortunately I don't even have local benchmarks for my code for this. It's all coming from profiling in production and using that data to inform changes.

Haha, perfectly fine. It's on my list of things to do, particularly because of the performance potential. In the meantime, I agree with you that both solutions 2 and 3 are viable ideas. I'll definitely take a look at review the PR you added. I guess whichever one of us can manage to get to this first will put up a PR for it 😄

Amazing! Sounds good!