Is there any point in passing cfeerate by const reference vs just const

Agree. A CFeeRate is basically a wrapper around a CAmount, so 8 bytes. Seems for constant use it's better to pass it by value, instead of adding a level of indirection by reference.

Yes a CAmount is 8 bytes, and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in suggests a threshold of 2-3 words, e.g. 2 * sizeof(void*).

So, I wondered why elsewhere than this patch CFeeRate params are passed by reference to const. For instance, in feerate.h, fees.{h,cpp}, policy.{h,cpp} andtxmempool.{h,cpp} (granted, it could all be an oversight).

I found more in-depth discussion in Scott Meyers' "Effective C++, Third Edition" (2005), Item 20, pages 86-94, and "Effective Modern C++" (2015), Item 41, pages 281-292. Maybe it is somewhat dated. Apologies if it is obvious info. Some excerpts:

• Avoid passing objects of user-defined types by value.

• One might conclude that all small types are good candidates for pass-by-value,
  even if they're user-defined. However, just because an object is small doesn't
  mean that calling its copy constructor is inexpensive. Many objects--most STL
  containers among them--contain little more than a pointer, but copying such
  objects entails copying everything they point to. That can be very expensive.

• Even when small objects have inexpensive copy constructors, there can be
  performance issues. Some compilers treat built-in and user-defined types
  differently, even if they have the same underlying representation. When that
  type of thing happens, you can be better off passing such objects by
  reference, because compilers will certainly put pointers (the implementation
  of references) into registers.

• Another reason why small user-defined types are not necessarily good
  pass-by-value candidates is that, being user-defined, their size is subject to
  change. A type that's small now may be bigger in the future if its internal
  implementation changes. Things can even change when switching to a different
  C++ implementation.

• In general, the only types for which you can reasonably assume that
  pass-by-value is inexpensive are built-in types and STL iterator and function
  object types. For everything else, prefer pass-by-reference-to-const over
  pass-by-value.

• Exception: possibly consider pass-by-value of user-defined types for copyable
  parameters that are cheap to move, if they are always copied anyway in their
  implementation.

• When there are chains of function calls, each of which passes-by-value, the
  cost for the entire chain of calls accumulates and may not be something you can
  tolerate. With by-reference passing, chains of calls don't incur this kind of
  accumulated overhead.

• The most practical approach may be to adopt a "guilty until proven innocent"
  policy for pass-by-value of user-defined types.

It looks like there are 18 lines of CFeeRate params passed by reference to const, and 7 in this patch passed by value. If the 18 should be changed rather than these 7, it would remain a reasonably small change.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23475 (wallet: add config to prioritize a solution that doesn't create change in coin selection by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Yes I think the main thing to consider is maintainability: if CFeeRate might grow larger at some point in the future, we do not want to change all of these again. FWIW, the story behind CAmount historically being passed by-reference is that it makes it easier to share code with coins that have non-POD amounts (at the time, Freicoin). I don't think we actually care about this, but for the info.

Anyhow:

• I think the chance of more fields being added to CFeeRate is relatively small
• however, CFeeRate is never passed to really performance critical functions where copying a few more bytes would really make a difference. Nor the extra level of indirection, for that matter.

So to be honest I don't feel strongly either way. It would be somewhat nice to be consistent but that's all.

i don't particularly care either just figured it might be better to pass by const value.

Yes, ISTM the advantages of this change are small amounts (pun intended) of:

• maintainability (no need to change further as CFeeRate evolves)
• clarity for developers reading the code and this discussion
• and perhaps even performance...

With by-reference passing, chains of calls don't incur this kind of accumulated overhead.

...as the CFeeRate comparison operators in feerate.h are reference to const, when feerates are passed into functions to be compared by invoking one of those operators, like in policy/rbf.{h,cpp} in this diff, performance might be not worse and indeed (very slightly) better if the call chain remains reference to const rather than introducing a needless copy in the middle. At least, performance is a wash or shouldn't be an issue.

I tried writing some benchmarks as a sanity check but I didn't see anything conclusive (the difference may be too small to see and my machine isn't very stable for benchmarking anyway): https://github.com/jonatack/bitcoin/commits/bench-passing-CAmount-and-CFeeRate

FWIW, the story behind CAmount historically being passed by-reference is that it makes it easier to share code with coins that have non-POD amounts (at the time, Freicoin).

Thanks for the context! Been wondering for some time why that was.

Rebased. git range-diff e7b6272 2e4a6cd 4f9c44b

Rebased per git range-diff 26296eb 4f9c44b 3c7a3c1.

Rebased, 2 previous ACKs by @PastaPastaPasta and @w0xlt (thanks!)

Closing as this simple change has been open with ACKs for almost a year without merge. If it was desired, it would have been merged.