attractivechaos / klib

A standalone and lightweight C library
http://attractivechaos.github.io/klib/
MIT License
4.18k stars 556 forks source link

Remove invalid optimization from put that negatively impacts performance #86

Open dwforbes opened 7 years ago

dwforbes commented 7 years ago

khput##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.

trapexit commented 7 years ago

Do you mean "decreases flag memory usage"?

dwforbes commented 7 years ago

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.

larseggert commented 5 years ago

Both of these patches are great additions