Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fixed|libcore: Improved thread-safety
Thread Sanitizer pointed out a number of places that hadn’t been
protected by mutexes. Various global ID counters now use std::atomic
types.

de::Animation’s current time was changed to be a static instance
within Animation that gets updated by the animation clock’s audience.
This avoids polling the clock and makes time checks easier (just
lock the time and do the check).
  • Loading branch information
skyjake committed Feb 10, 2017
1 parent 101e0ce commit 456532f
Show file tree
Hide file tree
Showing 17 changed files with 164 additions and 66 deletions.
14 changes: 14 additions & 0 deletions doomsday/sdk/libcore/include/de/concurrency/lockable.h
Expand Up @@ -63,6 +63,20 @@ class DENG2_PUBLIC Lockable
#endif
};

template <typename Type>
struct LockableT : public Lockable
{
typedef Type ValueType;
Type value;

LockableT() {}
LockableT(Type const &initial) : value(initial) {}
LockableT(Type &&initial) : value(initial) {}

operator Type &() { return value; }
operator Type const &() const { return value; }
};

} // namespace de

#endif // LIBDENG2_LOCKABLE_H
2 changes: 1 addition & 1 deletion doomsday/sdk/libcore/include/de/core/loop.h
Expand Up @@ -103,7 +103,7 @@ class DENG2_PUBLIC LoopCallback : public Lockable, DENG2_OBSERVES(Loop, Iteratio
~LoopCallback();

bool isEmpty() const;
operator bool() const;
inline operator bool() const { return !isEmpty(); }

void enqueue(Callback func);
void loopIteration() override;
Expand Down
5 changes: 5 additions & 0 deletions doomsday/sdk/libcore/include/de/data/observers.h
Expand Up @@ -349,6 +349,11 @@ class Observers : public Lockable, public IAudience
return *this;
}

Observers<Type> const &operator -= (Type &observer) const {
const_cast<Observers<Type> *>(this)->remove(&observer);
return *this;
}

size_type size() const {
DENG2_GUARD(this);
return _members.size();
Expand Down
4 changes: 4 additions & 0 deletions doomsday/sdk/libcore/include/de/data/time.h
Expand Up @@ -165,6 +165,8 @@ class DENG2_PUBLIC Time : public ISerializable
Time();

Time(Time const &other);

Time(Time &&moved);

Time(QDateTime const &t);

Expand All @@ -179,6 +181,8 @@ class DENG2_PUBLIC Time : public ISerializable

Time &operator = (Time const &other);

Time &operator = (Time &&moved);

bool isValid() const;

bool operator < (Time const &t) const;
Expand Down
2 changes: 1 addition & 1 deletion doomsday/sdk/libcore/include/de/filesys/fileindex.h
Expand Up @@ -144,7 +144,7 @@ class DENG2_PUBLIC FileIndex
QList<File *> files() const;

protected:
// C++ iterator (not thread safe):
// C++ iterator (not thread-safe):
typedef Index::const_iterator const_iterator;
Index::const_iterator begin() const;
Index::const_iterator end() const;
Expand Down
1 change: 1 addition & 0 deletions doomsday/sdk/libcore/include/de/libcore.h
Expand Up @@ -405,6 +405,7 @@ class PrivateAutoPtr
~PrivateAutoPtr() { reset(); }

PrivateAutoPtr &operator = (PrivateAutoPtr &&moved) {
reset();
ptr = moved.ptr;
moved.ptr = nullptr;
return *this;
Expand Down
2 changes: 1 addition & 1 deletion doomsday/sdk/libcore/include/de/widgets/animation.h
Expand Up @@ -177,7 +177,7 @@ class DENG2_PUBLIC Animation : public ISerializable, public Deletable
*/
static void setClock(Clock const *clock);

static Time const &currentTime();
static Time currentTime();

static Animation range(Style style, float from, float to, TimeDelta span, TimeDelta delay = 0);

Expand Down
10 changes: 6 additions & 4 deletions doomsday/sdk/libcore/src/concurrency/taskpool.cpp
Expand Up @@ -30,11 +30,13 @@ namespace de {

namespace internal
{
struct CallbackTask : public Task
class CallbackTask : public Task
{
TaskPool::TaskFunction func;
CallbackTask(TaskPool::TaskFunction func) : func(func) {}
void runTask() { func(); }
public:
CallbackTask(TaskPool::TaskFunction func) : _func(func) {}
void runTask() override { _func(); }
private:
TaskPool::TaskFunction _func;
};
}

Expand Down
3 changes: 2 additions & 1 deletion doomsday/sdk/libcore/src/core/clock.cpp
Expand Up @@ -17,14 +17,15 @@
*/

#include "de/Clock"
#include <atomic>

namespace de {

DENG2_PIMPL_NOREF(Clock)
{
Time startedAt;
Time time;
duint32 tickCount { 0 };
std::atomic_uint tickCount { 0 };

DENG2_PIMPL_AUDIENCE(TimeChange)
};
Expand Down
7 changes: 2 additions & 5 deletions doomsday/sdk/libcore/src/core/loop.cpp
Expand Up @@ -129,14 +129,11 @@ LoopCallback::~LoopCallback()

bool LoopCallback::isEmpty() const
{
DENG2_GUARD(this);

return _funcs.isEmpty();
}

LoopCallback::operator bool() const
{
return !isEmpty();
}

void LoopCallback::enqueue(Callback func)
{
DENG2_GUARD(this);
Expand Down
12 changes: 9 additions & 3 deletions doomsday/sdk/libcore/src/data/sourcelinetable.cpp
Expand Up @@ -19,16 +19,18 @@
#include "de/SourceLineTable"
#include "de/PathTree"

#include <atomic>

namespace de {

static int const SOURCE_SHIFT = 17;
static int const NUMBER_MASK = 0x1ffff;

DENG2_PIMPL_NOREF(SourceLineTable)
DENG2_PIMPL_NOREF(SourceLineTable), public Lockable
{
struct IdNode : public PathTree::Node
{
static LineId counter;
static std::atomic_uint counter;

LineId id;
IdNode(PathTree::NodeArgs const &args)
Expand All @@ -39,14 +41,16 @@ DENG2_PIMPL_NOREF(SourceLineTable)
QHash<duint, IdNode const *> lookup; // reverse lookup
};

SourceLineTable::LineId SourceLineTable::Impl::IdNode::counter = 0;
std::atomic_uint SourceLineTable::Impl::IdNode::counter;

SourceLineTable::SourceLineTable() : d(new Impl)
{}

SourceLineTable::LineId SourceLineTable::lineId(String const &path, duint lineNumber)
{
Path const source(path);

DENG2_GUARD(d);

auto const *node = d->paths.tryFind(source, PathTree::MatchFull | PathTree::NoBranch);
if (!node)
Expand All @@ -67,6 +71,8 @@ SourceLineTable::PathAndLine SourceLineTable::sourcePathAndLineNumber(LineId sou
{
duint const lineNumber = (NUMBER_MASK & sourceId);

DENG2_GUARD(d);

auto found = d->lookup.constFind(sourceId >> SOURCE_SHIFT);
if (found != d->lookup.constEnd())
{
Expand Down
43 changes: 39 additions & 4 deletions doomsday/sdk/libcore/src/data/stringpool.cpp
Expand Up @@ -20,6 +20,8 @@
#include "de/StringPool"
#include "de/Reader"
#include "de/Writer"
#include "de/Lockable"
#include "de/Guard"

#include <vector>
#include <list>
Expand Down Expand Up @@ -146,7 +148,7 @@ typedef std::set<CaselessStringRef> Interns;
typedef std::vector<CaselessString *> IdMap;
typedef std::list<InternalId> AvailableIds;

DENG2_PIMPL_NOREF(StringPool)
DENG2_PIMPL_NOREF(StringPool), public Lockable
{
/// Interned strings (owns the CaselessString instances).
Interns interns;
Expand All @@ -171,6 +173,8 @@ DENG2_PIMPL_NOREF(StringPool)

void clear()
{
DENG2_GUARD(this);

for (dsize i = 0; i < idMap.size(); ++i)
{
if (!idMap[i]) continue; // Unused slot.
Expand All @@ -184,7 +188,7 @@ DENG2_PIMPL_NOREF(StringPool)
assertCount();
}

void inline assertCount() const
inline void assertCount() const
{
DENG2_ASSERT(count == interns.size());
DENG2_ASSERT(count == idMap.size() - available.size());
Expand Down Expand Up @@ -276,11 +280,11 @@ DENG2_PIMPL_NOREF(StringPool)
};

StringPool::StringPool() : d(new Impl)
{
}
{}

StringPool::StringPool(String const *strings, uint count) : d(new Impl)
{
DENG2_GUARD(d);
for (uint i = 0; strings && i < count; ++i)
{
intern(strings[i]);
Expand All @@ -294,18 +298,24 @@ void StringPool::clear()

bool StringPool::empty() const
{
DENG2_GUARD(d);

d->assertCount();
return !d->count;
}

dsize StringPool::size() const
{
DENG2_GUARD(d);

d->assertCount();
return d->count;
}

StringPool::Id StringPool::intern(String str)
{
DENG2_GUARD(d);

Interns::iterator found = d->findIntern(str); // O(log n)
if (found != d->interns.end())
{
Expand All @@ -317,6 +327,8 @@ StringPool::Id StringPool::intern(String str)

String StringPool::internAndRetrieve(String str)
{
DENG2_GUARD(d);

InternalId id = IMPORT_ID(intern(str));
return *d->idMap[id];
}
Expand All @@ -327,6 +339,8 @@ void StringPool::setUserValue(Id id, uint value)

InternalId const internalId = IMPORT_ID(id);

DENG2_GUARD(d);

DENG2_ASSERT(internalId < d->idMap.size());
DENG2_ASSERT(d->idMap[internalId] != 0);

Expand All @@ -339,6 +353,8 @@ uint StringPool::userValue(Id id) const

InternalId const internalId = IMPORT_ID(id);

DENG2_GUARD(d);

DENG2_ASSERT(internalId < d->idMap.size());
DENG2_ASSERT(d->idMap[internalId] != 0);

Expand All @@ -351,6 +367,8 @@ void StringPool::setUserPointer(Id id, void *ptr)

InternalId const internalId = IMPORT_ID(id);

DENG2_GUARD(d);

DENG2_ASSERT(internalId < d->idMap.size());
DENG2_ASSERT(d->idMap[internalId] != 0);

Expand All @@ -363,6 +381,8 @@ void *StringPool::userPointer(Id id) const

InternalId const internalId = IMPORT_ID(id);

DENG2_GUARD(d);

DENG2_ASSERT(internalId < d->idMap.size());
DENG2_ASSERT(d->idMap[internalId] != 0);

Expand All @@ -371,6 +391,8 @@ void *StringPool::userPointer(Id id) const

StringPool::Id StringPool::isInterned(String str) const
{
DENG2_GUARD(d);

Interns::const_iterator found = d->findIntern(str); // O(log n)
if (found != d->interns.end())
{
Expand All @@ -382,6 +404,8 @@ StringPool::Id StringPool::isInterned(String str) const

String StringPool::string(Id id) const
{
DENG2_GUARD(d);

/// @throws InvalidIdError Provided identifier is not in use.
return stringRef(id);
}
Expand All @@ -396,13 +420,17 @@ String const &StringPool::stringRef(StringPool::Id id) const
return emptyString;
}

DENG2_GUARD(d);

InternalId const internalId = IMPORT_ID(id);
DENG2_ASSERT(internalId < d->idMap.size());
return *d->idMap[internalId];
}

bool StringPool::remove(String str)
{
DENG2_GUARD(d);

Interns::iterator found = d->findIntern(str); // O(log n)
if (found != d->interns.end())
{
Expand All @@ -416,6 +444,8 @@ bool StringPool::removeById(Id id)
{
if (id == 0) return false;

DENG2_GUARD(d);

InternalId const internalId = IMPORT_ID(id);
if (id >= d->idMap.size()) return false;

Expand All @@ -429,6 +459,7 @@ bool StringPool::removeById(Id id)

LoopResult StringPool::forAll(std::function<LoopResult (Id)> func) const
{
DENG2_GUARD(d);
for (duint i = 0; i < d->idMap.size(); ++i)
{
if (d->idMap[i])
Expand All @@ -443,6 +474,8 @@ LoopResult StringPool::forAll(std::function<LoopResult (Id)> func) const
// Implements ISerializable.
void StringPool::operator >> (Writer &to) const
{
DENG2_GUARD(d);

// Number of strings altogether (includes unused ids).
to << duint32(d->idMap.size());

Expand All @@ -456,6 +489,8 @@ void StringPool::operator >> (Writer &to) const

void StringPool::operator << (Reader &from)
{
DENG2_GUARD(d);

clear();

// Read the number of total number of strings.
Expand Down

0 comments on commit 456532f

Please sign in to comment.