klib icon indicating copy to clipboard operation
klib copied to clipboard

Remove invalid optimization from put that negatively impacts performance

Open dwforbes opened this issue 8 years ago • 3 comments

kh_put_##name has an early check for an initial empty bucket that negatively impacts significant use, while only benefiting unrealistically simplistic benchmarks (e.g. inserting a single item).

As an example, for k-nucleotide, with this revision on an arbitrary test machine the entire process completes in ~3.3s real time, with ~9.5s of user time. Prior to this revision it completes in ~3.9s, with ~11.0s of user time.

dwforbes avatar Jun 06 '17 14:06 dwforbes

Do you mean "decreases flag memory usage"?

trapexit avatar Jun 06 '17 16:06 trapexit

The first commit -- 9b12aa4 -- was the only one I intended to get captured in the pull request, in that case removing an unnecessary extra conditional that adds overhead (but was seemingly added as an optimization), discovered during some quick analysis regarding the performance of C in the nucleotide benchmark game.

It is a change that can only be positive, even for projects that might have poorly considered dependencies on internals.

The second commit -- 348601f -- is a potentially breaking change that reduces the amount of bit packing, increasing the memory footprint for flags (albeit still a tiny overhead for all but the most enormous of hash tables), however it has a dramatic benefit to performance on all architectures because it removes three expensive bit shifts per loop evaluation. It can break uses that might have a direct hard-fixed dependencies on the structure of flags, for instance (though any that used the macros provided would be fine), so it was more an example of cost-benefit analysis of memory versus performance.

dwforbes avatar Jun 06 '17 17:06 dwforbes

Both of these patches are great additions

larseggert avatar Aug 14 '19 12:08 larseggert