Commit fe331012 authored by Milian Wolff's avatar Milian Wolff
Browse files

Port indexed identifier item repository access to external locking

Ensure the mutex is always locked when we operate on the item
repositories. This way, we only lock the mutex once for every
high level operation instead of potentially multiple times, e.g. to
lookup an index and then access the corresponding item.

I initially wanted to leverage this then to get rid of the recursive
mutex here too, like was done in other areas. But it seems that here
this is not possible, as the IndexedQualifiedIdentifier can load
IndexedIdentifiers, and by sharing the mutex we would need to have
that recursive. I.e. using a non-recursive mutex could trigger this
deadlock here:

```
(gdb) bt
#0  0x00007ffff3ea018d in syscall () at /usr/lib/libc.so.6
#1  0x00007ffff450c316 in QBasicMutex::lockInternal() () at /usr/lib/libQt5Core.so.5
#2  0x00007ffff6bfaf3d in QMutexLocker::QMutexLocker(QBasicMutex*)
    (this=0x7fffffffbb18, m=0x7ffff7a30c68 <KDevelop::RepoForItem<KDevelop::IndexedIdentifier>::repo()::mutex>) at /usr/include/qt/QtCore/qmutex.h:238
#3  0x00007ffff6cd2e20 in KDevelop::repositoryOp<KDevelop::IndexedIdentifier, KDevelop::inc<KDevelop::IndexedIdentifier>(KDevelop::IndexedIdentifier*)::<lambda(auto:23&)> >(struct {...} &&) (op=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:172
#4  0x00007ffff6cd2eb7 in KDevelop::inc<KDevelop::IndexedIdentifier>(KDevelop::IndexedIdentifier*) (item=0x5555556cf4d4)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:377
#5  0x00007ffff6cd26e8 in KDevelop::IndexedIdentifier::IndexedIdentifier(unsigned int) (this=0x5555556cf4d4, index=65604)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1392
#6  0x00007ffff6cd2764 in KDevelop::IndexedIdentifier::IndexedIdentifier(KDevelop::IndexedIdentifier const&) (this=0x5555556cf4d4, rhs=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1406
#7  0x00007ffff6cde959 in KDevelop::AppendedList<false, KDevelop::IndexedIdentifier>::copy(KDevelop::IndexedIdentifier*, KDevelop::IndexedIdentifier const*, unsigned int) (this=0x5555556cf4d0, target=0x5555556cf4d4, data=0x5555557195a0, size=1)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/appendedlist_static.h:148
#8  0x00007ffff6cdab26 in KDevelop::QualifiedIdentifierPrivate<false>::identifiersCopyAllFrom<KDevelop::QualifiedIdentifierPrivate<true> >(KDevelop::QualifiedIdentifierPrivate<true> const&) (this=0x5555556cf4c4, rhs=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:233
#9  0x00007ffff6cd7165 in KDevelop::QualifiedIdentifierPrivate<false>::copyListsFrom<KDevelop::QualifiedIdentifierPrivate<true> >(KDevelop::QualifiedIdentifierPrivate<true> const&) (this=0x5555556cf4c4, rhs=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:235
#10 0x00007ffff6cd4db9 in KDevelop::QualifiedIdentifierPrivate<false>::QualifiedIdentifierPrivate<true>(KDevelop::QualifiedIdentifierPrivate<true> const&)
    (this=0x5555556cf4c4, rhs=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:216
#11 0x00007ffff6cd4358 in KDevelop::QualifiedIdentifierItemRequest::createItem(KDevelop::QualifiedIdentifierPrivate<false>*) const
    (this=0x7fffffffbe80, item=0x5555556cf4c4) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:314
#12 0x00007ffff6cdb8a3 in KDevelop::Bucket<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, 0u>::index(KDevelop::QualifiedIdentifierItemRequest const&, unsigned int)::{lambda()#1}::operator()() const (__closure=0x7fffffffbd30)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:289
#13 0x00007ffff6cdbef8 in KDevelop::Bucket<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, 0u>::index(KDevelop::QualifiedIdentifierItemRequest const&, unsigned int) (this=0x5555555d8b30, request=..., itemSize=20)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:424
#14 0x00007ffff6cd77b5 in KDevelop::ItemRepositoryPrivate<KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, false, 0u, 1048576u>, KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, 0u, 1048576u>::index(KDevelop::QualifiedIdentifierItemRequest const&) (this=0x7fffd79d1028, request=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:1185
#15 0x00007ffff6cd4fdb in KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, false, 0u, 1048576u>::index(KDevelop::QualifiedIdentifierItemRequest const&) (this=0x7fffd79d1010, request=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:2326
#16 0x00007ffff6cd1caa in operator()(KDevelop::QualifiedIdentifierRepository&) const (__closure=0x7fffffffbf00, repo=warning: RTTI symbol not found for class 'KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, false, 0u, 1048576u>'
...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1230
#17 0x00007ffff6cd1d63 in KDevelop::repositoryOp<KDevelop::IndexedQualifiedIdentifier, KDevelop::QualifiedIdentifier::makeConstant() const::<lambda(KDevelop::QualifiedIdentifierRepository&)> >(struct {...} &&) (op=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:173
#18 0x00007ffff6cd1dc1 in KDevelop::QualifiedIdentifier::makeConstant() const (this=0x7fffffffbf90)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1229
#19 0x00007ffff6cd0787 in KDevelop::QualifiedIdentifier::operator=(KDevelop::QualifiedIdentifier const&) (this=0x7fffffffbfa0, rhs=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:850
#20 0x000055555556070a in TestIdentifier::testQualifiedIdentifier() (this=0x7fffffffc7d0)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/tests/test_identifier.cpp:134
#21 0x000055555555cb73 in TestIdentifier::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)
    (_o=0x7fffffffc7d0, _c=QMetaObject::InvokeMetaMethod, _id=4, _a=0x7fffffffc130)
    at /home/milian/projects/kde/build/kdevelop/kdevplatform/language/duchain/tests/test_identifier_autogen/EWIEGA46WW/moc_test_identifier.cpp:105
#22 0x00007ffff46c66bb in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const () at /usr/lib/libQt5Core.so.5
#23 0x00007ffff7f10ce2 in QTest::qRun() () at /usr/lib/libQt5Test.so.5
#24 0x00007ffff7f11eb2 in QTest::qExec(QObject*, int, char**) () at /usr/lib/libQt5Test.so.5
```

For now, I'll leave this part as-is and investigate changes in a
follow-up patch. For now, the changes proposed here are already good
for readability alone I believe.
parent 1c9649d7
......@@ -147,17 +147,35 @@ struct IdentifierItemRequest
const DynamicIdentifierPrivate& m_identifier;
};
using IdentifierRepository = RepositoryManager<ItemRepository<ConstantIdentifierPrivate, IdentifierItemRequest>, false>;
static IdentifierRepository& identifierRepository()
using IdentifierRepository = ItemRepository<ConstantIdentifierPrivate, IdentifierItemRequest, true, false>;
using IdentifierRepositoryManager = RepositoryManager<IdentifierRepository, false>;
template <typename Item>
struct RepoForItem;
template <>
struct RepoForItem<IndexedIdentifier> {
static IdentifierRepository& repo()
{
static QMutex mutex(QMutex::Recursive);
static IdentifierRepositoryManager manager(QStringLiteral("Identifier Repository"), &mutex);
return *manager.repository();
}
};
template <typename Item, typename Op>
static auto repositoryOp(Op&& op)
{
static auto mutex = QMutex(QMutex::Recursive);
static IdentifierRepository identifierRepositoryObject(QStringLiteral("Identifier Repository"), &mutex);
return identifierRepositoryObject;
auto& repo = RepoForItem<Item>::repo();
QMutexLocker lock(repo.mutex());
return op(repo);
}
static uint emptyConstantIdentifierPrivateIndex()
{
static const uint index = identifierRepository()->index(DynamicIdentifierPrivate());
static const uint index = repositoryOp<IndexedIdentifier>(
[](IdentifierRepository& repo) { return repo.index(DynamicIdentifierPrivate()); });
return index;
}
......@@ -318,19 +336,24 @@ struct QualifiedIdentifierItemRequest
const DynamicQualifiedIdentifierPrivate& m_identifier;
};
using QualifiedIdentifierRepository = RepositoryManager<ItemRepository<ConstantQualifiedIdentifierPrivate,
QualifiedIdentifierItemRequest>, false>;
using QualifiedIdentifierRepository
= ItemRepository<ConstantQualifiedIdentifierPrivate, QualifiedIdentifierItemRequest, true, false>;
using QualifiedIdentifierRepositoryManager = RepositoryManager<QualifiedIdentifierRepository, false>;
static QualifiedIdentifierRepository& qualifiedidentifierRepository()
{
static QualifiedIdentifierRepository repo(QStringLiteral("Qualified Identifier Repository"),
identifierRepository().repositoryMutex());
return repo;
}
template <>
struct RepoForItem<IndexedQualifiedIdentifier> {
static QualifiedIdentifierRepository& repo()
{
static QualifiedIdentifierRepositoryManager manager(QStringLiteral("Qualified Identifier Repository"),
RepoForItem<IndexedIdentifier>::repo().mutex());
return *manager.repository();
}
};
static uint emptyConstantQualifiedIdentifierPrivateIndex()
{
static const uint index = qualifiedidentifierRepository()->index(DynamicQualifiedIdentifierPrivate());
static const uint index = repositoryOp<IndexedQualifiedIdentifier>(
[](QualifiedIdentifierRepository& repo) { return repo.index(DynamicQualifiedIdentifierPrivate()); });
return index;
}
......@@ -340,22 +363,6 @@ static const ConstantQualifiedIdentifierPrivate* emptyConstantQualifiedIdentifie
return &item;
}
auto& managerForItem(IndexedIdentifier*)
{
return identifierRepository();
}
auto& managerForItem(IndexedQualifiedIdentifier*)
{
return qualifiedidentifierRepository();
}
template <typename Item>
auto& repoForItem(Item* item)
{
return *managerForItem(item).repository();
}
template <typename Item>
static inline bool inc(Item* item)
{
......@@ -363,12 +370,12 @@ static inline bool inc(Item* item)
return false;
}
auto& repo = repoForItem(item);
auto index = item->index();
QMutexLocker lock(repo.mutex());
ifDebug(qCDebug(LANGUAGE) << "increasing");
item->increase(repo.dynamicItemFromIndexSimple(index)->m_refCount, index);
repositoryOp<Item>([&](auto& repo) {
ifDebug(qCDebug(LANGUAGE) << "increasing");
item->increase(repo.dynamicItemFromIndexSimple(index)->m_refCount, index);
});
return true;
}
......@@ -379,12 +386,12 @@ static inline bool dec(Item* item)
return false;
}
auto& repo = repoForItem(item);
auto index = item->index();
QMutexLocker lock(repo.mutex());
ifDebug(qCDebug(LANGUAGE) << "decreasing");
item->decrease(repo.dynamicItemFromIndexSimple(index)->m_refCount, index);
repositoryOp<Item>([&](auto& repo) {
ifDebug(qCDebug(LANGUAGE) << "decreasing");
item->decrease(repo.dynamicItemFromIndexSimple(index)->m_refCount, index);
});
return true;
}
......@@ -396,17 +403,15 @@ static inline void setIndex(Item* item, unsigned int& m_index, unsigned int inde
}
if (shouldDoDUChainReferenceCounting(item)) {
auto& repo = repoForItem(item);
QMutexLocker lock(repo.mutex());
ifDebug(qCDebug(LANGUAGE) << "decreasing");
item->decrease(repo.dynamicItemFromIndexSimple(m_index)->m_refCount, m_index);
repositoryOp<Item>([&](auto& repo) {
ifDebug(qCDebug(LANGUAGE) << "decreasing");
item->decrease(repo.dynamicItemFromIndexSimple(m_index)->m_refCount, m_index);
m_index = index;
m_index = index;
ifDebug(qCDebug(LANGUAGE) << "increasing");
item->increase(repo.dynamicItemFromIndexSimple(m_index)->m_refCount, m_index);
ifDebug(qCDebug(LANGUAGE) << "increasing");
item->increase(repo.dynamicItemFromIndexSimple(m_index)->m_refCount, m_index);
});
} else {
m_index = index;
}
......@@ -428,22 +433,20 @@ static void moveIndex(Item* lhs, unsigned int& lhs_index, Item* rhs, unsigned in
return;
}
auto& repo = repoForItem(lhs);
QMutexLocker lock(repo.mutex());
if (lhsShouldDoDUChainReferenceCounting) {
lhs->decrease(repo.dynamicItemFromIndexSimple(lhs_index)->m_refCount, lhs_index);
} else if (rhsShouldDoDUChainReferenceCounting) {
rhs->decrease(repo.dynamicItemFromIndexSimple(rhs_index)->m_refCount, rhs_index);
}
repositoryOp<Item>([&](auto& repo) {
if (lhsShouldDoDUChainReferenceCounting) {
lhs->decrease(repo.dynamicItemFromIndexSimple(lhs_index)->m_refCount, lhs_index);
} else if (rhsShouldDoDUChainReferenceCounting) {
rhs->decrease(repo.dynamicItemFromIndexSimple(rhs_index)->m_refCount, rhs_index);
}
lhs_index = rhs_index;
rhs_index = emptyIndex;
lhs_index = rhs_index;
rhs_index = emptyIndex;
if (lhsShouldDoDUChainReferenceCounting && !rhsShouldDoDUChainReferenceCounting) {
lhs->increase(repo.dynamicItemFromIndexSimple(lhs_index)->m_refCount, lhs_index);
}
if (lhsShouldDoDUChainReferenceCounting && !rhsShouldDoDUChainReferenceCounting) {
lhs->increase(repo.dynamicItemFromIndexSimple(lhs_index)->m_refCount, lhs_index);
}
});
}
Identifier::Identifier(const Identifier& rhs)
......@@ -457,7 +460,7 @@ Identifier::Identifier(uint index)
: m_index(index)
{
Q_ASSERT(m_index);
cd = identifierRepository()->itemFromIndex(index);
cd = repositoryOp<IndexedIdentifier>([index](IdentifierRepository& repo) { return repo.itemFromIndex(index); });
}
Identifier::Identifier(const IndexedString& str)
......@@ -722,9 +725,12 @@ void Identifier::makeConstant() const
{
if (m_index)
return;
m_index = identifierRepository()->index(IdentifierItemRequest(*dd));
delete dd;
cd = identifierRepository()->itemFromIndex(m_index);
repositoryOp<IndexedIdentifier>([&](IdentifierRepository& repo) {
m_index = repo.index(IdentifierItemRequest(*dd));
delete dd;
cd = repo.itemFromIndex(m_index);
});
}
void Identifier::prepareWrite()
......@@ -746,13 +752,16 @@ bool QualifiedIdentifier::inRepository() const
{
if (m_index)
return true;
else
return ( bool )qualifiedidentifierRepository()->findIndex(QualifiedIdentifierItemRequest(*dd));
return repositoryOp<IndexedQualifiedIdentifier>([&](QualifiedIdentifierRepository& repo) {
return static_cast<bool>(repo.findIndex(QualifiedIdentifierItemRequest(*dd)));
});
}
QualifiedIdentifier::QualifiedIdentifier(uint index)
: m_index(index)
, cd(qualifiedidentifierRepository()->itemFromIndex(index))
, cd(repositoryOp<IndexedQualifiedIdentifier>(
[index](QualifiedIdentifierRepository& repo) { return repo.itemFromIndex(index); }))
{
}
......@@ -1219,9 +1228,12 @@ void QualifiedIdentifier::makeConstant() const
{
if (m_index)
return;
m_index = qualifiedidentifierRepository()->index(QualifiedIdentifierItemRequest(*dd));
delete dd;
cd = qualifiedidentifierRepository()->itemFromIndex(m_index);
repositoryOp<IndexedQualifiedIdentifier>([&](QualifiedIdentifierRepository& repo) {
m_index = repo.index(QualifiedIdentifierItemRequest(*dd));
delete dd;
cd = repo.itemFromIndex(m_index);
});
}
void QualifiedIdentifier::prepareWrite()
......
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