Page MenuHomePhabricator

UniValue performance speedups for .write()
ClosedPublic

Authored by jasonbcox on May 6 2020, 17:37.

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix requested changes to this revision.May 6 2020, 18:27
deadalnix added a subscriber: deadalnix.

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 ↗(On Diff #19754)

The inline qualifiers have 0 effect here.

92 ↗(On Diff #19754)

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 edited the test plan for this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.May 6 2020, 23:12

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
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.

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 .

This revision is now accepted and ready to land.May 7 2020, 14:33
  • UniValueStream -> UniValueStreamWriter
  • Use getter instead of direct access to str