search

dune-community / dune-xt-common

Other
2 stars 3 forks source link

Rebase PerThreadValue on tbb::enumerable_thread_specific #89

Closed tobiasleibner closed 6 years ago

tobiasleibner commented 6 years ago

The current implementation relies on a vector with length threadmanager.max_threads(). Unfortunately, TBB does not guarantee the maximal number of threads, the limit set by task_scheduler_init is just a soft limit. This lead to spurious segfaults when using PerThreadValue.

tobiasleibner commented 6 years ago

Note that I remove the boost::noncopyable because I didn't see why this class needs to be noncopyable. I also removed the operator=(ConstValueType&&) because I think its dangerous and if you really want to set the value in all threads you can now explicitly assign a new PerThreadValue. If we want to keep the ability to set all values in the existing PerThreadValue (for performance reasons?), I would vote for introducing a method for that (something like set_all_values(const ValueType&).

renefritze commented 6 years ago

And now I've realized that this dynamic storage could also be problematic wrt constness.

tobiasleibner commented 6 years ago

I now changed the enumerable_thread_specific to always hold the non-const value (it does not compile with const types). As we are always returning ValueType from methods like operator*, if ValueType is const, we only provide const access to the value, so it should behave as if the value was const.

renefritze commented 6 years ago

Ok, that sounds usable then. Can you check this change against dune-gdt master tests before merging please?

tobiasleibner commented 6 years ago

Tests should compile now also with clang, but the timing tests fail as TimingData currently starts timing on creation, and the objects in the PerThreadValue are now lazily created. I do not really see a way to fix this with the new implementation. So we can either rewrite the timer or stick with our own implementation and fix the issue (e.g. by dynamically checking and resizing the storage, or by setting a hard limit on the number of threads if possible (probably not, see https://software.intel.com/en-us/forums/intel-threading-building-blocks/topic/618706).

tobiasleibner commented 6 years ago

I added back the old implementation of PerThreadValue as UnsafePerThreadValue to use it in the timer and opened issue #90 so we can fix it properly at some point.