Skip to content

univalue: make spaceStr thread-safe#4851

Merged
sipa merged 1 commit intobitcoin:masterfrom
laanwj:2014_09_univalue_threadsafe
Sep 6, 2014
Merged

univalue: make spaceStr thread-safe#4851
sipa merged 1 commit intobitcoin:masterfrom
laanwj:2014_09_univalue_threadsafe

Conversation

@laanwj
Copy link
Copy Markdown
Member

@laanwj laanwj commented Sep 5, 2014

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

Fixes #4756.

Tested with src/bitcoin-tx -json -create.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be more than just using a copy?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 += ' '?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

string.append(spaces, ' ');

Gotta test this :)

@Diapolo
Copy link
Copy Markdown

Diapolo commented Sep 5, 2014

ut ACK

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

Fixes bitcoin#4756.
@laanwj laanwj force-pushed the 2014_09_univalue_threadsafe branch from 92a7533 to 41ef558 Compare September 5, 2014 12:42
@BitcoinPullTester
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member

sipa commented Sep 5, 2014

Untested ACK

@jgarzik
Copy link
Copy Markdown
Contributor

jgarzik commented Sep 5, 2014

ut ACK -- nice improvement

@sipa sipa merged commit 41ef558 into bitcoin:master Sep 6, 2014
sipa added a commit that referenced this pull request Sep 6, 2014
41ef558 univalue: make spaceStr thread-safe (Wladimir J. van der Laan)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spaceStr in univalue_write.cpp is not thread-safe

5 participants