util: move MapIntoRange() for reuse in fuzz tests #23760

No description provided.

This technique is actually used in a few other places in the codebase, though this is the only one that does it for 64-bit inputs and ranges; at least in cache::compute_hashes in cuckoocache.h and FastMod in common/bloom.cpp. It's also used in bench/nanobench.h, but I think that file is intended to stay dependencyless.

Perhaps it would make sense to have specific functions for both (e.g. FastRange32, FastRange64) and put them in an integer util file or so?

Code review ACK https://github.com/bitcoin/bitcoin/pull/23760/commits/a8dc99f21cafaa8d37ea16bf9441872d5d653e09

Mergeable as it stands IMO, but what @sipa's saying probably makes sense too.

Concept ACK

Moving the MapIntoRange allows both src/blockfilter.cpp and src/test/fuzz/golomb_rice.cpp to access the function while removing unnecessary duplicated code.

The one small thing that bugs me is using static keywords with the utility function. To be honest, I could not clearly understand the reasoning behind using (or not using) the static keyword with a utility function. Still, I had not seen many src/util functions with a static keyword in its definition.

P.s.: I tried to comprehend the reasoning of static keyword, but I could not find a good reference for it. And I considered it is better to ask rather than to give an uninformed ACK.

I think using static in a header file is a way to embed the compiled function into every object file that uses it (as opposed to putting it into one object only and then linking that)?

embed the compiled function into every object file that uses it

So wouldn't it increase the memory used at runtime by the application? Is the speed boost provided by using static worth increasing memory usage?

Using static here means that indeed, every compilation unit gets its own copy of the function. But, on the other hand, it means it doesn't need to be emitted either if all call-sites can be inlined (because static functions are only accessible from within their own compilation unit; if there are no explicit calls left, the function doesn't need to be emitted), which is almost certainly the case. So the practical effect is really that it appears 0 times in the overall compilation output rather than 1 time.

I guess changing static to inline here wouldn't change the binary, but clarify the intent a bit more?

It could change the binary. Marking it inline but not static means the compiler still has to emit it. -ffunction-sections and -Wl,--gc-sections will let the linker remove it, though.

I think it should be both inline and static.

I think it should be both inline and static.

As far as I understand the usage of static and inline, I think it's best to go with this solution.

I think it should be both inline and static.

Updated to be static inline.

See also the feedback here: #23760 (comment)

See also the feedback here: #23760 (comment)

Going to leave that for now. Happy for someone else to follow up.

ACK df2307cdc3d08233d17beb9a50c144baaef1f44e

Changes since my last review:

• static -> static inline

The rationale for the change is discussed above, with which I agree.

Going to leave that for now. Happy for someone else to follow up.

#23760 (comment) seems interesting. Let me try working upon it, and give an update as soon as possible.

Going to leave that for now. Happy for someone else to follow up.

See #23994.