various performance improvements

[martinus]

This PR contains has a few performance improvements for UniValue. Most importantly, it adds move semantics, and a way to easily transition legacy code to make use of the new methods.

I've used BlockToJsonVerbose as my benchmark. With this PR alone the benchmark runs 1.26 times faster. When actually making use of the new move semantics in bitcoin's core_write.cpp and blockchain.cpp, this speeds up the benchmark by a factor of ~2.3 on my machine.

ns/op	op/s	err%	total	benchmark
63,370,373.00	15.78	0.9%	0.70	BlockToJsonVerbose master
50,111,950.00	19.96	0.7%	0.56	BlockToJsonVerbose this PR
27,775,808.00	36.00	0.4%	0.31	BlockToJsonVerbose this PR + using move semantics in core_write.cpp and blockchain.cpp

I got no response whatsoever on move-semantic PR on jgarzik#79 from about 8 months ago, so I'm opening this PR here and close the other one.

[fanquake]

@martinus want to rebase this on the master branch, as we've added a CI, and made some other changes recently. Note that if you wanted to start using C++17 here you could do that as well (we don't explicitly require it in this repo yet, but that is easily changed).

[fanquake]

@theuni you might also be interested in this.

[theuni]

Neat. For the most part, I don't think it's necessary to have functions for const ref and rvalues. Just pass it by value. The caller can move it in and the function can always move-from.

More generally though, I think it's worth a discussion about what to do with UniValue long-term. It's certainly not modern code.

FWIW: I took a stab at rewriting UniValue's api to be backed by a single c++17 std::variant (notice it takes heap alloc'd arguments by value), but tbh I don't see that as much of an improvement. I'm wondering if it would be interesting to instead just drop UniValue altogether, and replace it with a std::variant of json types and helper parsing functions. Afterall, UniValue is just a naive and slow tagged union.

[theuni]

That said, I'll review this in detail. There's certainly some low-hanging fruit here. Thanks @martinus!

[martinus]

Hi! I unfortunately didn't have any time to work on this lately. Here are some things I still wanted to do, that's why I've marked this PR as draft:

• I don't like that there are so many push_back, pushKV methods. Maybe it's possible to get rid of all of them and have just a single templated version in the header. Care needs to be taken about automatic conversions.
• Have a close look at https://github.com/cculianu/univalue which is supposed to be a c++17 rewrite of univalue, which was posted on reddit a while ago
• Univalue's members make plain default constructor and destructor slow, because there are so many objects. It would be interesting to experiment with doing something else there

[martinus]

I've rebased to ced6a36 so CI ran, and some minor fixes. Marking as "ready for review", further cleanup like reducing number of push_back or pushKV methods can also be done later (if needed)

[maflcko]

Needs rebase 🐙

[martinus]

I accidentally rebased first to the wrong branch, so see git range-diff ced6a36...55265e8 for the actual rebase changes. Fixed a few conflicts in the includes

[maflcko]

Maybe split up the first and last commit into a separate change? This way there'd be appendNumToString and std::move separate from each other.

[maflcko]

Concept ACK. See also bitcoin/bitcoin#15974 and bitcoin/bitcoin@e3b7f10

[maflcko]

Suggested change

	* Performs locale-independend to string conversion of integral numbers.
	* Performs locale-independent to string conversion of integral numbers.

[fanquake]

@martinus note that now we've "unsubtree'd" univalue, so you could open this PR against bitcoin/bitcoin directly. At the same time you could drop WITH_UNIVALUE_COPY_OPERATIONS and directly use the new code.

[maflcko]

Agree. Also, let's archive/delete this repo.