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

Optionally construct Post Processing Info map in MemTableInserter #2011

Closed
wants to merge 1 commit into from

Conversation

yuslepukhin
Copy link
Contributor

@yuslepukhin yuslepukhin commented Mar 20, 2017

MemTableInserter default constructs Post processing info
std::map. However, on Windows with 2015 STL the default
constructed map still dynamically allocates one node
which shows up on a profiler and we loose ~40% throughput
on fillrandom benchmark.
Solution: declare a map as std::aligned storage and optionally
construct.

This addresses #1976

Before:

Initializing RocksDB Options from command-line flags
DB path: [k:\data\BulkLoadRandom_10M_fillonly]
fillrandom : 2.775 micros/op 360334 ops/sec; 280.4 MB/s
Microseconds per write:
Count: 10000000 Average: 2.7749 StdDev: 39.92
Min: 1 Median: 2.0826 Max: 26051
Percentiles: P50: 2.08 P75: 2.55 P99: 3.55 P99.9: 9.58 P99.99: 51.5**6

After:

Initializing RocksDB Options from command-line flags
DB path: [k:\data\BulkLoadRandom_10M_fillonly]
fillrandom : 1.794 micros/op 557284 ops/sec; 433.7 MB/s
Microseconds per write:
Count: 10000000 Average: 1.7940 StdDev: 45.46
Min: 0 Median: 1.0812 Max: 23620
Percentiles: P50: 1.08 P75: 1.57 P99: 2.81 P99.9: 5.39 P99.99: 17.36

@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@IslamAbdelRahman
Copy link
Contributor

Thanks!
will using std::unique_ptrstd::map have the same effect ?

@siying
Copy link
Contributor

siying commented Mar 21, 2017

@IslamAbdelRahman unique_ptr is a pointer, so the map itself needs an extra allocation.

@yuslepukhin
Copy link
Contributor Author

@IslamAbdelRahman @siying is correct. We'd need an extra allocation for map itself. Preserving space reserved for map inside the inserter gives us the same cost-less stack allocation.

@yuslepukhin
Copy link
Contributor Author

@siying I do not know what's wrong with Travis

@siying
Copy link
Contributor

siying commented Mar 21, 2017

@yuslepukhin what we can do?

@siying
Copy link
Contributor

siying commented Mar 21, 2017

@yuslepukhin failing for our internal CI too:

db/write_batch.cc:789:17: error: expected type-specifier
   PostMapType = std::aligned_storage_t<sizeof(MemPostInfoMap)>;
                 ^
db/write_batch.cc:790:3: error: ‘PostMapType’ does not name a type
   PostMapType mem_post_info_map_;
   ^
db/write_batch.cc: In member function ‘rocksdb::MemTableInserter::MemPostInfoMap& rocksdb::MemTableInserter::GetPostMap()’:
db/write_batch.cc:797:13: error: ‘mem_post_info_map_’ was not declared in this scope
       new (&mem_post_info_map_) MemPostInfoMap();
             ^
db/write_batch.cc:800:48: error: ‘mem_post_info_map_’ was not declared in this scope
     return *reinterpret_cast<MemPostInfoMap*>(&mem_post_info_map_);
                                                ^
db/write_batch.cc: In destructor ‘virtual rocksdb::MemTableInserter::~MemTableInserter()’:
db/write_batch.cc:828:11: error: ‘mem_post_info_map_’ was not declared in this scope
         (&mem_post_info_map_)->~MemPostInfoMap();
           ^

@yuslepukhin
Copy link
Contributor Author

@siying I have included <type_traits> for std::std::aligned_storage_t,
It looks like std::std::aligned_storage_t is not defined, in which case we will need to replace it with std::std::aligned_storage<sizeof(MemPostInfoMap)>::type

Will post an update

  std::map. However, on Windows with 2015 STL the default
  constructed map still dynamically allocates one node
  which shows up on a profiler and we loose ~40% throughput
  on fillrandom benchmark.
  Solution: declare a map as std::aligned storage and optionally
  construct. Results on the same commit:
  Before:
  ************************ BulkLoadRandom_10M_fillonly ************************
  Initializing RocksDB Options from command-line flags
  DB path: [k:\data\BulkLoadRandom_10M_fillonly]
  fillrandom   :       2.775 micros/op 360334 ops/sec;  280.4 MB/s
  Microseconds per write:
  Count: 10000000 Average: 2.7749  StdDev: 39.92
  Min: 1  Median: 2.0826  Max: 26051
  Percentiles: P50: 2.08 P75: 2.55 P99: 3.55 P99.9: 9.58 P99.99: 51.56
  ------------------------------------------------------
  After:
  ************************ BulkLoadRandom_10M_fillonly ************************
  Initializing RocksDB Options from command-line flags
  DB path: [k:\data\BulkLoadRandom_10M_fillonly]
  fillrandom   :       1.794 micros/op 557284 ops/sec;  433.7 MB/s
  Microseconds per write:
  Count: 10000000 Average: 1.7940  StdDev: 45.46
  Min: 0  Median: 1.0812  Max: 23620
  Percentiles: P50: 1.08 P75: 1.57 P99: 2.81 P99.9: 5.39 P99.99: 17.36
  ------------------------------------------------------
@facebook-github-bot
Copy link
Contributor

@yuslepukhin updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yuslepukhin yuslepukhin deleted the optional_map_ctor branch March 22, 2017 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants