Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes required to compile with TBB 2021.2 #33474

Merged
merged 11 commits into from
May 4, 2021
6 changes: 3 additions & 3 deletions CondCore/Utilities/bin/conddb_test_gt_perf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

#include <boost/thread/mutex.hpp>
#include "tbb/parallel_for_each.h"
#include "tbb/task_scheduler_init.h"
#include "tbb/global_control.h"

namespace cond {

Expand Down Expand Up @@ -505,7 +505,7 @@ int cond::TestGTPerf::execute() {
if (nThrF > 1)
session.transaction().commit();

tbb::task_scheduler_init init(nThrF);
tbb::global_control init(tbb::global_control::max_allowed_parallelism, nThrF);
std::vector<std::shared_ptr<FetchWorker> > tasks;

std::string payloadTypeName;
Expand Down Expand Up @@ -568,7 +568,7 @@ int cond::TestGTPerf::execute() {

std::shared_ptr<void> payloadPtr;

tbb::task_scheduler_init initD(nThrD);
tbb::global_control initD(tbb::global_control::max_allowed_parallelism, nThrD);
std::vector<std::shared_ptr<DeserialWorker> > tasksD;

timex.interval("setup deserialization");
Expand Down
5 changes: 4 additions & 1 deletion DetectorDescription/Core/interface/DDName.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include <iosfwd>
#include <string>
#include <utility>

#include "FWCore/Utilities/interface/StdPairHasher.h"
#include <tbb/concurrent_vector.h>
#include <tbb/concurrent_unordered_map.h>

Expand All @@ -15,7 +17,8 @@ class DDCurrentNamespace;
class DDName {
public:
using id_type = int;
using Registry = tbb::concurrent_unordered_map<std::pair<std::string, std::string>, id_type>;
using key_type = std::pair<const std::string, std::string>;
using Registry = tbb::concurrent_unordered_map<key_type, id_type, edm::StdPairHasher>;
using IdToName = tbb::concurrent_vector<Registry::const_iterator>;

//! Constructs a DDName with name \a name and assigns \a name to the namespace \a ns.
Expand Down
3 changes: 2 additions & 1 deletion DetectorDescription/Core/interface/DDValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "DetectorDescription/Core/interface/DDValuePair.h"
#include "tbb/concurrent_unordered_map.h"
#include "tbb/concurrent_vector.h"
#include "FWCore/Utilities/interface/zero_allocator.h"

/** A DDValue std::maps a std::vector of DDValuePair (std::string,double) to a name. Names of DDValues are stored
transiently. Furthermore, an ID is assigned std::mapping to the name.
Expand Down Expand Up @@ -107,7 +108,7 @@ class DDValue {

void init(const std::string&);

using Names = tbb::concurrent_vector<StringHolder, tbb::zero_allocator<StringHolder>>;
using Names = tbb::concurrent_vector<StringHolder, edm::zero_allocator<StringHolder>>;
static Names& names();
static Names initializeNames();

Expand Down
5 changes: 2 additions & 3 deletions DetectorDescription/Core/src/StoresInstantiation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,5 @@ template class DDI::Singleton<DDI::Store<DDName, std::unique_ptr<DDRotationMatri
template class DDI::Singleton<DDI::Store<DDName, std::unique_ptr<DDI::Division>, std::unique_ptr<DDI::Division> > >;
template class DDI::Singleton<std::map<std::string, std::vector<DDName> > >; //Used internally by DDLogicalPart
//The following are used by DDName
template class DDI::Singleton<tbb::concurrent_unordered_map<std::pair<std::string, std::string>, int> >;
template class DDI::Singleton<
tbb::concurrent_vector<tbb::concurrent_unordered_map<std::pair<std::string, std::string>, int>::const_iterator> >;
template class DDI::Singleton<DDName::Registry>;
template class DDI::Singleton<DDName::IdToName>;
1 change: 1 addition & 0 deletions FWCore/Framework/test/dependentrecord_t.cppunit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <memory>
#include <string>
#include <vector>
#include <cstring>
gartung marked this conversation as resolved.
Show resolved Hide resolved
gartung marked this conversation as resolved.
Show resolved Hide resolved

using namespace edm::eventsetup;

Expand Down
4 changes: 2 additions & 2 deletions FWCore/PluginManager/interface/PluginFactoryBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#include <atomic>
#include "tbb/concurrent_unordered_map.h"
#include "tbb/concurrent_vector.h"

#include "FWCore/Utilities/interface/zero_allocator.h"
#include "FWCore/Utilities/interface/Signal.h"
#include "FWCore/Utilities/interface/thread_safety_macros.h"
// user include files
Expand Down Expand Up @@ -61,7 +61,7 @@ namespace edmplugin {
std::atomic<void*> m_ptr;
};

typedef tbb::concurrent_vector<PluginMakerInfo, tbb::zero_allocator<PluginMakerInfo>> PMakers;
typedef tbb::concurrent_vector<PluginMakerInfo, edm::zero_allocator<PluginMakerInfo>> PMakers;
typedef tbb::concurrent_unordered_map<std::string, PMakers> Plugins;

// ---------- const member functions ---------------------
Expand Down
2 changes: 1 addition & 1 deletion FWCore/Services/plugins/InitRootHandlers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ namespace edm {
public:
typedef tbb::concurrent_unordered_set<pthread_t> Container_type;

ThreadTracker() : tbb::task_scheduler_observer(true) { observe(true); }
ThreadTracker() : tbb::task_scheduler_observer() { observe(); }
~ThreadTracker() override = default;

void on_scheduler_entry(bool) override {
Expand Down
6 changes: 4 additions & 2 deletions FWCore/Services/plugins/StallMonitor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include "FWCore/Utilities/interface/Algorithms.h"
#include "FWCore/Utilities/interface/OStreamColumn.h"
#include "FWCore/Utilities/interface/Exception.h"

#include "FWCore/Utilities/interface/StdPairHasher.h"
#include "tbb/concurrent_unordered_map.h"

#include <atomic>
Expand Down Expand Up @@ -227,7 +227,9 @@ namespace edm {
// for this purpose.
using StreamID_value = decltype(std::declval<StreamID>().value());
using ModuleID = decltype(std::declval<ModuleDescription>().id());
tbb::concurrent_unordered_map<std::pair<StreamID_value, ModuleID>, std::pair<decltype(beginTime_), bool>>
tbb::concurrent_unordered_map<std::pair<StreamID_value, ModuleID>,
std::pair<decltype(beginTime_), bool>,
edm::StdPairHasher>
stallStart_{};

std::vector<std::string> moduleLabels_{};
Expand Down
18 changes: 18 additions & 0 deletions FWCore/Utilities/interface/StdPairHasher.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#ifndef FWCore_Utilities_std_pair_hasher_h
#define FWCore_Utilities_std_pair_hasher_h
/*
tbb::hash was changed to used std::hash which does not have an implementation for std::pair.
*/
#include "FWCore/Utilities/interface/hash_combine.h"

namespace edm {
struct StdPairHasher {
std::size_t operator()(const std::pair<const std::string, const std::string>& a) const noexcept {
return edm::hash_value(a.first, a.second);
}
std::size_t operator()(const std::pair<const unsigned int, const unsigned int>& a) const noexcept {
return edm::hash_value(a.first, a.second);
}
};
} // namespace edm
#endif
2 changes: 1 addition & 1 deletion FWCore/Utilities/interface/hash_combine.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ namespace edm {
hash_combine(seed, args...);
return seed;
}
} // namespace edm

gartung marked this conversation as resolved.
Show resolved Hide resolved
} // namespace edm
#endif
61 changes: 61 additions & 0 deletions FWCore/Utilities/interface/zero_allocator.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#ifndef FWCore_Utilities_zero_allocator_h
#define FWCore_Utilities_zero_allocator_h
/*
Copyright (c) 2005-2020 Intel Corporation

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment stating that this was copied from oneTBB version X because of it was dropped from version Y, maybe with a link to the issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

#include "tbb/tbb_allocator.h"
#include <cstring>

/* Copied from tbb_2020 branch's tbb/tbb_allocator linked here
https://github.com/oneapi-src/oneTBB/blob/tbb_2020/include/tbb/tbb_allocator.h
and renamed to edm namespace because it was removed from oneapi_2021 branch's
tbb/tbb_allocator.
*/

namespace edm {
template <typename T, template <typename X> class Allocator = tbb::tbb_allocator>
class zero_allocator : public Allocator<T> {
public:
using value_type = T;
using base_allocator_type = Allocator<T>;
template <typename U>
struct rebind {
typedef zero_allocator<U, Allocator> other;
};

zero_allocator() throw() {}
zero_allocator(const zero_allocator &a) throw() : base_allocator_type(a) {}
template <typename U>
zero_allocator(const zero_allocator<U> &a) throw() : base_allocator_type(Allocator<U>(a)) {}

T *allocate(const std::size_t n, const void *hint = nullptr) {
//T* ptr = base_allocator_type::allocate( n, hint );
T *ptr = base_allocator_type::allocate(n);
std::memset(static_cast<void *>(ptr), 0, n * sizeof(value_type));
return ptr;
}
};

template <typename T1, template <typename X1> class B1, typename T2, template <typename X2> class B2>
inline bool operator==(const zero_allocator<T1, B1> &a, const zero_allocator<T2, B2> &b) {
return static_cast<B1<T1> >(a) == static_cast<B2<T2> >(b);
}
template <typename T1, template <typename X1> class B1, typename T2, template <typename X2> class B2>
inline bool operator!=(const zero_allocator<T1, B1> &a, const zero_allocator<T2, B2> &b) {
return static_cast<B1<T1> >(a) != static_cast<B2<T2> >(b);
}
} // namespace edm
#endif
3 changes: 1 addition & 2 deletions HLTrigger/Timer/plugins/FastTimerService.cc
Original file line number Diff line number Diff line change
Expand Up @@ -767,8 +767,7 @@ void FastTimerService::PlotsPerJob::fill_lumi(AtomicResources const& data, unsig
///////////////////////////////////////////////////////////////////////////////

FastTimerService::FastTimerService(const edm::ParameterSet& config, edm::ActivityRegistry& registry)
: tbb::task_scheduler_observer(true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was introduced in #33261; I don't know what the change does, but maybe @dan131riley can comment ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was needed to prevent the coroutine from being deleted before this service ended. The early deletion caused a segfault.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

class ThreadTracker : public tbb::task_scheduler_observer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onetbb_2021 removed the constructor that takes a global/local boolean. While the comments and docs still mention global observers, that actually isn't an option anymore--the only. constructors are a default (that actually does nothing) and one that takes a task_arena& to observe a specific arena other than the current one. See

https://github.com/oneapi-src/oneTBB/blob/2dba2072869a189b9fdab3ffa431d3ea49059a19/include/oneapi/tbb/task_scheduler_observer.h#L74-L85
https://spec.oneapi.com/versions/latest/elements/oneTBB/source/task_scheduler/task_arena/task_scheduler_observer_cls.html

In fact, we could (and probably should) just remove that line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So public tbb::task_scheduler_observer should be removed in both locations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThreadTracker and FastTimerService?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The member variable must stay, but the explicit call to the default constructor can be removed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually a parent class, and indeed the parent class needs to stay, it's just the explicit call to the constructor, the tbb::task_scheduler_observer(), that can be removed, since the default constructor will be called anyway.

// configuration
: // configuration
callgraph_(),
// job configuration
concurrent_lumis_(0),
Expand Down
6 changes: 4 additions & 2 deletions PhysicsTools/TensorFlow/interface/TBBThreadPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@

#include "tensorflow/core/lib/core/threadpool.h"

#include "tbb/task_scheduler_init.h"
#include "tbb/task_arena.h"
#include "tbb/task_group.h"
#include "tbb/global_control.h"

namespace tensorflow {

Expand All @@ -27,7 +27,9 @@ namespace tensorflow {
}

explicit TBBThreadPool(int nThreads = -1)
: nThreads_(nThreads > 0 ? nThreads : tbb::task_scheduler_init::default_num_threads()), numScheduleCalled_(0) {
: nThreads_(nThreads > 0 ? nThreads
: tbb::global_control::active_value(tbb::global_control::max_allowed_parallelism)),
numScheduleCalled_(0) {
// when nThreads is zero or smaller, use the default value determined by tbb
}

Expand Down