Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace std::map with std::unordered_map. #679

Merged
merged 2 commits into from Nov 18, 2017

Conversation

denravonska
Copy link
Member

@denravonska denravonska commented Oct 6, 2017

This pulls the std::map -> std::unordered_map change from Bitcoin. The benefit is that unordered map is almost always faster in all aspects, plus it makes the code a bit more readable.

CPU usage

The CPU usage is drastically reduced. I did some profiling when syncing to see how much time was spent comparing uint256 when locating blocks in the maps while syncing on a Raspberry Pi:

  %   cumulative   self              self     total           
 time   seconds   seconds    calls  Ks/call  Ks/call
 46.28    749.82   749.82 702389359     0.00     0.00  map
 13.43    332.04   332.04 2917441538    0.00     0.00  unordered_map

As we can see, even though the std::map did fewer iterations due to shorter sync time and thus having its results skewed by scrypt operations, it still consumed far more CPU cycles than the unordered_map version.

Memory usage

There is a slight memory usage increase with unordered_map:

                    VIRT   RES   SHR
std::map            2076M  974M  398M
std::unordered_map: 1978M  990M  421M

I think this can be optimized away by improving CBlockIndex.

Remaining

  • Sync from 0 (ongoing)
  • Ensure that there are no collisions due to only using 64bit keys
  • Check increase memory consumption
  • Run CPU usage tests once more. unordered_map might have used asm optimized scrypt

@denravonska denravonska force-pushed the unordered_map branch 2 times, most recently from 1d8ebd5 to 093b2db Compare October 9, 2017 12:41
@tomasbrod
Copy link
Member

That 64 bit hash requires a test or research. Is it just a hash for hash table implementation and then the hash table handles collisions by comparing full key or is the 64 bit key directly used as key to the map?

@denravonska
Copy link
Member Author

denravonska commented Oct 12, 2017

Seems to be the former:

// Key
Type of the key values. Each element in an unordered_map is uniquely identified by its key value.
Aliased as member type unordered_map::key_type.

// Hash
The unordered_map object uses the hash values returned by this function to organize its elements internally, speeding up the process of locating individual elements.

http://www.cplusplus.com/reference/unordered_map/unordered_map/

I will verify this just to be sure.

@iFoggz
Copy link
Member

iFoggz commented Oct 14, 2017

look forward to testing this one. good work.

@denravonska
Copy link
Member Author

Test program with same hash for different keys:

#include <iostream>
#include <unordered_map>
#include <string>

struct hasher {
   size_t operator()(const std::string& key) const { return 0; }
};

int main(int argc, char** argv) {
   std::unordered_map<std::string, int, hasher> map;
   map["first"]  = 1;
   map["second"] = 2;
   std::cout << "First="    << map["first"] <<
                ", second=" << map["second"] <<
                ", size=" << map.size() << std::endl;
                
   return 0;
}

$ ./apa 
First=1, second=2, size=2

@denravonska
Copy link
Member Author

Updated with some benchmarks. If everyone is ok with a little more RAM usage I think we should include this in the version after the current mandatory.

@denravonska denravonska added this to the Autotools milestone Nov 7, 2017
@denravonska denravonska merged commit 901ea7c into gridcoin-community:development Nov 18, 2017
denravonska added a commit that referenced this pull request May 25, 2018
Fixed
 - Fixes for displaying on high DPI displays, #517 (@skcin).
 - Re-enable unit tests, add unit test to Travis, #769, #808 (@TheCharlatan).
 - Fix empty string in sendalert2 (@tomasbrod).

Added
 - Neural Report RPC command, #1063 (@tomasbrod).
 - GUI wallet redign with new icons and purple native style (@skcin).

Changed
 - Switch to autotools and Depends from Bitcoin, #487 (@TheCharlatan).
 - Clean and update docs for new build system, remove outdated, #828 (@TheCharlatan).
 - Change estimated time to stake calculations to be more accurate, #1084 (@jamescowens).
 - Move logging to tinyformat, #1009 (@TheCharlatan).
 - Improve appcache performance, #734 (@denravonska).
 - Improve block index memory access performance, #679 (@denravonska).
 - NN fixes: clean logging, explain mag single response, move contract to ndata_nresp (@denravonska)
 - Updated translations:
    - Turkish, #771 (@confuest).
    - Chinese, #1012 (@linnaea).
 - RPC refactor: Cleaner locks, better error handling, move execute calls to straght rpc calls, #1024 (@Foggyx420).
 - Change locking primitives from Boost to STL, #1029 (@Foggyx420).

Removed
 - gridcoindiagnostic RPC call (@denravonska).
 - Galaza, #945 (@barton2526).
 - Assertion in SignSignature, #998 (@TheCharlatan).
 - Upgrade menu, #1094 (@jamescowens).
 - Acid test functions, #871 (@tomasbrod).
 - Qt4 support, #801 (@denravonska).
div72 pushed a commit to div72/Gridcoin-Research that referenced this pull request Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants