Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Addded caching value in iter store to avoid extra retrievals from RocksDB #9997

Closed
wants to merge 2 commits into from

Conversation

brianjohnson5972
Copy link
Contributor

@brianjohnson5972 brianjohnson5972 commented Feb 3, 2021

Change Description

Added caching value in the iter store and avoiding accessing RocksDB and removed unneeded RocksDB iterators from the typical use case path. This is only an optimization.

Change Type

Select ONE:

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Testing Changes

Select ANY that apply:

  • New Tests
  • Existing Tests
  • Test Framework
  • CI System
  • Other

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@@ -252,6 +254,7 @@ namespace eosio { namespace chain { namespace backing_store {
table_store.table, old_payer, payer, name(key_store.primary),
old_key_value.value->data(), old_key_value.value->size(), value, value_size);
}
primary_iter_store.set_value(itr, value, value_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be primary_iter_store.set_value(iterator, pp.value, pp.value_size); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, value and pp.value and value_size and pp.value_size are just copies of each other. The payer_payload makes a copy and then when you call pp.to_payload() it returns a packed buffer with both the payer and value data in it. But since we have value and value_size passed in, we can just use it.

@@ -288,19 +291,12 @@ namespace eosio { namespace chain { namespace backing_store {

int32_t db_context_rocksdb::db_get_i64(int32_t itr, char* value , size_t value_size) {
const auto& key_store = primary_iter_store.get(itr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that itr is not cached in the iter_store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only if other code was written incorrectly. the itr is a "handle" to the code/scope/table/primary_key that would have been stored and returned by a call to db_find_i64 (or db_lower/upper_bound, db_next/previous_i64, etc)

@brianjohnson5972
Copy link
Contributor Author

This failed for replaying blocks, and was not able to identify the bug and after analysis, it was determined that this would provide little improvement since in every scenario that the cache would have data, the value would be read directly from the most recent session's cache, so closing.

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.

2 participants