Session optimizations #9600
Session optimizations #9600
Conversation
| auto key = eosio::session::shared_bytes{sizeof(uint64_t) + prefix_key.size() + move_to_rocks->kv_key.size()}; | ||
| std::copy(std::begin(prefix_key), std::end(prefix_key), std::begin(key)); | ||
| b1::chain_kv::insert_key(key, prefix_key.size(), move_to_rocks->contract.to_uint64_t()); | ||
| std::copy(std::begin(move_to_rocks->kv_key), std::end(move_to_rocks->kv_key), std::begin(key) + prefix_key.size() + sizeof(uint64_t)); |
There was a problem hiding this comment.
So how is it safe for eosio::session::shared_bytes to share a key buffer that can be updated? I thought the point was that it was created and was constant from then on.
Also, is this faster than using the make_shared_bytes method?
There was a problem hiding this comment.
That's how I originally intended it to be but it seemed like there was a lot of temporary buffers being created when I could just allow the user to write directly into the shared_bytes.
It depends. If you already have a buffer that you are wanting to copy from then you'd want to instantiate the shared_bytes with that buffer. If you have to construct a buffer, then it will be a bit more efficient to just create the shared_bytes instance with a length and construct it in place.
There was a problem hiding this comment.
Does the session return the same shared_bytes keys and values that it holds internally?
There was a problem hiding this comment.
Discussion about this was on slack.
There was a problem hiding this comment.
As this isn't on the critical path for the immediate future, it will be a while before this PR is reviewed.
This also introduces breaking changes to the API that some of the other engineers are actively developing against, so I suggest that this gets put on the back burner until all issues with RocksDB as a feature are hammered out.
Don't merge this PR until it has my go-ahead.
TimothyBanks
changed the title
TimothyBanks
force-pushed
the
session-optimizations-new
branch
from
d91b4b3
to
02286a9
Compare
libraries/chain/include/eosio/chain/backing_store/db_key_value_sec_lookup.hpp
Outdated
Show resolved
Hide resolved
libraries/chain/include/eosio/chain/backing_store/db_key_value_any_lookup.hpp
Outdated
Show resolved
Hide resolved
libraries/chain/include/eosio/chain/backing_store/db_key_value_sec_lookup.hpp
Outdated
Show resolved
Hide resolved
| // keys that have been deleted during this session. | ||
| mutable std::unordered_set<shared_bytes> m_deleted_keys; | ||
| parent_variant_type m_parent{ nullptr }; | ||
| mutable cache_type m_cache; |
There was a problem hiding this comment.
I don't know of a good reason why we want the cache to be mutable.
I would say that from a logical const-ness view point, the cache manipulations have external observable side effects.
There was a problem hiding this comment.
That means that all methods on this type cannot be const. So if you're fine with that I will remove mutable from this data member. The problem is that the "iterator cache" needs to be updated, so if we are calling that an observable side effect, then none of the methods can be const. I can imagine that this change will cascade out to other interfaces that take a const session type.
There was a problem hiding this comment.
This isn't really feasible. Once we remove mutable, it forces us to remove const on those member functions in session which cascades into other types that hold a reference to session and forces us to either const cast that reference or remove const on those member functions (for example, db_context_rocksdb holds a session and it has const member functions and I'm assuming that whomever holds a reference to db_context_rocksdb also has const member functions). It is quite the rabbit hole.
There was a problem hiding this comment.
I'll see how far down it goes.
There was a problem hiding this comment.
It stopped at db_context_rocksdb. Change has been made and pushed up.
There was a problem hiding this comment.
Some of these are optional, but I feel like the mutable interface of shared_bytes should be changed to have an mutable class, with no copy ctor or operator=, for creation and then that class is moved to the shared_bytes, which is immutable. Not sure how big an impact that is, but ir it is not huge I think shared_bytes, since the session is handing off access to its cached data, should not give everyone the right to write over its cache, particularly since half of those shared_bytes are used for sorting in containers.
| auto key = eosio::session::shared_bytes{sizeof(uint64_t) + prefix_key.size() + move_to_rocks->kv_key.size()}; | ||
| std::copy(std::begin(prefix_key), std::end(prefix_key), std::begin(key)); | ||
| b1::chain_kv::insert_key(key, prefix_key.size(), move_to_rocks->contract.to_uint64_t()); | ||
| std::copy(std::begin(move_to_rocks->kv_key), std::end(move_to_rocks->kv_key), std::begin(key) + prefix_key.size() + sizeof(uint64_t)); |
There was a problem hiding this comment.
Does the session return the same shared_bytes keys and values that it holds internally?
|
Updated. |
brianjohnson5972
dismissed
their stale review
The important issues were addressed, but leaving approval for Bucky.
Change Description
Removed cache.hpp and moved it entirely into session. Updated session by rolling all the containers used to managed various state information into 1 container.
Change Type
Select ONE
Consensus Changes
API Changes
Documentation Additions