Iterating shared QMap on multiple threads

[Christian Ehrlicher]

@BrianL said in Iterating shared QMap on multiple threads:

so path should already existed in database?

You're right here but

https://doc.qt.io/qt-6/qmap.html#operator-5b-5d-1

operator[] const returns a value. So the reference you return goes out of scope.

[BrianL]

@Christian-Ehrlicher
ohh I see it now, I was trying to avoid using .value() because that returns a copy, but didn't notice operator[] const also returns a copy....

In that case is there no way to return a reference from QMap unless I made it return a non-const reference?

[IgKh]

@Christian-Ehrlicher @BrianL

The LanguageEntries const& languageEntries = database[path]; part is indeed problematic, but not because of what you think. database is a reference to a constant, so the const version of operator[] is called. However do notice that unlike the non-const version it returns a T, not a T&. Assigning it to a const l-value reference performs lifetime extension of the temporary that got returned until the end of the scope, but you are playing with fire doing it like that (e.g if you tried to return it). Due to the Copy-on-Write nature of Qt containers (which will lead me to the next point), they should generally be passed and assigned by value and not by reference (which might seem strange, but they are all actually shared-ownership reference wrappers anyway).

On the other hand GetEntryDatabase()[path][language] is exactly a mutating call, since it invokes the non-const operator[]. ANY non-const method on a CoW Qt container will first detach() the caller, even if it didn't end up modifying anything. The detach itself is thread-safe, as the copy-on-write semantics are implemented with atomic operations, but it does cause some iterator invalidation. The Qt documentation actually has a specific section on this - Implicit sharing iterator problem.

That specific case of detaching operation that doesn't actually modify the container is why it looks like if you fix the end iterator it works fine. It doesn't really. In such cases you should stick to external synchronization, or perhaps use the STL containers - they aren't thread safe as well, but their semantics are more obvious to C++ programmers.

[BrianL]

@IgKh said in Iterating shared QMap on multiple threads:

ANY non-const method on a CoW Qt container will first detach() the caller, even if it didn't end up modifying anything. The detach itself is thread-safe, as the copy-on-write semantics are implemented with atomic operations, but it does cause some iterator invalidation.

So even if I change my code to use T& operator[], it would still invalidate currently in-use iterator from other threads?

Entries &EntryDatabase::GetEntries(const QString &path, LanguageType language)
{
    EntryDatabase& database = GetDatabase();
    if (database.contains(path))
    {
        LanguageEntries& languageEntries = database[path];
        if (languageEntries.contains(language))
        {
            return languageEntries[language];
        }
    }

    static OCREntries emptyEntries;
    return emptyEntries;
}

[IgKh]

@BrianL said in Iterating shared QMap on multiple threads:

So even if I change my code to use T& operator[], it would still invalidate currently in-use iterator from other threads?

Basically yes. It is more nuanced than that, but discussing that would be just an academic exercise. As said, I think you are better served by std::vector and std::map for this use case.

[BrianL]

@IgKh Thanks for the answer! I have switch to using std::map now

[JonB]

@IgKh said in Iterating shared QMap on multiple threads:

Basically yes. It is more nuanced than that, but discussing that would be just an academic exercise. As said, I think you are better served by std::vector and std::map for this use case.

Without getting deep into a "nuanced" discussion, could you just summarise why these std ones are more suited here than the Qt ones, please? Is it because they do not get into references or temporaries or detaches or what? And the problem/difference would not arise/matter if multi-thread was not involved?

[Christian Ehrlicher]

@JonB said in Iterating shared QMap on multiple threads:

Is it because they do not get into references or temporaries or detaches or what?

std::map operator[] returns a reference whereas QMap operator[] const returns a temporary. That's the problem here.

[sierdzio]

I guess it's also an option to use QMap::find() to get a const iterator which will not be a copy of the data, unless I misremember stuff.

[SimonSchroeder]

@JonB said in Iterating shared QMap on multiple threads:

Without getting deep into a "nuanced" discussion, could you just summarise why these std ones are more suited here than the Qt ones, please?

I would say the most important point for this would be the "detaching" issue. Especially in the context of multithreading it is quite hard to figure out when detaches might happen. I guess the best way with QMap is to sprinkle qAsConst (or now even std::as_const) as often as possible. std::map just doesn't have this problem, so it is easier to get right with the STL compared to Qt.