Simplify long/short term SNL data serialisation #1717
Conversation
|
I'm good with the simplification and getting implementation details out of the header here, but I'm not okay with the change to using a global variable to store the data. For one, it violates OO design principles by because it creates a violation of the promise of
Because that mutex isn't good enough anymore; effectively this creates a class that cannot guard its own internal data structures. While I think we can avoid the // service_node_list.cpp:
struct data_for_serialization { ... };
using serialize_cache_t = std::array<data_for_serialization, 2>; // {short term, long term}
// In constructor:
/*...*/, serialize_cache{new serialize_cache_t{}}, /*...*/
// In destructor:
service_node_list::~service_node_list { delete static_cast<serialize_cache_t*>(serialize_cache); }
// and in use in a member function:
auto& [short_term_data, long_term_data] = *static_cast<serialize_cache_t*>(serialize_cache);Edit after thinking some more and realizing that a struct serialize_cache_data; // predeclaration
class service_node_list {
// ...
// member variable:
std::unique_ptr<serialize_cache_data> serialize_cache;
~service_node_list();
};in the .h and the .cpp having: struct data_for_serialization { ... };
struct serialize_cache_data {
data_for_serialization short, long;
};
// Force the destructor instantiation to be here, where serialize_cache_data is actually visible:
service_node_list::~service_node_list() = default;
// And then in the member function:
auto& [short_term_data, long_term_data] = *serialize_cache; |
Doy-lee
force-pushed
the
doyle-data-serialisation-simplify
branch
2 times, most recently
from
26236a2
to
58c72cc
Compare
|
It's a fair point, kind of a footgun if the invariant one day breaks which I was okay with but it's not a big deal to avoid it. I've gone ahead and made the change, used the pimpl pattern as suggested but instead of just pulling in |
|
I received a deserialization error my first time running this. Subsequent runs have been fine. My guess is this won't be a problem just wanted to bring it forward. |
Interesting, I haven't changed the version numbers here yet for the blobs, by chance were you jumping around different branches for the stagenet? Some of those branches have some newer versions for If you then switch to this branch and run it'll complain about the unknown version that got serialized in the blob. This will need a rebase which I'll do now |
- Remove 'get_version' in data_for_serialisation. In the past we might of had a dynamic version that was assigned and it might of changed during a change in the hardfork. This is no longer the case and we always return a fixed value which means we don't use this feature. Inline and remove. - Remove the long/short term data cache in the transient struct on the SNL. In the past this was probably modified outside of the serialisation function 'service_node_list::store()' but now it is only modified in the single function and everytime we enter this function we clear out the cache and re-fill it in with runtime data. Potentially, it was cached to save a heap allocation on store, in this case I would switch to thread-local storage OR a static to reuse the vector storage. I've inline it for now onto the stack to reduce complexity in the header. - Pull data_for_serialization into the implementation file. This structure is not used outside of the SNL. - Rename 'state_added_to_archive' to 'long_term_dirty_flag' to more accurately convey the intent of this flag (e.g. when it's dirty, the next store will have to write the [long term] data to disk ...) - Remove the cache data blob, similar reasoning as above. This is cleared everytime we enter the function. I suspect it was done to save a heap allocation. The solution I'd advise here is thread-local storage or static. Simplifies the complicated SNL header. - Remove the for-loop to initialise the data for serialisation which just clears the structures and reassigns the version. The version is static now and default initialised so no need to set it. The data structures, potentially were there for heap allocation optimisation. Again recommended solution is thread local storage or static. -- I've chosen to use static because - There will always only be one SNL list instance in the entire program so there aren't multiple SNL's that we have to synchronise against. - Storing requires taking a mutex so the instruction level lock will never be contended. - Store will be called from multiple threads (multiple P2P threads AFAICT) so we'd end up with multiple per-thread 'caches' if we used thread-local storage.
Doy-lee
force-pushed
the
doyle-data-serialisation-simplify
branch
from
58c72cc
to
298811f
Compare
this was definitely the case, good catch |
Remove 'get_version' in data_for_serialisation. In the past we might of had a dynamic version that was assigned and it might of changed during a change in the hardfork. This is no longer the case and we always return a fixed value which means we don't use this feature. Inline and remove.
Remove the long/short term data cache in the transient struct on the SNL. In the past this was probably modified outside of the serialisation function 'service_node_list::store()' but now it is only modified in the single function and everytime we enter this function we clear out the cache and re-fill it in with runtime data.
Potentially, it was cached to save a heap allocation on store, in this case I would switch to thread-local storage OR a static to reuse the vector storage. I've inline it for now onto the stack to reduce complexity in the header.
Pull data_for_serialization into the implementation file. This structure is not used outside of the SNL.
Rename 'state_added_to_archive' to 'long_term_dirty_flag' to more accurately convey the intent of this flag (e.g. when it's dirty, the next store will have to write the [long term] data to disk ...)
Remove the cache data blob, similar reasoning as above. This is cleared everytime we enter the function. I suspect it was done to save a heap allocation.
The solution I'd advise here is thread-local storage or static. Simplifies the complicated SNL header.
--
I've chosen to use static because