Skip to content

Use boost::unordered_map for mapBlockIndex#4838

Merged
sipa merged 4 commits intobitcoin:masterfrom
sipa:fastmap
Sep 4, 2014
Merged

Use boost::unordered_map for mapBlockIndex#4838
sipa merged 4 commits intobitcoin:masterfrom
sipa:fastmap

Conversation

@sipa
Copy link
Member

@sipa sipa commented Sep 4, 2014

As some profiling by @kdomanski showed that we're doing a very high amount of calls to uint256::CompareTo, in particular for mapBlockIndex operations. So:

  • Turn mapBlockIndex into an unordered_map, so it only needs (few) operator== calls on uint256 instead of (multiple) operator< calls.
  • Using memcmp for uint256's operator== and operator!=.

@BitcoinPullTester
Copy link

BitcoinPullTester commented Sep 4, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4838_1e4f87f5a13e34a457b537e9d13a212e6c5b754f/ 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.

@laanwj
Copy link
Member

laanwj commented Sep 4, 2014

Looks good to me, utACK

laanwj
laanwj reviewed Sep 4, 2014
View reviewed changes
src/checkpoints.cpp
Copy link
Member

@laanwj laanwj Sep 4, 2014

Choose a reason for hiding this comment

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

No strong feeling on this, but I slightly prefer passing this explicitly as it documents what input is used to the function.

Copy link
Member Author

@sipa sipa Sep 4, 2014

Choose a reason for hiding this comment

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

That's not possible. Checkpoints and main are a circular dependency, only possible now because checkpoints.h doesn't need main.h. If the type of mapBlockIndex is defined in main.h, it becomes an actual .h circular dependency.

Better solution: move the code pieces of checkpoints to main, and the data pieces to chainparams...

Copy link
Member

@laanwj laanwj Sep 4, 2014

Choose a reason for hiding this comment

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

OK, clear.
#3802 does that data move, by the way.

Copy link
Member Author

@sipa sipa Sep 4, 2014

Choose a reason for hiding this comment

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

It moves the data but does not break the cyclic dependency.

@gmaxwell
Copy link
Contributor

gmaxwell commented Sep 4, 2014

I tested out most of the same change last weekend in the hope it would decrease memory usage (it didn't). I liked the effect on the code (esp, getting rid of the explicit types for a typedef, is 99% of the work here and should be done regardless).

ACK (with quasi-testing, I tested my nearly identical changes)

@jgarzik
Copy link
Contributor

jgarzik commented Sep 4, 2014

ut ACK

@sipa sipa merged commit 1e4f87f into bitcoin:master Sep 4, 2014
sipa added a commit that referenced this pull request Sep 4, 2014
@sipa
Merge pull request #4838
af9c3b0 
1e4f87f Use memcmp for uint256 equality/inequality (Pieter Wuille)
8a41e1e Use boost::unordered_map for mapBlockIndex (Pieter Wuille)
145d5be Introduce BlockMap type for mapBlockIndex (Pieter Wuille)
a0dbe43 checkpoints.cpp depends on main, it can use mapBlockIndex directly (Pieter Wuille)
@sipa sipa mentioned this pull request Sep 4, 2014
Merged
@laanwj laanwj mentioned this pull request Feb 16, 2016
Closed
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sipa @BitcoinPullTester @laanwj @gmaxwell @jgarzik