univalue: make spaceStr thread-safe #4851

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2014_09_univalue_threadsafe changing 1 files +6 −12

laanwj commented at 11:12 AM on September 5, 2014: member

Simply add spaces to the existing string instead of using a temporary.

Fixes #4756.

Tested with src/bitcoin-tx -json -create.
in src/univalue/univalue_write.cpp:None in 92a75339e2 outdated
```
  74 | -
  75 | -static string indentStr(unsigned int prettyIndent, unsigned int indentLevel)
  76 | +static void indentStr(unsigned int prettyIndent, unsigned int indentLevel, string& s)
  77 |  {
  78 |      unsigned int spaces = prettyIndent * indentLevel;
  79 | -    while (spaceStr.size() < spaces)
```
Diapolo commented at 11:58 AM on September 5, 2014:

This seems to be more than just using a copy?

laanwj commented at 12:01 PM on September 5, 2014:

I don't get where this idea comes from. Why would copying be cheaper than just generating spaces on the fly? Memory access is much slower than (trivial) computation. And what do you think a copy does internally? (not to say the temporary generated by substr)

Diapolo commented at 12:07 PM on September 5, 2014:

Sorry wrong wording perhaps, let me try again. You are now using a reference, but what is done in this function now looks a little different. See the long space line in the diff and now just s += ' '?

laanwj commented at 12:35 PM on September 5, 2014:

Oh I see what you mean now. But it seems correct: the old loop adds spaces in units of 16 and then does a substring, but there is no point in doing that in the new case which only adds as many spaces as requested.
laanwj commented at 12:38 PM on September 5, 2014:

Looking at the std::string API there seems to be an even better way:

string.append(spaces, ' ');

Gotta test this :)
Diapolo commented at 12:37 PM on September 5, 2014: none

ut ACK

univalue: make spaceStr thread-safe

Simply add spaces to the existing string instead of using a
temporary.

Fixes #4756.

41ef558aa9

laanwj force-pushed on Sep 5, 2014
BitcoinPullTester commented at 1:00 PM on September 5, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4851_41ef558aa907c50a055c44c4e6abaf813b23aeff/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa commented at 8:23 PM on September 5, 2014: member

Untested ACK
jgarzik commented at 8:39 PM on September 5, 2014: contributor

ut ACK -- nice improvement
sipa merged this on Sep 6, 2014
sipa closed this on Sep 6, 2014
sipa referenced this in commit 93193c8ffd on Sep 6, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4756 spaceStr in univalue_write.cpp is not thread-safe