Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING4812274977f5: UniValue performance speedups for .write()
rABC4812274977f5: UniValue performance speedups for .write()

Summary

Inspired by https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/commit/472118a0da52c480f18430cc97ea31950d48324e
with a number of fixups that:

Improve internal linkage.
Reduce code ownership where unnecessary ownership was taken.
Keeps API reasonably consistent with original UniValue code.
Various formatting, comments, and style fixes.

Depends on D5981 for the test plan

Test Plan

ninja check check-functional

Run this before and after patch:

./src/bench/bitcoin-bench -filter=BlockToJsonVerbose

Observe ~3-4% speedup.

Verify internal linkage:

cmake -GNinja -DCMAKE_CXX_FLAGS="-emit-llvm -S" ..
ninja univalue
grep "define internal" src/univalue/libunivalue.a

I was only able to verify internal linkage for writeAny() and escapeJson(). Not the other UniValueStreamWriter functions.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

univalue-write

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 10617
Build 19043: Default Diff Build & Tests
Build 19042: arc lint + arc unit

Event Timeline

jasonbcox created this revision.May 6 2020, 17:37

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 6 2020, 17:37

jasonbcox requested review of this revision.May 6 2020, 17:37

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10617: Diff 19754.May 6 2020, 17:48

This whle inline business is wrong What you actually want is for all this stuff to have internal linkage. The way you achieve this is by Putting the class stream in an anonymous namespace and predeclaring it and making Univalue friend with it.

See https://godbolt.org/z/cxesKS for reference. You see that _ZN12_GLOBAL__N_13Foo3barERK3Bar has internal linkage, and if you enable optimization, the compiler will simply remove it altogether - as it only has one callsite. You can also remove the anonymous namepsace and play with the inline qualifier to see that it changes nothing (both have linkonce_odr as linkage).

You will not be able to change the member functions of Univalue such as writeStream & al as internal so you probably should flip the thing on its head and make them member functions of Stream and pass the Univalue as an explicit parameter.

src/univalue/lib/univalue_write.cpp
30	The inline qualifiers have 0 effect here.
92	dito, inline is not useful.

This revision now requires changes to proceed.May 6 2020, 18:27

Remove useless inlines
Move helper functions to Stream (now called UniValueStream)
Fix some internal linkage

jasonbcox planned changes to this revision.May 6 2020, 22:40

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

jasonbcox edited the summary of this revision. (Show Details)May 6 2020, 22:43

jasonbcox edited the test plan for this revision. (Show Details)

jasonbcox requested review of this revision.May 6 2020, 22:46

jasonbcox edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B10624: Diff 19769.May 6 2020, 22:50

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

That looks much better, but there are still a few things that do not make sense.

src/univalue/lib/univalue_write.cpp
13 ↗	(On Diff #19769)	You'll need to verify, but you can probably make this a value and return via move semantic.
34 ↗	(On Diff #19769)	This name does not make sense.
52 ↗	(On Diff #19769)	You need to define all of these in the anonymous namepsace as well or they won't have internal linkage. It would also be good that they are defined in the same order as they are declared so one doesn't have to play where is waldo in the file.
58 ↗	(On Diff #19769)	It seems like using move semantic would be better here.
74 ↗	(On Diff #19769)	I'm doubtful that this is actually the case when optimizations are on considering they both end up calling string.append .
104 ↗	(On Diff #19769)	Just define this kind of stuff inline in the class directly. You may want to do so in another patch if you want to make old and ne implementation easier to compare.

This revision now requires changes to proceed.May 6 2020, 23:12

Fix anonymous namespace
Use move semantic to simplify UniValueStream init API.
Fix various names
Remove operator<< functions as they apparently do not serve a purpose for this class.
Various cleanup

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

jasonbcox added inline comments.May 7 2020, 01:07

src/univalue/lib/univalue_write.cpp
104 ↗	(On Diff #19769)	After a few rounds of cleanup, this patch doesn't match the original code very well in some places. I went ahead and moved it so as not to generate more work for myself later.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10626: Diff 19775.May 7 2020, 01:16

deadalnix accepted this revision.May 7 2020, 14:33

deadalnix added inline comments.

src/univalue/include/univalue.h
20 ↗	(On Diff #19775)	UniValueStreamWriter ?
src/univalue/lib/univalue_write.cpp
149 ↗	(On Diff #19775)	A getString function or alike would be preferable so the way the string is generated becomes the sole business of UniValueStream .