Skip to content
GitLab
Commit ef2608f4 authored by scott snyder's avatar scott snyder
Browse files

Add initial discussion of rules for AthenaMT.

parent 6e1b1355
Hide whitespace changes
Inline Side-by-side
rules.org
View file @ ef2608f4
......@@ -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
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment