DB intrinsic implementation in RocksDB #9529

brianjohnson5972 · 2020-09-28T06:35:02Z

Change Description

Providing the implemenation for db_context_rocksdb, allowing db intrinsic data to be stored in rocksdb.
(WIll merge in Timothy's session changes as the last step before commiting this PR)

Change Type

Select ONE

Documentation

Stability bug fix

Other

Other - special case

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

…thod outside the class definition.

… obvious it is the chainbase implementation till it can be refactored out into db_context_chainbase.

…sert.

…the size needed.

… to subtract.

libraries/chain/include/eosio/chain/backing_store/chain_kv_payer.hpp

… rocksdb_db_implementation-updated

libraries/chain/apply_context.cpp

libraries/chain/backing_store/db_context_rocksdb.cpp

TimothyBanks · 2020-09-28T15:15:10Z

libraries/chain/backing_store/db_context_rocksdb.cpp

-      int32_t db_idx_double_end(uint64_t code, uint64_t scope, uint64_t table) override {
-         return -1;
-      }
+      const int64_t billable_size = (int64_t)(value_size + db_key_value_any_lookup::overhead);


Two questions for my own information:

Do we want to use auto in cases like this?

Are C style or C++ style casts preferred? (static_cast probably in this case)

1)I think the only advantage of using auto versus the type is if you are saving yourself typing, and going back and replacing would be more typing. (i.e. I think auto is just for a developers convenience, it doesn't make it more maintainable)
2) yes

TimothyBanks · 2020-09-28T15:18:35Z

libraries/chain/backing_store/db_context_rocksdb.cpp

+
+      // copy locally, since below the key_store memory will be changed
+      const auto old_payer = key_store.payer;
+      if (payer == name{}) {


How about payer.empty()?

TimothyBanks · 2020-09-28T15:24:13Z

libraries/chain/backing_store/db_context_rocksdb.cpp

+   }
+
+   int32_t db_context_rocksdb::db_next_i64(int32_t itr, uint64_t& primary) {
+      if (itr < primary_iter_store.invalid_iterator()) return primary_iter_store.invalid_iterator(); // cannot increment past end iterator of table


Is there a way to make the logic here itr == primary_iter_store.invalid_iterator()? Would seem a better fit with how std iterators work.

Yeah, this is a confusing use of iterator. It really should be called handle. But this was written to mimic the similar chainbase logic for DB, so I think it should match. "Handles" above 0 indicate a specifically cached database value, -1 is invalid, less than -1 indicates that you have a handle to the end of the table (-2 is the table[0], -3 is the table[1], etc). So in this case it is using this to indicate that db_next_i64 for "end handles" returns an invalid handle. As you can see as I talk about it, it is kind of an amalgam of handle and iterator.

TimothyBanks · 2020-09-28T15:52:58Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+      }
+   }
+
+   void db_key_value_any_lookup::remove_table_if_empty(name scope, name table) {


Is there a version that removes the table if it is not empty? Could we shorten the name to remove_table if not?

I prefer having the longer name, rather than having a short name and longer comment.

TimothyBanks · 2020-09-28T15:57:58Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+      if (!view.get(parent.receiver.to_uint64_t(), slice_table_key)) {
+         std::string event_id;
+         apply_context& context = parent.context;
+         if (context.control.get_deep_mind_logger() != nullptr) {


Could we cache the result of get_deep_mind_logger and use that for the conditional below too?

TimothyBanks · 2020-09-28T15:59:40Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+      const name payer = payer_payload{first_kv->value.data(), first_kv->value.size()}.payer;
+      std::string event_id;
+      apply_context& context = parent.context;
+      if (context.control.get_deep_mind_logger() != nullptr) {


Maybe cache the result of get_deep_mind_logger and use that for the conditional below too?

TimothyBanks · 2020-09-28T16:18:30Z

libraries/chain/include/eosio/chain/backing_store/db_key_value_any_lookup.hpp

+
+   struct db_key_value_any_lookup {
+      db_key_value_any_lookup(db_context& c, b1::chain_kv::view& v) : parent(c), view(v) {}
+      virtual ~db_key_value_any_lookup() {}


If the destructor is empty, we can let the compiler generate it for us.

libraries/chain/include/eosio/chain/backing_store/db_key_value_any_lookup.hpp

linhuang-blockone

I looked at code at high level. I liked the refactor work, especially for payer related code.

brianjohnson5972 · 2020-10-06T16:18:11Z

@linhuang-blockone it is inside the if because we only need to construct the event_id if we are going to log it, otherwise we don't care. See the similar apply_context::dm_***_chainbase methods. This requirement comes from that code.

TimothyBanks · 2020-10-06T19:49:05Z

libraries/chain/backing_store/db_context_rocksdb.cpp

+      if( itr < primary_iter_store.invalid_iterator() ) { // is end iterator
+         const backing_store::unique_table* table_store = primary_iter_store.find_table_by_end_iterator(itr);
+         EOS_ASSERT( table_store, invalid_table_iterator, "not a valid end iterator" );
+         if (current_session.begin() == current_session.end()) {


This would be an opportunity for an optimization here, but session should just provide an empty and size method.

TimothyBanks · 2020-10-06T19:58:17Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+
+   void db_key_value_any_lookup::add_table_if_needed(const shared_bytes& key, account_name payer) {
+      auto table_key = db_key_value_format::create_full_key_prefix(key, end_of_prefix::pre_type);
+      auto session_iter = current_session.lower_bound(table_key);


lower_bound is definitely the lynchpin method in all of this

TimothyBanks · 2020-10-06T20:00:56Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+      auto entire_table_prefix_key = db_key_value_format::create_full_key_prefix(key, end_of_prefix::at_type);
+      // since this prefix key is just scope and table, it will include all primary, secondary, and table keys
+      auto session_itr = current_session.lower_bound(entire_table_prefix_key);
+      EOS_ASSERT( session_itr != current_session.end(), db_rocksdb_invalid_operation_exception,


Something that you might want to see if it makes a difference in performance is checking session_itr.key() instead. The end iterator will return an empty key and if you have an iterator that returns an empty key, it must be the end. That would bypass the needed creation of the end iterator. But I'm assuming that keys can never be empty.

well, for any of my code the key should at the very least have the db type prefix, so empty key does mean we are not looking at anything in the db intrinsics portion of the code.

TimothyBanks · 2020-10-06T20:05:20Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+   }
+
+   bool db_key_value_any_lookup::match(const shared_bytes& lhs, const shared_bytes& rhs) {
+      return lhs.size() == rhs.size() &&


shared_bytes has this operator.

TimothyBanks · 2020-10-06T20:06:11Z

libraries/chain/backing_store/db_key_value_any_lookup.cpp

+      if (iter == current_session.end()) {
+         return false;
+      }
+      return match(lhs, (*iter).first);


The iterator also has a key() method will only return the key instead of both the key and value.

TimothyBanks · 2020-10-06T20:08:13Z

libraries/chain/backing_store/db_key_value_format.cpp

+      const auto new_kt = static_cast<char>(key_type::table);
+      const char* new_kt_ptr = &new_kt;
+      // already have everything in char format/order, so just need to assemble it
+      return detail::assemble<2, eosio::session::shared_bytes>({rocksdb::Slice{prefix_key.data(), full_prefix_size},


We should get rid of the dependency of rocksdb in this file.

I was planning on cleaning out the old code that used Slices, but figured I would use the same type that it is using throughout the cpp and hpp and then clean them all up together.

TimothyBanks · 2020-10-06T20:10:16Z

libraries/chain/backing_store/db_key_value_format.cpp

+      static const char db_type_prefix = detail::get_rocksdb_contract_db_prefix_as_char();
+      b1::chain_kv::bytes code_as_bytes;
+      b1::chain_kv::append_key(code_as_bytes, code.to_uint64_t());
+      auto ret = eosio::session::make_shared_bytes<std::string_view, 3>({std::string_view{&db_type_prefix, 1},


I think I removed the make_shared_bytes function. We can add it back though since it would be much easier than converting all these over.

I first tried making it one of the constructors, but was having lots of compiler problems so I just went the easy route. I'll circle back if this shows up as a performance hotspot.

TimothyBanks · 2020-10-06T20:12:25Z

libraries/chain/include/eosio/chain/backing_store/chain_kv_payer.hpp

+      eosio::session::shared_bytes as_payload() const {
+         char payer_buf[payer_in_value_size];
+         memcpy(payer_buf, &payer, payer_in_value_size);
+         return eosio::session::make_shared_bytes<std::string_view, 2>({std::string_view{payer_buf,


That's a very nice concise of creating one of these by passing an array of string_view instances.

… rocksdb_db_implementation--last-merge libraries/chain/include/eosio/chain/backing_store/kv_context_rocksdb.hpp

…_bytes>.

brianjohnson5972 added 17 commits September 16, 2020 12:58

Added table key type for storing table payer for resource tracking.

c028de6

Moving common methods for use by db_context_rocksdb.

d5a0fc3

Initial implementation for primary keys.

ce3a9b7

Added secondary_lookup template class and moved db_context_rocksdb me…

785a4da

…thod outside the class definition.

Added secondary key lookup for db instrinsics in rocksdb.

34080c2

Fixes for rebase changes.

97ef9ae

Changed db_intrinsic in chainbase methods on apply_context to make it…

240ba03

… obvious it is the chainbase implementation till it can be refactored out into db_context_chainbase.

Changed backing_store default to CHAINBASE instead of ROCKSDB.

70bf025

Fix to make db_context last for life of apply_context and fixed by as…

c2ccac9

…sert.

Added methods needed for snapshot iteration through the whole key space.

a62e938

Fixed error with moving const bytes.

9676d1c

Added method to retrieve scope/table/key_type from composite_key.

7ffa75a

Fixed test errors and warning.

41f656d

Fixed test parameters and expected distance.

cbffd4f

Fixed db_get_i64 for use case of sending in 0 size to have it return …

c997a1a

…the size needed.

Fixed passing const to std::move and calculating the correct db usage…

7266d5a

… to subtract.

Bug fixes to get all tests passing.

a2684d1

brianjohnson5972 requested review from linhuang-blockone, larryk85 and TimothyBanks September 28, 2020 06:35

brianjohnson5972 added 2 commits September 28, 2020 07:14

Trying to fix odd macos build error.

c0a478d

Attempting to fix macos build errors.

414654c

linhuang-blockone reviewed Sep 28, 2020

View reviewed changes

libraries/chain/include/eosio/chain/backing_store/chain_kv_payer.hpp Show resolved Hide resolved

Merge remote-tracking branch 'origin/rocksdb_nodeos_integration' into…

f0487d9

… rocksdb_db_implementation-updated

TimothyBanks reviewed Sep 28, 2020

View reviewed changes

Fixes for changing from chain_kv to session.

0992634

linhuang-blockone approved these changes Oct 5, 2020

View reviewed changes

Changes for PR comments.

e143b72

TimothyBanks reviewed Oct 6, 2020

View reviewed changes

brianjohnson5972 added 2 commits October 8, 2020 08:47

Merge remote-tracking branch 'origin/rocksdb_nodeos_integration' into…

4321449

… rocksdb_db_implementation--last-merge libraries/chain/include/eosio/chain/backing_store/kv_context_rocksdb.hpp

Changes due to all values in session being changed to optional<shared…

3efcac7

…_bytes>.

TimothyBanks approved these changes Oct 8, 2020

View reviewed changes

brianjohnson5972 merged commit d82288e into rocksdb_nodeos_integration Oct 8, 2020

brianjohnson5972 deleted the rocksdb_db_implementation branch January 27, 2021 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DB intrinsic implementation in RocksDB #9529

DB intrinsic implementation in RocksDB #9529

brianjohnson5972 commented Sep 28, 2020 •

edited

Loading

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

TimothyBanks Sep 28, 2020

brianjohnson5972 Oct 6, 2020

linhuang-blockone left a comment

brianjohnson5972 commented Oct 6, 2020

TimothyBanks Oct 6, 2020

TimothyBanks Oct 6, 2020

TimothyBanks Oct 6, 2020

brianjohnson5972 Oct 8, 2020

TimothyBanks Oct 6, 2020

TimothyBanks Oct 6, 2020

TimothyBanks Oct 6, 2020

brianjohnson5972 Oct 8, 2020

TimothyBanks Oct 6, 2020

brianjohnson5972 Oct 8, 2020

TimothyBanks Oct 6, 2020

DB intrinsic implementation in RocksDB #9529

DB intrinsic implementation in RocksDB #9529

Conversation

brianjohnson5972 commented Sep 28, 2020 • edited Loading

Change Description

Change Type

Consensus Changes

API Changes

Documentation Additions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linhuang-blockone left a comment

Choose a reason for hiding this comment

brianjohnson5972 commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjohnson5972 commented Sep 28, 2020 •

edited

Loading