library BrowseFeature: add threading for IO #3623

vigsterkr · 2021-02-09T11:34:11Z

It's never a good idea to call IO in a UI thread as anything can go wrong, see for example this bug.

some other details of this patch:

since the project requires c++17 feature, i've replaced the directory checking logic using std::filesystem
switched to std::unordered_map instead of QHash in FolderTreeModel for performance reasons

Tasks that still needs to be done:

~~handle the returned Future of QtConcurrent::run and update the UI based on a timer if loading takes more time than n seconds~~ WONTFIX
add Read/Write mutex protecting FolderTreeModel::m_directoryCache
finish QFileSystemWatcher logic for monitoring FS changes.
only specialize std::hash<QString> when needed (avoid redefinition error)

vigsterkr · 2021-02-09T11:39:27Z

Not strictly related, but there's a performance regression with the whole WLibrarySidebar & SidebarModel: when using mouse to scroll around in the WLibrarySidebar for some reason even for a smallest scroll or even mouse movement the FolderTreeModel::hasChildren function is being called. This adds a lot of signaling overhead to the whole system, for no apparent reason (or not that i can think of) especially that the very same behaviour cannot be observed when using the keyboard to move round in the view

daschuer · 2021-02-09T12:01:46Z

Thank you for picking this up.

Not strictly related, but there's a performance regression

Between which versions? Is it related to https://bugs.launchpad.net/mixxx/+bug/1905124

vigsterkr · 2021-02-09T12:12:36Z

Between which versions? Is it related to https://bugs.launchpad.net/mixxx/+bug/1905124

i'm testing based off main branch. and yeah probably the bug i was mentioning above is actually related, because currently any mouse movement in the sidebar kicks of a redraw right away that trickles down to call FolderTreeModel::hasChildren which is either a direct IO call to the FS or a lookup in the hashmap. but either way as mentioned this redraw should not be triggered, as it cannot be observed (and everything is working as expected) when using keyboard...

this patch would actually make the app more usable the scenario mentioned in 1905124, but the actual fix should be somewhere else. this patch just makes it sure that we never block UI because of IO calls/delays.

src/library/browse/browsefeature.cpp

vigsterkr · 2021-02-09T12:43:57Z

i'm guessing based on the CI error, that you dont want to explicitly require std::filesystem feature out of C++17 standard... or? in case not then i'll revert those changes... in case yes i can add a cmake script to check whether the compiler supports std::filesystem. note this will likely to cause troubles with some old system and their default compilers.

uklotzde · 2021-02-09T15:26:01Z

i'm guessing based on the CI error, that you dont want to explicitly require std::filesystem feature out of C++17 standard... or? in case not then i'll revert those changes... in case yes i can add a cmake script to check whether the compiler supports std::filesystem. note this will likely to cause troubles with some old system and their default compilers.

Not all supported platforms provide the same level of features and compatibility. We are not excluding anything explicitly.

daschuer · 2021-02-09T15:41:18Z

By the way, can you please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done?
This allows to include your patch in Mixxx.
Thank you.

vigsterkr · 2021-02-09T16:03:57Z

i'm guessing based on the CI error, that you dont want to explicitly require std::filesystem feature out of C++17 standard... or? in case not then i'll revert those changes... in case yes i can add a cmake script to check whether the compiler supports std::filesystem. note this will likely to cause troubles with some old system and their default compilers.

Not all supported platforms provide the same level of features and compatibility. We are not excluding anything explicitly.

gotcha! then i'm just gonna be leaving the old code. although i guess i could check with macros if std::filesystem is available and use that and fall back to current code, but that would at a lot of noise to the code and there's literally no reason (although haven't checked whether there are any performance differences, but dont expect too much) to replace the currently working solution.

Be-ing · 2021-02-09T16:05:19Z

This is why we have CI. :) mixxxdj/website#205

vigsterkr · 2021-02-09T16:07:20Z

By the way, can you please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done?
This allows to include your patch in Mixxx.
Thank you.

🐐 it's GPLv2, no? :) dont get me wrong i dont have anything against signing things, i'm just not really sure why it's necessary :)

Be-ing · 2021-02-09T16:08:47Z

We don't currently distribute Mixxx via the Mac App Store and have not for years, but we ask people to sign the CLA anyway in case so we could in the future and we have contact information for everyone in case we want to make any other changes to the license.

vigsterkr · 2021-02-10T08:56:16Z

ok after spending way too much time in order to fix all the requested changes (using signals/slots, adding a mutex), the whole thing blew up. namely spin ball and blocked UI is back. i'm really not sure what's happening, but i have a strong feeling that this whole TreeItemModel (especially the FolderItemModel) is flawed by design. as none of the UI action items should ever block basically rendering a TreeView.

i'm gonna leave this patch here as it at least works and makes the current main usable. it's up to you guys what to do with it...

vigsterkr · 2021-02-10T09:04:47Z

namely the problem is that FolderTreeModel::hasChildren can block, either because of a mutex or because of IO... and this should never happen as this is actually used by the TreeView which resides in UI thread (afaik)... and having on top the regression for firing redraw of tree for every little mouse movement this is just wasting resources prime time...

vigsterkr · 2021-02-10T09:20:07Z

@uklotzde just for the reference based on your and this suggestion patching the following:

        auto listWorker = [=]() mutable -> void {
            // we assume that the path refers to a folder in the file system
            // populate childs
            MDir dir(path);
            QList<TreeItem*> children;
            QFileInfoList all = dir.dir().entryInfoList(
                    QDir::Dirs | QDir::NoDotAndDotDot);

            // loop through all the item and construct the childs
            foreach (QFileInfo one, all) {
#if defined(__APPLE__)
                if (one.isDir() && one.fileName().endsWith(".app"))
                    continue;
#endif
                // We here create new items for the sidebar models
                // Once the items are added to the TreeItemModel,
                // the models takes ownership of them and ensures their deletion
                auto* folder = new TreeItem(
                        one.fileName(),
                        QVariant(one.absoluteFilePath() + QStringLiteral("/")));
                children << folder;
            }
            if (!children.isEmpty()) {
                // the reason for this indirection
                // https://stackoverflow.com/q/1144240 
                emit scanDir(index, children);
            }
        };

where scanDir is connected to FolderTreeModel::onNewChildren(const QModelIndex&, const QList<TreeItem*>&) causes the whole eventloop blow up in a way that the whole UI gets blocked 💃

or you had something else in mind?

poelzi · 2021-02-10T09:26:40Z

@vigsterkr have you tried forcing a queued connection ?

poelzi · 2021-02-10T09:31:09Z

I think you should create a real background thread that does the io querying and emitting the results. I did something similar in:
#3406 to populate the infobar with crate/playlist informations and I did not have any problems or crashes so far.

vigsterkr · 2021-02-10T09:35:23Z

@vigsterkr have you tried forcing a queued connection ?

connect(this, &BrowseFeature::scanDir, &m_childModel, &FolderTreeModel::onNewChildren, Qt::QueuedConnection);

this is a queued connection, or? in case yes, then i've tried it...

vigsterkr · 2021-02-10T09:37:26Z

I think you should create a real background thread that does the io querying and emitting the results. I did something similar in:
#3406 to populate the infobar with crate/playlist informations and I did not have any problems or crashes so far.

yeah i'm guessing that having an internal queue that handles the requests of reads should be there... note this is becoming even more shitty coz then the list passed around needs to be copied (so far passing around references). but that still does not solve the main issue FolderTreeModel::hasChildren can be blocked... and this should not be ever possible...

vigsterkr · 2021-02-10T10:43:35Z

as a quick has i've tried moving every IO logic into a separate worker thread that works on a queue to process all the IO operations and then fill out the model. same brokenness with the UI: spinball on mac, and UI blocks... i really reckon that even though the IO should really move into a background thread, there is something wrong with the View/Model design as now the only thing that can cause this behaviour is when in the worker thread i call TreeItemModel::insertTreeItemRows(folders, 0, parent) 🤷

any ideas is appreciated.... note even though this current patch is apparently not following Qt's standards are actually working as expected compared to the solution that Qt suggests (signals and slots and all that event loop magic)

uklotzde · 2021-02-10T11:35:10Z

If the current design of TreeItemModel doesn't fit it could be extended. Maybe a boolean return value for hasChildren() is not sufficient to cover all use cases and it also needs to account for maybe if the result is not immediately available.

vigsterkr · 2021-02-10T13:30:52Z

apparently indirection of indirection should solve this problem: http://blog.hostilefork.com/qt-model-view-different-threads/
meaning everything needs to be signaled... although since now i've moved everything out of BrowseFeature into FolderItemModel, i guess that indirection i can eliminate...

vigsterkr · 2021-02-10T16:43:41Z

so, i'm hoping that this is the Qt standard solution for the problem i was trying to fix. unfortunately this solution results in an as bad as experience that this PR was intended to solve. i'm really not sure why is the UI thread gets blocked when a FolderTreeModel::addChildren slot is being called and consequently the insertion would happen...

i have 2 guesses but would be super interested to hear about anybody else's opinion:

i'm trying to add too big a list at a time to the TreeItemModel and basically while that insertion is happening (event loop picked up the call of this function) the function itself takes a long time to run....? note i'm experiencing massive UI blocking (10+ seconds) when i'm trying to add 782 items at once. should the insertion happen in batches? or is it maybe that we should use the fetchMore() and canFetchMore() API
the other UI problem with the sidebar widget causes this...?

src/library/browse/foldertreemodel.cpp

src/library/browse/foldertreemodel.h

uklotzde · 2021-02-15T12:54:47Z

src/library/browse/foldertreemodel.cpp

+            &FolderTreeModel::newChildren,
+            this,
+            &FolderTreeModel::addChildren,
+            Qt::QueuedConnection);


Explicit queued connections should only be used if there is a reason, i.e. to prevent recursive signal/slot cascades within the same thread. Qt implicitly uses queued connections between threads.

imo this is a good enough reason: https://stackoverflow.com/a/1145628

The answer is wrong. This connection is implicitly queued.

cool man then just propose a change...? 🙃
i'm sorry man this is a bit ridicolous how you are handling an open source project's reviewing... could be more welcoming... this is not really encouraging new contributors to the project.... just my 2 cents...

and note this is in retrospect with our whole conversations in this PR...

Hi @vigsterkr, Sorry you had a bad experience in this pull request. Mixxx is a very large, very complex, very old codebase, and the developers here have a lot of experience with it. There is also a lot to do on this project and it's not always possible to give full explanations or help new contributors work through every change. We do appreciate your input, as shown by the large amount of communication on this pull request.

I would encourage you to stick with contributing, with a smaller change that focuses on just one small fix rather than a whole group of changes. Then you can get a better feel for how Mixxx is put together and how we do pull requests.

vigsterkr · 2022-04-02T08:26:12Z

@JoergAtGithub took waaaaaaaaaaaaaaaaay longer than expected, but finally done.

DO NOT MERGE as this fully blocks the thread UI upon insertion of children

the thread should die

Qt 5.14.0 already defines itself std::hash<QString>

resolve issue with const signal

this way one still needs to refresh the subtree manually...

acolombier · 2024-03-22T17:05:00Z

What happened with this PR? I can see there is still a few TODO and FIXME that feels like they were part of that on going work, does that mean that PR still need some work? If yes, shall we remove need review @JoergAtGithub ?

JoergAtGithub · 2024-03-22T19:29:57Z

As far as I can see Viktor didn't received any feedback to his last commits and stopped working on this PR. @vigsterkr are you still interested to finish this PR and get a detailed code review therefore?.

vigsterkr · 2024-03-25T05:19:38Z

patiently waiting for the review still

JoergAtGithub · 2024-03-25T07:23:09Z

patiently waiting for the review still

Great, could you respond to the TODO and FIXME question by Antoine above. Do you think this needs to be done before merging this PR?

daschuer

Thank you for picking this up. The review slipped through because @uklotzde has left the project, sorry. Let's continue.

daschuer · 2024-03-25T07:37:12Z

src/library/browse/browsefeature.cpp

-                      "your library. Would you like to rescan now?"));
-    QPushButton* scanButton = msgBox.addButton(
-        tr("Scan"), QMessageBox::AcceptRole);
+    msgBox.setText(


This PR is hard to review because all the minor code style changes. In general, mass reformating is not permitted in Mixxx. Please make sure you use pre-commit, which will take care of formating the touched lines only. Configure your editor to not reformat untouched files.
There is no need to remove the changes here though.

daschuer · 2024-03-25T08:00:37Z

src/library/browse/browsefeature.cpp

+        auto title = m_childModel.data(index, Qt::DisplayRole);
+        m_childModel.setData(index, QString("loading"));
+        m_childModel.triggerRepaint(index);
+        */


What is the status here?

daschuer · 2024-03-25T08:25:42Z

src/library/browse/foldertreemodel.cpp

+            &FolderTreeModel::newChildren,
+            this,
+            &FolderTreeModel::addChildren,
+            Qt::QueuedConnection);


Strange. I have no Idea how these issue are related.

The default Qt::AutoConnection should do the right thing here. Hover explicit Qt::QueuedConnection adds a bit of documentation. Normally it would be better to use Auto, but if it is a workaround of a Qt issue please add a comment.
In addition you may add DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); into FolderTreeModel::addChildren

daschuer · 2024-03-25T08:29:00Z

src/library/browse/foldertreemodel.cpp

+                if (!folders->isEmpty()) {
+                    sync.waitForFinished();
+                    m_cacheLock.unlock();
+                    emit newChildren(parent, folders);


It is not clear from the name if it is an imperative slot or a signal. Can you rename it to something more signal like?
For example newChildFoldersFound() or such?

daschuer · 2024-03-25T08:35:38Z

src/library/browse/foldertreemodel.cpp

+    connect(this,
+            &FolderTreeModel::newChildren,
+            this,
+            &FolderTreeModel::addChildren,
+            Qt::QueuedConnection);


The connect called form the "Thread (pooled)"! This means that the new object is has probably not the main thread affinity as desired. Please assert your assumptions using DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
It can be adjusted by moveToThread(thread);

daschuer · 2024-03-25T08:47:44Z

src/library/browse/foldertreemodel.cpp

+    connect(this,
+            &FolderTreeModel::hasSubDirectory,
+            this,
+            &FolderTreeModel::onHasSubDirectory);


Can this become a direct call?

daschuer · 2024-03-25T08:51:13Z

src/library/browse/foldertreemodel.cpp

+    if (m_directoryCache.count(str)) {
+        if (!QFileInfo::exists(str)) {
+            m_cacheLock.lockForWrite();
+            // TODO: when m_directoryCache is a prefix tree


daschuer · 2024-03-25T08:52:43Z

src/library/browse/foldertreemodel.h

+    QThreadPool m_pool;
+    mutable FolderQueue m_folderQueue;
+    mutable std::mutex m_queueLock;
+    std::atomic<bool> m_isRunning;


This looks more like a query variable for something else. How about invert it and call it m_stopFolderThread

daschuer · 2024-03-25T09:34:58Z

src/library/browse/foldertreemodel.h

+    mutable FolderQueue m_folderQueue;
+    mutable std::mutex m_queueLock;
+    std::atomic<bool> m_isRunning;
+    QFuture<void> m_folderProcess;


QFuture handles threads, not processes. How about m_folderFuture or m_folderThread

daschuer · 2024-03-25T09:37:10Z

src/library/browse/foldertreemodel.cpp

+                    m_cacheLock.unlock();
+                }
+            }
+            QThread::msleep(100);


Busy looking at the m_folderQueue is wasting CPU. Can we replace it with a single Future that is started for every folder added? This was we would get rid of the m_folderQueue altogether. This would also solve the issue with the signal from the Future. It would be just replaced with a QFuture::result()

acolombier · 2024-10-27T13:23:17Z

Removing needs review for now as this appears to have gone cold. Please let us know if you need any help/review to complete that.

github-actions bot added the library label Feb 9, 2021

uklotzde previously requested changes Feb 9, 2021

View reviewed changes

src/library/browse/browsefeature.cpp Outdated Show resolved Hide resolved

vigsterkr force-pushed the fix_io_browsefeature branch from e265836 to de8f99d Compare February 10, 2021 17:10

vigsterkr commented Feb 10, 2021

View reviewed changes

src/library/browse/foldertreemodel.cpp Show resolved Hide resolved

vigsterkr commented Feb 10, 2021

View reviewed changes

src/library/browse/foldertreemodel.cpp Outdated Show resolved Hide resolved

Be-ing changed the title ~~add threading for IO~~ library BrowseFeature: add threading for IO Feb 10, 2021

vigsterkr force-pushed the fix_io_browsefeature branch from de8f99d to c602db4 Compare February 15, 2021 10:12

vigsterkr commented Feb 15, 2021

View reviewed changes

src/library/browse/foldertreemodel.h Outdated Show resolved Hide resolved

uklotzde reviewed Feb 15, 2021

View reviewed changes

vigsterkr force-pushed the fix_io_browsefeature branch from 999fc8e to 847dfa1 Compare April 2, 2022 06:51

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Apr 3, 2022

vigsterkr force-pushed the fix_io_browsefeature branch from e1538e2 to 46d2fbb Compare September 19, 2022 11:49

vigsterkr added 14 commits October 12, 2022 17:52

add threading for IO

16f9fdf

Qt (c) ready solution of the previous fix

4064150

DO NOT MERGE as this fully blocks the thread UI upon insertion of children

make QString std::hashable

319332f

fix FolderTreeModel destructor

ed934e5

the thread should die

all IO operations should be done in background thread

02b6f46

fix formatting

f74090f

only define std::hash<QString> when necessary

24fbdc7

Qt 5.14.0 already defines itself std::hash<QString>

fix windows compilation by moving windows.h include first

f0dc6f5

resolve issue with const signal

fix old Qt connect

6226825

add initial support for filesystem watch

1341329

this way one still needs to refresh the subtree manually...

fix after rebase

925fd30

fix spelling

2c62eaa

fix codestyle

4dd0890

handle return value of QtConcurrent::run

47ada1f

vigsterkr force-pushed the fix_io_browsefeature branch from 46d2fbb to 47ada1f Compare October 12, 2022 15:53

uklotzde mentioned this pull request Apr 5, 2023

Browsing large folders makes track table unusable during scan due to repeated scrolling to the top #11445

Open

JoergAtGithub added needs review performance labels Jul 15, 2023

daschuer requested changes Mar 25, 2024

View reviewed changes

JoergAtGithub removed the request for review from uklotzde March 25, 2024 19:29

acolombier removed the needs review label Oct 27, 2024

library BrowseFeature: add threading for IO #3623

Are you sure you want to change the base?

library BrowseFeature: add threading for IO #3623

Conversation

vigsterkr commented Feb 9, 2021 • edited Loading

vigsterkr commented Feb 9, 2021

daschuer commented Feb 9, 2021

vigsterkr commented Feb 9, 2021

vigsterkr commented Feb 9, 2021

uklotzde commented Feb 9, 2021

daschuer commented Feb 9, 2021

vigsterkr commented Feb 9, 2021

Be-ing commented Feb 9, 2021

vigsterkr commented Feb 9, 2021

Be-ing commented Feb 9, 2021

vigsterkr commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

poelzi commented Feb 10, 2021

poelzi commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

uklotzde commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

vigsterkr commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigsterkr commented Apr 2, 2022

acolombier commented Mar 22, 2024 • edited Loading

JoergAtGithub commented Mar 22, 2024

vigsterkr commented Mar 25, 2024 via email • edited by daschuer Loading

JoergAtGithub commented Mar 25, 2024

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier commented Oct 27, 2024

vigsterkr commented Feb 9, 2021 •

edited

Loading

acolombier commented Mar 22, 2024 •

edited

Loading

vigsterkr commented Mar 25, 2024 via email •

edited by daschuer

Loading