Replace ExecutionStatePool with ObjectPool #608
Conversation
Codecov Report
@@ Coverage Diff @@
## master #608 +/- ##
==========================================
- Coverage 81.75% 81.75% -0.01%
==========================================
Files 169 167 -2
Lines 13958 13947 -11
==========================================
- Hits 11411 11402 -9
+ Misses 2547 2545 -2
Continue to review full report at Codecov.
|
| using ptr_t = std::unique_ptr<T, TDtor>; | ||
|
|
||
| ObjectPool() = default; | ||
| explicit ObjectPool(bool thread_safe = false) : thread_safe_{thread_safe} {} |
There was a problem hiding this comment.
if there a location where ObjectPool is created with thread_safe=true?
There was a problem hiding this comment.
Yes, in cmd/test/consensus.
| EvmHost host{*this}; | ||
|
|
||
| gsl::owner<EvmoneExecutionState*> state{acquire_state()}; |
There was a problem hiding this comment.
Before unique_ptr did protect the life of the pointed object in case of an exception, gsl::owner doesn't do that.
Why did you replace unique_ptr with gsl::owner? had you been forced by ObjectPool interface?
In the next comment I sketch an ObjectPool interface that let's us use unique_ptr
|
|
||
| void add(T*& t) { | ||
| void add(gsl::owner<T*> t) { | ||
| SILKWORM_DETAIL_OBJECT_POOL_GUARD |
There was a problem hiding this comment.
There is another way to implement this:
- we can add a mutex-type parameter to the template
template <class T, class M, class TDtor = std::default_delete<T>>
class ObjectPool {
...
mutable M mutex_;
}
so the user can use a real mutex or a null_mutex (whose methods do nothing)
- then code the methods usually
void add(gsl::owner<T*> t) {
std::unique_lock lock(mutex_);
...
}
The only problem is that standard library lacks null_mutex, it is present in boost.
There was a problem hiding this comment.
The reason I didn't add an extra template parameter is because I don't want to templatize EVM.
ObjectPool<EvmoneExecutionState>* state_pool{nullptr};
is fully specialized there.
| if (pool_.empty()) { | ||
| return nullptr; | ||
| } | ||
| T* ret(pool_.top().release()); | ||
| gsl::owner<T*> ret(pool_.top().release()); |
There was a problem hiding this comment.
I think the ObjectPool can be improved if we use unique_ptr not only inside but also outside so that no leak can happen neither by forgetfulness nor by exception. (Or maybe we do not want to address this case because we have less or no function throwing exception).
Sorry for not saying this before but I have just realized it here seeing that you have been forced to replace unique_ptr with gsl::owner in the methods implementations (see my next comment).
This is a sample implementation returning and accepting unique_ptr:
template <class T> // sample implementation
class ObjectPool {
public:
using ptr_t = std::unique_ptr<T>;
explicit ObjectPool() {}
void add(ptr_t t) { // the user is forced to do a move here, i.e. transfer ownership
pool_.push(std::move(t));
}
ptr_t acquire() {
if (pool_.empty()) {
return nullptr;
}
ptr_t ret;
swap(ret, pool_.top());
pool_.pop();
return ret; // transfer ownership
}
private:
std::stack<ptr_t, std::vector<ptr_t>> pool_{};
};
So the usage becomes:
auto resource = pool.acquire();
...
pool.add(std::move(resource));
There was a problem hiding this comment.
std::unique_ptr is probably a good fit for the use of ObjectPool for evm states, but it might be a bad fit for its use in node/silkworm/db/mdbx.cpp. Prior to PR #605 ObjectPool was actually using std::unique_ptr. In this PR I'm simply using gsl::owner for formalization of the changes made in PR #605.
There was a problem hiding this comment.
And core doesn't use exceptions, so no need to worry about them.
No description provided.