Restricting thread_data to creating only with intrusive_pointers #1765
Conversation
| memset (p, freed_value, sizeof(thread_data)); | ||
| #endif | ||
| pool.deallocate(static_cast<thread_data*>(p)); | ||
| if (thread_data* td = dynamic_cast<thread_data*>(p)) |
There was a problem hiding this comment.
Why do we need this dynamic_cast?
There was a problem hiding this comment.
@hkaiser: In current implementation thread_data_base can also refer to stackless_thread_data that doesn't use a pool for allocation scheme.
There was a problem hiding this comment.
@AntonBikineev This operator delete() belongs to thread_data, not the base class. It shouldn't be called for a stackless_thread_data instance.
There was a problem hiding this comment.
@hkaiser sorry, Hartmut, don't get your point. This operator delete doesn't exist in this PR (I removed it). global delete is needed to be called for stackless_thread_data, as its object is allocated with global new here https://github.com/STEllAR-GROUP/hpx/blob/fixing_1763_2/hpx/runtime/threads/policies/thread_queue.hpp#L220
There was a problem hiding this comment.
Doh! my bad - I got confused by the diff. You're certainly right.
There was a problem hiding this comment.
@hkaiser, I guessed that diff might have confused you :)
There was a problem hiding this comment.
I don't like that dynamic_cast here. A virtual thread_pool *get_pool() { return 0; } in thread_data_base and a thread_pool *get_pool() { return pool_; } in thread_data would avoid this.
There was a problem hiding this comment.
@sithhell don't like it either. I thought about the approach with get_pool. In this way is_created_from(pool*) also seems to be redundant. Another problem is that we have to check whether actual type is thread_data, not stackless_thread_data, because the latter doesn't actually use a pool for its creation/deletion.
Ideally, we should also think about the interface of these three classes and specify, do we need creation from pool be defined in thread_data_base?
|
I think we can remove the |
|
LGTM now. Thanks! |
Restricting thread_data to creating only with intrusive_pointers
@hkaiser @K-ballo @sithhell What do you think of this sort of approach?
It seems to be close to the design where pool is a part of the thread_data model, but introduces some sort of dependency in intrusive_ptr_release with dynamic_cast..