diff --git a/rules.org b/rules.org index 4f3828baaca95027bf239bbd3e52176e8b922d01..6f1b58705e540e1e01472ce4e52f066743f6d45a 100644 --- a/rules.org +++ b/rules.org @@ -1112,7 +1112,7 @@ int convertAndBuffer (int* buf, float x) #+END_EXAMPLE (As a special case, you can safely convert any pointer type to or from - a =char*= with.) The proper way to do such a conversion is with a union. + a =char*=.) The proper way to do such a conversion is with a union. ** The class interface @@ -1253,7 +1253,7 @@ void foo (std::unique_ptr<Object> obj); } #+END_EXAMPLE - - *Return basic types or new instances of a class type by value*. [<<return-by-value>>]] + - *Return basic types or new instances of a class type by value*. [<<return-by-value>>] Returning a class instance by value is generally preferred to passing an argument by non-const reference: @@ -1669,6 +1669,245 @@ float int (int x) and avoid such surprises. +** Thread friendliness and thread safety + +Code that is to be run in AthenaMT as part of an =AthAlgorithm= must +be ``thread-friendly.'' While the framework will ensure that no more +than one thread is executing a given =AthAlgorithm= instance at one time, +the code must ensure that it doesn't interfere with /other/ threads. +Some guidelines for this are outlined below; but in brief: don't +use static data, don't use =mutable=, and don't cast away =const=. +Following these rules will keep you out of most potential trouble. + +Code that runs as part of an =AthService=, an =AthReentrantAlgorithm=, +a data object implementation, +or other common code may need to be fully ``thread-safe''; that is, +allow for multiple threads to operate simultaneously on the /same/ object. +The easiest way to ensure this is for the object to have no mutable +internal state, and only =const= methods. If, however, some threads +may be modifying the state of the object, then some sort of locking +or other means of synchronization will likely be required. A full +discussion of this is beyond the scope of these guidelines. + +To run successfully in a multithreaded environment, algorithmic code +must also respect the rules imposed by the framework on event and conditions +data access. This is also beyond the scope of these guidelines. + + + - *Follow C++ thread-safety conventions for data objects.* [<<mt-follow-c++-conventions>>] + + The standard C++ container objects follow the rule that methods declared + as =const= are safe to call simultaneously from multiple threads, + while no non-const method can be called simultaneously with any other + method (=const= or non-const) on the same object. + + Classes meant to be data objects should generally follow the same rules, + unless there is a good reason to the contrary. This will generally + happen automatically if the rules outlined below are followed: briefly, + don't use static data, don't use =mutable=, and don't cast away =const=. + + Sometimes it may be useful to have data classes for which non-const methods + may be called safely from multiple threads. If so, this should be + indicated in the documentation of the class, and perhaps hinted + from its name (maybe like =ConcurrentFoo=). + + + - *Do not use non-const static variables* [<<mt-no-nonconst-static>>] + + Do not use non-const static variables in thread-friendly code, + either global or local. + + #+BEGIN_EXAMPLE +int a; +int foo() { + if (a > 0) { // Bad use of global static. + static int count = 0; + return ++count; // Bad use of local static. + } + return 0; +} + +struct Bar +{ + static int s_x; + int x() { return s_x; } // Bad use of static class member. +}; +#+END_EXAMPLE + + A const static is, however, perfectly fine: + + #+BEGIN_EXAMPLE + static const std::string s = "a string"; // ok, const +#+END_EXAMPLE + + It's generally ok to have static mutex or thread-local variables: + + #+BEGIN_EXAMPLE +static std::mutex m; // Ok. It's a mutex, + // so it's meant to be accessed + // from multiple threads. +static thread_local int a; // Ok, it's thread-local. +#+END_EXAMPLE + + (Be aware, though, that thread-local variables can be quite slow.) + A static =std::atomic<T>= variable may be ok, but only if it doesn't + need to be updated consistently with other variables. + + + - *Do not cast away const* [<<mt-no-const-cast>>] + + This rule was already mentioned above. However, it deserves particular + emphasis in the context of thread safety. The usual convention for C++ + is that a =const= method is safe to call simultaneously from multiple + threads, while if you call a non-const method, no other threads can be + simultaneously accessing the same object. If you cast away =const=, + you are subverting these guarantees. Any use of =const_cast= needs + to be analyzed for its effects on thread-safety and possibly protected + with locking. + + For example, consider this function: + #+BEGIN_EXAMPLE +void foo (const std::vector<int>& v) +{ + ... + // Sneak this in. + const_cast<std::vector<int>&>(v).push_back(10); +} +#+END_EXAMPLE + Someone looking at the signature of this function would see that it + takes only a =const= argument, and therefore conclude that that it is safe + to call this simultaneously with other code that is also reading + the same vector instance. But it is not, and the =const_cast= is what + causes that reasoning to fail. + + + - *Avoid mutable members* [<<mt-no-mutable>>] + + The use of =mutable= members has many of the same problems + as =const_cast= (as indeed, =mutable= is really just a restricted + version of =const_cast=). A =mutable= member can generally not + be changed from a non-const method without some sort of explicit + locking or other synchronization. It is best avoided in code + that should be used with threading. + + =mutable= can, however, be used with objects that are explicitly intended + to be accessed from multiple threads. These include mutexes and + thread-local pointers. In some cases, members of =atomic= type may also be + safely made =mutable=, but only if they do not need to be updated + consistently with other members. + + + - *Do not return non-const member pointers/references from const methods* [<<mt-const-consistency>>] + + Consider the following fragment: + #+BEGIN_EXAMPLE +class C +{ +public: + Impl* impl() const { return m_impl; } +private: + Impl* m_impl; +}; +#+END_EXAMPLE + This is perfectly valid according to the C++ =const= rules. However, + it allows modifying the =Impl= object following a call to the =const= method + =impl()=. Whether this is actually a problem depends on the context. + If =m_impl= is pointing at some unrelated object, then this might be ok; + however, if it is pointing at something which should be considered + part of =C=, then this could be a way around the =const= guarantees. + + To maintain safety, and to make the code easier to reason about, + do not return a non-const pointer (or reference) member from a + =const= member function. + + + - *Be careful returning const references to class members.* [<<mt-const-references>>] + + Consider the following example: + #+BEGIN_EXAMPLE +class C +{ +public: + const std::vector<int>& v() const { return m_v; } + void append (int x) { m_v.push_back (x); } +private: + std::vector<int> m_v; +}; + +int getSize (const C& c) +{ + return c.v().size(); +} + +int push (C& c) +{ + c.append (1); +} +#+END_EXAMPLE + + This is a fairly typical example of a class that has a large object as a + member, with an accessor the returns the member by const reference to + avoid having to do a copy. + + But suppose now that one thread calls =getSize()= while another thread + calls =push()= at the same time on the same object. It can happen + that first =getSize()= gets the reference and starts the call to =size()=. + At that point, the =push_back()= can run in the other thread. + If =push_back()= runs at the same time as =size()=, then the results + are unpredictable --- the =size()= call could very well return garbage. + + Note that it doesn't help to add locking within the class =C=: + #+BEGIN_EXAMPLE +class C +{ +public: + const std::vector<int>& v() const + { + std::lock_guard<std::mutex> lock (m_mutex); + return m_v; + } + void append (int x) + { + std::lock_guard<std::mutex> lock (m_mutex); + m_v.push_back (x); + } +private: + mutable std::mutex m_mutex; + std::vector<int> m_v; +}; +#+END_EXAMPLE + This is because the lock is released once =v()= returns --- and at that + point, the caller can call (=const=) methods on the =vector= instance + unprotected by the lock. + + Here are a few ways in which this could possibly be solved. + Which is preferable would depend on the full context in which the + class is used. + + - Change the =v()= accessor to return the member by value instead + of by reference. + + - Remove the =v()= accessor and instead add the needed operations + to the =C= class, with appropriate locking. For the above example, + we could add something like: + #+BEGIN_EXAMPLE + size_t C::vSize() const + { + std::lock_guard<std::mutex> lock (m_mutex); + return m_v.size(); + } +#+END_EXAMPLE + + - Change the type of the =m_v= member to something that is inherently + thread-safe. This could mean replacing it with a wrapper around + =std::vector= that does locking internally, or using something + like =concurrent_vector= from TBB. + + - Do locking externally to class =C=. For example, introduce a mutex + that must be acquired in both =getSize()= and =push()= in the above + example. + + ** Assertions and error conditions - *Pre-conditions and post-conditions should be checked for validity.* [<<pre-post-conditions>>] @@ -2607,6 +2846,9 @@ The comment includes the fact that it is the perpendicular distance. * Changes +** Version 0.5 + - Add an initial set of guidelines for AthenaMT. + ** Version 0.4 - Minor updates: we're now using c++14. Add note about implicit fallthrough warnings with gcc7. Add rule to use std::abs(). @@ -2653,27 +2895,27 @@ the python side with a starting upper or lower case? Zach: -It's great that you have "why" in many places, but some others could be useful.\ - Why no friends and why no macros for functions are two that come to mind. Th\ -e latter started getting picked up in our simulation code after somebody went t\ -o an OpenLab tutorial and decided they were great. +It's great that you have "why" in many places, but some others could be useful. + Why no friends and why no macros for functions are two that come to mind. +The latter started getting picked up in our simulation code after somebody +went to an OpenLab tutorial and decided they were great. You have the override keyword, but no "final" that I see? Do we just not care? -It might be useful to have a reminder about how singletons should work - we hav\ -e a lot of cases of pointers hanging out in various places, and several differe\ -nt implementations (some old-style, and some newer-style). +It might be useful to have a reminder about how singletons should work - + we have a lot of cases of pointers hanging out in various places, and several +different implementations (some old-style, and some newer-style). -You probably need a warning in [atlas-units] that the coder needs to be aware o\ -f what they're getting - for example, if we get a Geant4 object, it will use CL\ -HEP units, which "in principle" can be different from Gaudi units. +You probably need a warning in [atlas-units] that the coder needs to be aware +of what they're getting - for example, if we get a Geant4 object, it will use +CLHEP units, which "in principle" can be different from Gaudi units. -I'd like a style recommendation that if someone undertakes to "fix" the style o\ -f some code, they should check in only style changes, and then separately check\ - in functionality changes (ideally with a separate tag). Diff'ing two tags bec\ -omes absolutely impossible after significant reformatting, and debugging code t\ -hat's been touched here and there and reformatted all in one go is like startin\ -g over. +I'd like a style recommendation that if someone undertakes to "fix" the style +of some code, they should check in only style changes, and then separately +check in functionality changes (ideally with a separate tag). Diff'ing two +tags becomes absolutely impossible after significant reformatting, and +debugging code that's been touched here and there and reformatted all +in one go is like starting over. Doxygen also knows about //!< , I think? @@ -2721,15 +2963,15 @@ Doxygen also knows about //!< , I think? # LocalWords: krzemienski14 hrefsave nolinkurl Vlissides Sutter lxr # LocalWords: Krzemie nacute x144 RecustomVerbatimEnvironment rfoot # LocalWords: endnote makeenmark theenmark notesname enoteformat Ok -# LocalWords: enoteformatsave makeatletter thefnmark makeatother +# LocalWords: enoteformatsave makeatletter thefnmark makeatother Ok # LocalWords: lineno linenumbers fancyhdr pagestyle MYHEADER 42ul -# LocalWords: doSomething doSomethingElse caloCellContainer ufoo -# LocalWords: ret b''s convertAndBuffer buf fbuf updateTrack 0pt +# LocalWords: doSomething doSomethingElse caloCellContainer ufoo Ok +# LocalWords: ret b''s convertAndBuffer buf fbuf updateTrack 0pt ok # LocalWords: interoperate getVector makeFoo decrement 2ex bwd BNL # LocalWords: FaqCompileTimeWarnings DoxygenDocumentation mylabel # LocalWords: rulesversion FALLTHROUGH doSomethingMore gcc7 cstdlib # LocalWords: fallthrough fabs bitfields multithreaded atan2 n4140 -# LocalWords: Zach OpenLab +# LocalWords: Zach OpenLab AthenaMT AthAlgorithm AthService mutex # Local Variables: @@ -2742,3 +2984,7 @@ Doxygen also knows about //!< , I think? # eval: (defun org-html-target (target contents info) (let ((id (org-export-solidify-link-text (org-element-property :value target)))) (org-html--anchor id id))) # eval: (setq org-export-with-email t) # End: +# LocalWords: AthReentrantAlgorithm ConcurrentFoo nonconst mutexes +# LocalWords: Impl impl getSize accessor TBB Karsten MyFlag myFlag +# LocalWords: declareProperty myFlagValue Diff'ing nconc defun 'org +# LocalWords: backend 'latex 'my 42ul 0pt 2ex Geant4