Skip to content

Commit c77ff3f

Browse files
committed
qmlls: remove unused indexing code
It seems that we have some code in qmlls that runs an "indexing" thread in the background to load non-opened files. It is not tested and never used : the only commit using the indexing functionality is 2717985 but never sets `m_status` to `Index`, so that the indexing is almost never run. There are some code paths, for example when a DidChangeWorkspaceFoldersNotification is received by the server, where the indexing is triggered despite it being disabled. Enabling the indexing leads to a ASAN crash, probably because of some missing synchronization on some shared data like: * QQmlJSImporter and QQmlJSResourceFileMapper: both needed to construct the Dom while loading a QML file, * QQmlJScope::Ptr: the same ptr might be lazyloaded by two different threads at the same time) Remove the indexing funtionality from the code, and pick the change back to avoid memory errors on DidChangeWorkspaceFoldersNotifications. This could be related to one of the qmlls crashes described in the comments of QTBUG-133586. Task-number: QTBUG-133586 Pick-to: 6.8 6.9 6.10 Change-Id: I547cf11150dc25b04208a83bf5e123e7ee5b0482 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
1 parent 0c705ac commit c77ff3f

File tree

4 files changed

+8
-212
lines changed

4 files changed

+8
-212
lines changed

src/qmlls/qqmlcodemodel.cpp

Lines changed: 7 additions & 189 deletions
Original file line numberDiff line numberDiff line change
@@ -69,21 +69,20 @@ synchronization here.
6969
\section2 Parallelism/Theading
7070
Most operations are not parallel and usually take place in the main thread (but are still thread
7171
safe).
72-
There are two main task that are executed in parallel: Indexing, and OpenDocumentUpdate.
73-
Indexing is meant to keep the global view up to date.
72+
There is one task that is executed in parallel: OpenDocumentUpdate.
7473
OpenDocumentUpdate keeps the snapshots of the open documents up to date.
7574
7675
There is always a tension between being responsive, using all threads available, and avoid to hog
7776
too many resources. One can choose different parallelization strategies, we went with a flexiable
7877
approach.
79-
We have (private) functions that execute part of the work: indexSome() and openUpdateSome(). These
78+
We have (private) functions that execute part of the work: openUpdateSome(). These
8079
do all locking needed, get some work, do it without locks, and at the end update the state of the
8180
code model. If there is more work, then they return true. Thus while (xxxSome()); works until there
8281
is no work left.
8382
84-
addDirectoriesToIndex(), the internal addDirectory() and addOpenToUpdate() add more work to do.
83+
The internal addOpenToUpdate() add more work to do.
8584
86-
indexNeedsUpdate() and openNeedUpdate(), check if there is work to do, and if yes ensure that a
85+
openNeedUpdate() checks if there is work to do, and if yes ensure that a
8786
worker thread (or more) that work on it exist.
8887
*/
8988

@@ -119,7 +118,7 @@ QQmlCodeModel::~QQmlCodeModel()
119118
QMutexLocker l(&m_mutex);
120119
m_state = State::Stopping;
121120
m_openDocumentsToUpdate.clear();
122-
shouldWait = m_nIndexInProgress != 0 || m_nUpdateInProgress != 0;
121+
shouldWait = m_nUpdateInProgress != 0;
123122
}
124123
if (!shouldWait)
125124
break;
@@ -132,143 +131,8 @@ OpenDocumentSnapshot QQmlCodeModel::snapshotByUrl(const QByteArray &url)
132131
return openDocumentByUrl(url).snapshot;
133132
}
134133

135-
int QQmlCodeModel::indexEvalProgress() const
136-
{
137-
Q_ASSERT(!m_mutex.tryLock()); // should be called while locked
138-
const int dirCost = 10;
139-
int costToDo = 1;
140-
for (const ToIndex &el : std::as_const(m_toIndex))
141-
costToDo += dirCost * el.leftDepth;
142-
costToDo += m_indexInProgressCost;
143-
return m_indexDoneCost * 100 / (costToDo + m_indexDoneCost);
144-
}
145-
146-
void QQmlCodeModel::indexStart()
147-
{
148-
Q_ASSERT(!m_mutex.tryLock()); // should be called while locked
149-
qCDebug(codeModelLog) << "indexStart";
150-
}
151-
152-
void QQmlCodeModel::indexEnd()
153-
{
154-
Q_ASSERT(!m_mutex.tryLock()); // should be called while locked
155-
qCDebug(codeModelLog) << "indexEnd";
156-
m_lastIndexProgress = 0;
157-
m_nIndexInProgress = 0;
158-
m_toIndex.clear();
159-
m_indexInProgressCost = 0;
160-
m_indexDoneCost = 0;
161-
}
162-
163-
void QQmlCodeModel::indexSendProgress(int progress)
164-
{
165-
if (progress <= m_lastIndexProgress)
166-
return;
167-
m_lastIndexProgress = progress;
168-
// ### actually send progress
169-
}
170-
171-
bool QQmlCodeModel::indexCancelled()
172-
{
173-
QMutexLocker l(&m_mutex);
174-
if (m_state == State::Stopping)
175-
return true;
176-
return false;
177-
}
178-
179-
void QQmlCodeModel::indexDirectory(const QString &path, int depthLeft)
180-
{
181-
if (indexCancelled())
182-
return;
183-
QDir dir(path);
184-
if (depthLeft > 1) {
185-
const QStringList dirs =
186-
dir.entryList(QDir::Dirs | QDir::NoDotAndDotDot | QDir::NoSymLinks);
187-
for (const QString &child : dirs)
188-
addDirectory(dir.filePath(child), --depthLeft);
189-
}
190-
const QStringList qmljs =
191-
dir.entryList(QStringList({ u"*.qml"_s, u"*.js"_s, u"*.mjs"_s }), QDir::Files);
192-
int progress = 0;
193-
{
194-
QMutexLocker l(&m_mutex);
195-
m_indexInProgressCost += qmljs.size();
196-
progress = indexEvalProgress();
197-
}
198-
indexSendProgress(progress);
199-
if (qmljs.isEmpty())
200-
return;
201-
DomItem newCurrent = m_currentEnv.makeCopy(DomItem::CopyOption::EnvConnected).item();
202-
for (const QString &file : qmljs) {
203-
if (indexCancelled())
204-
return;
205-
QString fPath = dir.filePath(file);
206-
auto newCurrentPtr = newCurrent.ownerAs<DomEnvironment>();
207-
FileToLoad fileToLoad = FileToLoad::fromFileSystem(newCurrentPtr, fPath);
208-
if (!fileToLoad.canonicalPath().isEmpty()) {
209-
newCurrentPtr->loadBuiltins();
210-
newCurrentPtr->loadFile(fileToLoad, [](Path, const DomItem &, const DomItem &) {});
211-
newCurrentPtr->loadPendingDependencies();
212-
newCurrent.commitToBase(m_validEnv.ownerAs<DomEnvironment>());
213-
}
214-
{
215-
QMutexLocker l(&m_mutex);
216-
++m_indexDoneCost;
217-
--m_indexInProgressCost;
218-
progress = indexEvalProgress();
219-
}
220-
indexSendProgress(progress);
221-
}
222-
}
223-
224-
void QQmlCodeModel::addDirectoriesToIndex(const QStringList &paths, QLanguageServer *server)
225-
{
226-
Q_UNUSED(server);
227-
// ### create progress, &scan in a separate instance
228-
const int maxDepth = 5;
229-
for (const auto &path : paths)
230-
addDirectory(path, maxDepth);
231-
indexNeedsUpdate();
232-
}
233-
234-
void QQmlCodeModel::addDirectory(const QString &path, int depthLeft)
235-
{
236-
if (depthLeft < 1)
237-
return;
238-
{
239-
QMutexLocker l(&m_mutex);
240-
for (auto it = m_toIndex.begin(); it != m_toIndex.end();) {
241-
if (it->path.startsWith(path)) {
242-
if (it->path.size() == path.size())
243-
return;
244-
if (it->path.at(path.size()) == u'/') {
245-
it = m_toIndex.erase(it);
246-
continue;
247-
}
248-
} else if (path.startsWith(it->path) && path.at(it->path.size()) == u'/')
249-
return;
250-
++it;
251-
}
252-
m_toIndex.append({ path, depthLeft });
253-
}
254-
}
255-
256134
void QQmlCodeModel::removeDirectory(const QString &path)
257135
{
258-
{
259-
QMutexLocker l(&m_mutex);
260-
auto toRemove = [path](const QString &p) {
261-
return p.startsWith(path) && (p.size() == path.size() || p.at(path.size()) == u'/');
262-
};
263-
auto it = m_toIndex.begin();
264-
auto end = m_toIndex.end();
265-
while (it != end) {
266-
if (toRemove(it->path))
267-
it = m_toIndex.erase(it);
268-
else
269-
++it;
270-
}
271-
}
272136
if (auto validEnvPtr = m_validEnv.ownerAs<DomEnvironment>())
273137
validEnvPtr->removePath(path);
274138
if (auto currentEnvPtr = m_currentEnv.ownerAs<DomEnvironment>())
@@ -332,59 +196,13 @@ const RegisteredSemanticTokens &QQmlCodeModel::registeredTokens() const
332196
return m_tokens;
333197
}
334198

335-
void QQmlCodeModel::indexNeedsUpdate()
336-
{
337-
const int maxIndexThreads = 1;
338-
{
339-
QMutexLocker l(&m_mutex);
340-
if (m_toIndex.isEmpty() || m_nIndexInProgress >= maxIndexThreads)
341-
return;
342-
if (++m_nIndexInProgress == 1)
343-
indexStart();
344-
}
345-
QThreadPool::globalInstance()->start([this]() {
346-
while (indexSome()) { }
347-
});
348-
}
349-
350-
bool QQmlCodeModel::indexSome()
351-
{
352-
qCDebug(codeModelLog) << "indexSome";
353-
ToIndex toIndex;
354-
{
355-
QMutexLocker l(&m_mutex);
356-
if (m_toIndex.isEmpty()) {
357-
if (--m_nIndexInProgress == 0)
358-
indexEnd();
359-
return false;
360-
}
361-
toIndex = m_toIndex.last();
362-
m_toIndex.removeLast();
363-
}
364-
bool hasMore = false;
365-
{
366-
auto guard = qScopeGuard([this, &hasMore]() {
367-
QMutexLocker l(&m_mutex);
368-
if (m_toIndex.isEmpty()) {
369-
if (--m_nIndexInProgress == 0)
370-
indexEnd();
371-
hasMore = false;
372-
} else {
373-
hasMore = true;
374-
}
375-
});
376-
indexDirectory(toIndex.path, toIndex.leftDepth);
377-
}
378-
return hasMore;
379-
}
380-
381199
void QQmlCodeModel::openNeedUpdate()
382200
{
383201
qCDebug(codeModelLog) << "openNeedUpdate";
384-
const int maxIndexThreads = 1;
202+
const int maxThreads = 1;
385203
{
386204
QMutexLocker l(&m_mutex);
387-
if (m_openDocumentsToUpdate.isEmpty() || m_nUpdateInProgress >= maxIndexThreads)
205+
if (m_openDocumentsToUpdate.isEmpty() || m_nUpdateInProgress >= maxThreads)
388206
return;
389207
if (++m_nUpdateInProgress == 1)
390208
openUpdateStart();

src/qmlls/qqmlcodemodel_p.h

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,6 @@ class QQmlCodeModel : public QObject
112112
OpenDocument openDocumentByUrl(const QByteArray &url);
113113

114114
void openNeedUpdate();
115-
void indexNeedsUpdate();
116-
void addDirectoriesToIndex(const QStringList &paths, QLanguageServer *server);
117115
void addOpenToUpdate(const QByteArray &);
118116
void removeDirectory(const QString &path);
119117
// void updateDocument(const OpenDocument &doc);
@@ -149,13 +147,6 @@ class QQmlCodeModel : public QObject
149147
void documentationRootPathChanged(const QString &path);
150148

151149
private:
152-
void indexDirectory(const QString &path, int depthLeft);
153-
int indexEvalProgress() const; // to be called in the mutex
154-
void indexStart(); // to be called in the mutex
155-
void indexEnd(); // to be called in the mutex
156-
void indexSendProgress(int progress);
157-
bool indexCancelled();
158-
bool indexSome();
159150
void addDirectory(const QString &path, int leftDepth);
160151
bool openUpdateSome();
161152
void openUpdateStart();
@@ -169,11 +160,6 @@ class QQmlCodeModel : public QObject
169160

170161
mutable QMutex m_mutex;
171162
State m_state = State::Running;
172-
int m_lastIndexProgress = 0;
173-
int m_nIndexInProgress = 0;
174-
QList<ToIndex> m_toIndex;
175-
int m_indexInProgressCost = 0;
176-
int m_indexDoneCost = 0;
177163
int m_nUpdateInProgress = 0;
178164
QStringList m_importPaths;
179165
QQmlJS::Dom::DomItem m_currentEnv;

src/qmlls/qworkspace.cpp

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ void WorkspaceHandlers::registerHandlers(QLanguageServer *server, QLanguageServe
1919
{
2020
QObject::connect(server->notifySignals(),
2121
&QLspNotifySignals::receivedDidChangeWorkspaceFoldersNotification, this,
22-
[server, this](const DidChangeWorkspaceFoldersParams &params) {
22+
[this](const DidChangeWorkspaceFoldersParams &params) {
2323
const WorkspaceFoldersChangeEvent &event = params.event;
2424

2525
const QList<WorkspaceFolder> &removed = event.removed;
@@ -32,14 +32,10 @@ void WorkspaceHandlers::registerHandlers(QLanguageServer *server, QLanguageServe
3232
m_codeModel->removeRootUrls(toRemove);
3333
const QList<WorkspaceFolder> &added = event.added;
3434
QList<QByteArray> toAdd;
35-
QStringList pathsToAdd;
3635
for (const WorkspaceFolder &folder : added) {
3736
toAdd.append(QQmlLSUtils::lspUriToQmlUrl(folder.uri));
38-
pathsToAdd.append(m_codeModel->url2Path(
39-
QQmlLSUtils::lspUriToQmlUrl(folder.uri)));
4037
}
4138
m_codeModel->addRootUrls(toAdd);
42-
m_codeModel->addDirectoriesToIndex(pathsToAdd, server);
4339
});
4440

4541
QObject::connect(server->notifySignals(),
@@ -165,8 +161,6 @@ void WorkspaceHandlers::clientInitialized(QLanguageServer *server)
165161
rootPaths.insert(workspaceUrl.toLocalFile());
166162
}
167163
}
168-
if (m_status == Status::Indexing)
169-
m_codeModel->addDirectoriesToIndex(QStringList(rootPaths.begin(), rootPaths.end()), server);
170164
}
171165

172166
QT_END_NAMESPACE

src/qmlls/qworkspace_p.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ class WorkspaceHandlers : public QLanguageServerModule
2424
{
2525
Q_OBJECT
2626
public:
27-
enum class Status { NoIndex, Indexing };
2827
WorkspaceHandlers(QmlLsp::QQmlCodeModel *codeModel) : m_codeModel(codeModel) { }
2928
QString name() const override;
3029
void registerHandlers(QLanguageServer *server, QLanguageServerProtocol *protocol) override;
@@ -35,7 +34,6 @@ public Q_SLOTS:
3534

3635
private:
3736
QmlLsp::QQmlCodeModel *m_codeModel = nullptr;
38-
Status m_status = Status::NoIndex;
3937
};
4038

4139
QT_END_NAMESPACE

0 commit comments

Comments
 (0)