Various serialization simplifcations and optimizations

[sipa]

The commits in this pull request implement a sequence of changes:

• Simplifications:
  • Remove unused ReadVersion and WriteVersion CDataStream and CAutoFile had a ReadVersion and WriteVersion method that was never used. Remove them.
  • Make nType and nVersion private and sometimes const Make the various stream implementations' nType and nVersion private and const (except in CDataStream where we really need a setter).
  • Make streams' read and write return void The stream implementations have two layers (the upper one with operator<< and operator>>, and a lower one with read and write). The lower layer's return values are never used (nor should they, as they should only be used from the higher layer), so make them void.
  • Make GetSerializeSize a wrapper on top of CSizeComputer Given that in default GetSerializeSize implementations we're already using CSizeComputer(), get rid of the specialized GetSerializeSize methods everywhere, and just use CSizeComputer. This removes a lot of code which isn't actually used anywhere. In a few places, this removes an actually more efficient size computing algorithm, which we'll bring back in the "Add optimized CSizeComputer serializers" commit later.
  • Get rid of nType and nVersion The big change: remove the nType and nVersion as parameters to all serialization methods and functions. There is only one place where it's read and has an impact (in CAddress), and even there it does not impact any of the member objects' serializations. Instead, the few places that need nType or nVersion read it directly from the stream, through GetType and GetVersion calls which are added to all streams.
  • Avoid -Wshadow errors As suggested by @paveljanik, remove the few remaining cases of variable shadowing in the serialization code.
• Optimizations:
  • Make CSerAction's ForRead() constexpr The CSerAction's ForRead() method does not depend on any runtime data, so guarantee that requests to it can be optimized out by making it constexpr (suggested by @theuni in Make CTransaction actually immutable #8580).
  • Add optimized CSizeComputer serializers To get the advantages of faster GetSerializeSize implementations back, reintroduce them in the few places where they actually make a difference, in the form of a specialized Serialize implementation. This actually gets us in a better state than before, as these even get used when they're nested inside the serialization of another object.
  • Use fixed preallocation instead of costly GetSerializeSize dbwrapper uses GetSerializeSize to compute the size of the buffer to preallocate. For some cases (specifically: CCoins) this requires a costly compression call. Avoid this by just using fixed size preallocations instead.

This will make it easier to address @TheBlueMatt's comments in #8580, resulting is a simpler and more efficient way to simultaneously deserialize+construct objects with const members from streams.

I assume this conflicts/supersedes #8468.

[sipa]

A benchmark of a -reindex-chainstate until height 290000, with default dbcache, shows that this is around 1% faster (4 runs on master with times ranging 1370.99-1377.95s, 4 runs on this PR with times ranging 1359.90-1365.74s; measured using -bench=debug).

[paveljanik]

Can you please also change this into

LimitedString(std::string& _string) : string(_string) {}

This will remove 204 -Wshadow warnings...

[paveljanik]

_nSize please.

[paveljanik]

ditto, _nSize please.

[paveljanik]

Concept ACK. I like it!

[sipa]

Addressed @paveljanik's nits.

[paveljanik]

Compiling with clang brings new warnings (diff master->#9039):

+In file included from init.cpp:41:
+./wallet/wallet.h:137:13: warning: 'static' function 'ReadOrderPos' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
+static void ReadOrderPos(int64_t& nOrderPos, mapValue_t& mapValue)
             ^
+./wallet/wallet.h:148:13: warning: 'static' function 'WriteOrderPos' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
+static void WriteOrderPos(const int64_t& nOrderPos, mapValue_t& mapValue)
             ^

The overall efect on build logs is brutal here (-Wshadow turned on by default):

-rw-r--r--  1 pavel  wheel   536727 Oct 30 22:20 9039.log
-rw-r--r--  1 pavel  wheel  2873995 Oct 30 22:18 master.log

Great work @sipa! Thanks.

utACK 50473bb

I will continue testing tomorrow.

[sipa]

@paveljanik That seems completely unrelated to this PR. Is it possible that clang only shows a limited number of warnings, and only gets to those new ones after the current ones fixed by this PR are fixed?

[paveljanik]

@sipa Well, maybe. I do not know yet 8)

[paveljanik]

Hmm, compiling e.g. src/wallet/wallet.cpp prints NO warning in the current master. But this is printed after applying this PR:

In file included from wallet/wallet.cpp:6:
./wallet/wallet.h:137:13: warning: 'static' function 'ReadOrderPos' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
static void ReadOrderPos(int64_t& nOrderPos, mapValue_t& mapValue)
            ^
./wallet/wallet.h:148:13: warning: 'static' function 'WriteOrderPos' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
static void WriteOrderPos(const int64_t& nOrderPos, mapValue_t& mapValue)
            ^
2 warnings generated.

The command line used is the same, ccache removed.

[theuni]

Concept ACK and quick review utACK. I can confirm @paveljanik's new warning, but can't figure out why. Inlining shuts it up.

[sipa]

@theuni @paveljanik I can't explain the new warning, but it is correct. I'll add an (technically unrelated) commit here to fix it?

[paveljanik]

Yes, please.

[maflcko]

I'll add an (technically unrelated) commit here to fix it?

If it is unrelated, better move it to a new pull request.

(Could not see the warning here, when rebasing on current master and typing make)

[sipa]

@MarcoFalke @paveljanik See #9071.

[TheBlueMatt]

Your commit message on "Make streams' read and write return void" has the description of "Make nType and nVersion private and sometimes const" and vice versa.