Fixes slow update in field calculator #8177

elpaso · 2018-10-12T20:23:32Z

The diff is misleading, it's just a single
change in the logic: block the vector signals ...
... and emit dataChanged at
the end.

I don't really like the idea of blocking
signals (the edit buffer basically)
and I'm a bit worried of side effects,
but I can't see any other solution.

The root of the issue here is that
for each changed field/row an attribute
valueChanged signal is emitted, and the
QgsVectorLayerCache::featureAtId
loads the feature again.

Pocessing time for the test layer in the
issue tracker went down from ~20 minutes
to less than 3 seconds.

by blocking the vector signals ... ... and emitting dataChanged at the end. I'm a bit worried of side effects, but I can't see any other solution. The root of the issue here is that for each changed field/row an attribute valueChanged signal is emitted, and the QgsVectorLayerCache::featureAtId loads the feature again.

elpaso · 2018-10-12T20:24:57Z

@m-kuhn I'd appreciate a slice of your brain time here.

nyalldawson · 2018-10-12T23:58:43Z

Just a thought -- both the attribute table dialogs and field calculator live in app, so couldn't we directly block just the attribute table dialog updates from field calculator? That'd avoid any unwanted side effects which would come from the global signal blocking (which I'm also concerned about)

nyalldawson · 2018-10-12T23:59:04Z

Thanks for tackling this by the way!

much better approach ...

elpaso · 2018-10-13T08:23:07Z

@nyalldawson I slept on it and came to a similar conclusion: blocking just the layer cache signals, from the attribute table scope: simpler, safer, better (TM)

nyalldawson · 2018-10-13T09:23:22Z

But what about the situation where a user has an attribute table open, and then uses the field calculator from the main window toolbar? Or layer properties window?

elpaso · 2018-10-13T09:40:02Z

I don't understand your proposal: QgsFieldCalculator is instanciated inside the field calculator, it does not live in app.

Btw, you are right and my solution only works for the attribute table that directly opened the field calculator. It does not solve the issue for other attr tables that might have been opened separately on the same vector layer or for field calculator opened from the properties dialog.

nyalldawson · 2018-10-13T09:46:52Z

What I mean is that they are both app library classes. So we could potentially call from the field calculator something like QgisApp::instance()->blockTableUpdates, which then blocks the updates of all open table dialogs.

Fixes qgis#20094 - Extracting z-coordinates takes ages and makes the system unresponsive

elpaso · 2018-10-13T22:47:45Z

Actually, the problem was not strictly related to the field calculator: rollbacks of massive changes triggered the bug as well, I've fixed that too.

nyalldawson · 2018-10-14T00:19:25Z

src/app/qgisapp.h

+     *
+     * \since QGIS 3.4
+     */
+    void blockAttributeTableUpdates( const QgsVectorLayer *layer );


we could use one signal and one slot here with a bool blocked argument

Yes, I first implemented it like that. But I was in doubt, because in general we don't like bool switches, do we?
But you solved my doubt, so I'm fine with the bool now. (I've always thought that blockSignals(bool) was not very consistent with the rest of Qt API).

nyalldawson · 2018-10-14T00:21:04Z

src/app/qgsattributetabledialog.cpp

+    if ( layer == mLayer )
+      this->blockCacheUpdateSignals( false );
+  } );
+  // Massive rollbacks can also freeze the GUI due to the feature cache signals


We have the same issue with attribute addition/deletion too, don't we?

I you mean removing the columns as a whole, apparently not, at least I didn't observe the issue when removing the z field, I'll double check it.

Confirmed: with my PR removing a colum is FAST.

nyalldawson · 2018-10-14T00:21:52Z

One thing I'm not sure of here -- do we need to do a single "update cache/model" call after the calculator finishes?

elpaso · 2018-10-14T09:01:10Z

One thing I'm not sure of here -- do we need to do a single "update cache/model" call after the calculator finishes?

To be honest I wasn't sure either, here we have a babel of signals/slots firing. It's a spagetti-signal-mess!
Btw, I came to the conclusion that it does work as expected, but even if it didn't we cannot really emit a number of signals for each modified row!

It wouldn't be much much easier if we could have just three signals?

a signal for a single row change (geometry included)
a signal (and buffer implementations) for bulk changes, instead of emitting a signal for every row we are going to change (60K rows -> 60K signals emitted! I'm optimisitc here, we probably have more than one signal emitted for every row)
a generic signal for dataChanged

luipir · 2018-10-14T13:04:40Z

It wouldn't be much much easier if we could have just three signals?

Are you referring only to qgis implemented signals? or also to all emitted by qt infrastructure?

elpaso · 2018-10-14T14:06:45Z

@luipir nevermind: I know that's not going to happen until QGIS 7

I was talking about the various signals emitted by QgsVectorLayer when editing.

nyalldawson · 2018-10-14T22:37:49Z

: I know that's not going to happen until QGIS 7

First thing on the api break list for qgis 7 -- "rm -rf src/gui/attributetable" ;)

But in all seriousness -- attribute table is crying out for a complete, ground up rework.

m-kuhn · 2018-10-15T07:15:18Z

IIRC the idea was to use the beginEditCommand and endEditCommand for bulk updates. If those are properly emitted by a component (e.g. field calculator) individual row update signals should be ignored and a single refresh done only once the end.

m-kuhn · 2018-10-15T08:41:04Z

@elpaso did you read my comment?

elpaso · 2018-10-15T10:15:38Z

Yes, I'll have a look to that signals, just to be sure: are you suggesting that the attribute table should listen to those signals and block feature cache signals instead of the solution I've implemented in this PR?

Something like:

connect( mLayer, &QgsVectorLayer::editCommandStarted, [ = ]( const QString *)
  {
      this->blockCacheUpdateSignals( true );
  } );
connect( mLayer, &QgsVectorLayer::editCommandEnded [ = ]( const QString *)
  {
      this->blockCacheUpdateSignals( false );
  } );

m-kuhn · 2018-10-15T10:23:47Z

I would avoid to block signals if somehow possible.

The original idea was (IIRC) to set a variable inEditCommand (or similar) and then buffer all incoming change notifications but defer updating the attribute table to the end. This way a full cache invalidation and potentially expensive reload can be avoided for partial updates. I am not sure what the current problem is: either the approach explained here is not implemented properly and the buffering somehow fails, or if it is actually the signal-and-buffering code and not the attribute table update code that is too expensive and makes the calculation slow.

An alternative would be to selectively disconnect/reconnect the signals inside the attribute table instead of completely avoiding the emission of signals if it's actually the connections which are causing the troubles.

m-kuhn · 2018-10-15T10:24:28Z

src/app/qgsattributetabledialog.cpp

+  connect( QgisApp::instance(), &QgisApp::attributeTableUpdateBlocked, [ = ]( const QgsVectorLayer * layer, const bool blocked )
+  {
+    if ( layer == mLayer )
+      this->blockCacheUpdateSignals( blocked );


It's not python, we don't need this.

Thanks for reminding me that it's not Python but C++, I normally use this-> to get autocompletion and I forget to remove it.

ctrl+space is your friend

elpaso · 2018-10-15T10:33:43Z

@m-kuhn the original issue was that the field calculator loops through all records and change attribute values, but the feature cache listens to that signal (among others) and called QgsVectorLayerCache::featureAtId which called mLayer->getFeatures etc. etc.

m-kuhn · 2018-10-15T10:46:30Z

And in the etc. etc. part it will actually send requests to the provider? The code was likely written under the assumption that the feature should already be cached in the edit buffer at this time.

What are your thoughts on

using the edit command as indicator for batch updates?
disconnecting connections instead of blocking signals?
not using this ;)

…alculator-slowness" This reverts commit 096b4ce, reversing changes made to 87e0f69.

gioman · 2018-10-18T13:17:02Z

About this fix (that is MUCH appreciated): when clicking "ok" in the field calculator as also when saving edits the table is refreshed a couple of times, is this expected?

elpaso · 2018-10-18T13:39:56Z

@gioman I assume you are testing #8200 .
Yes, it's expected (and annoying), because it is refreshed the first time when a field is created and then again when the computation has finished (so, we are talking about at least two times, but there may be other signals that are firing up and triggering a refresh, nobody knows at this point what's happening in that ugly mess of code).

But, the important thing to check is that it does not hang and it completes in a reasonable time (in my test 60k rows with a default cache size of 10k completeted in ~8 seconds).

Note that #8200 is a few seconds slower than #8177 because I'm not blocking all signals but just deferring them to a later stage (and prevent cache reloads).

gioman · 2018-10-18T14:18:12Z

Thanks for the explanation!

m-kuhn · 2018-10-18T14:24:54Z

@elpaso I believe there are no notifications around that could be used for one single refresh even if someone came forward with the most beautiful mess of code, but I'm probably wrong.

luipir · 2018-10-18T14:55:16Z

I can't find a QT way to do debouncing as in JS https://davidwalsh.name/javascript-debounce-function, this could (hopefully) avoid to retrigger events in short intervals. Probably with QEvetFilter depending on running context?

elpaso added the Bugfix label Oct 12, 2018

Block signals in the cache only

afc873d

much better approach ...

elpaso added this to the 3.4.0 milestone Oct 13, 2018

elpaso added 2 commits October 13, 2018 15:40

Forward to app blocking/unblocking of attr table updates

9d82df5

Fixes qgis#20094 - Extracting z-coordinates takes ages and makes the system unresponsive

Fix rollback on large tables when attr table is opened

07b3128

Remove unused forward declaration

20e8592

nyalldawson reviewed Oct 14, 2018

View reviewed changes

blockAttributeTableUpdates: single signal with bool

c8ea3a7

elpaso closed this Oct 14, 2018

elpaso reopened this Oct 14, 2018

elpaso merged commit 096b4ce into qgis:master Oct 15, 2018

elpaso deleted the bugfix-20094-field-calculator-slowness branch October 15, 2018 08:32

m-kuhn reviewed Oct 15, 2018

View reviewed changes

elpaso added a commit to elpaso/QGIS that referenced this pull request Oct 16, 2018

Revert "Merge pull request qgis#8177 from elpaso/bugfix-20094-field-c…

0e50b3b

…alculator-slowness" This reverts commit 096b4ce, reversing changes made to 87e0f69.

This was referenced May 25, 2019

Very Slow Table Operations while the Table is Open #27830

Closed

Extracting z-coordinates takes ages and makes the system unresponsive #27916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes slow update in field calculator #8177

Fixes slow update in field calculator #8177

elpaso commented Oct 12, 2018

elpaso commented Oct 12, 2018

nyalldawson commented Oct 12, 2018

nyalldawson commented Oct 12, 2018

elpaso commented Oct 13, 2018

nyalldawson commented Oct 13, 2018

elpaso commented Oct 13, 2018

nyalldawson commented Oct 13, 2018

elpaso commented Oct 13, 2018

nyalldawson Oct 14, 2018

elpaso Oct 14, 2018

nyalldawson Oct 14, 2018

elpaso Oct 14, 2018

elpaso Oct 14, 2018

nyalldawson commented Oct 14, 2018

elpaso commented Oct 14, 2018 •

edited

Loading

luipir commented Oct 14, 2018

elpaso commented Oct 14, 2018 •

edited

Loading

nyalldawson commented Oct 14, 2018

m-kuhn commented Oct 15, 2018

m-kuhn commented Oct 15, 2018

elpaso commented Oct 15, 2018 •

edited

Loading

m-kuhn commented Oct 15, 2018

m-kuhn Oct 15, 2018

elpaso Oct 15, 2018

m-kuhn Oct 15, 2018

elpaso commented Oct 15, 2018

m-kuhn commented Oct 15, 2018

gioman commented Oct 18, 2018

elpaso commented Oct 18, 2018 •

edited

Loading

gioman commented Oct 18, 2018

m-kuhn commented Oct 18, 2018

luipir commented Oct 18, 2018

Fixes slow update in field calculator #8177

Fixes slow update in field calculator #8177

Conversation

elpaso commented Oct 12, 2018

elpaso commented Oct 12, 2018

nyalldawson commented Oct 12, 2018

nyalldawson commented Oct 12, 2018

elpaso commented Oct 13, 2018

nyalldawson commented Oct 13, 2018

elpaso commented Oct 13, 2018

nyalldawson commented Oct 13, 2018

elpaso commented Oct 13, 2018

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

nyalldawson commented Oct 14, 2018

elpaso commented Oct 14, 2018 • edited Loading

luipir commented Oct 14, 2018

elpaso commented Oct 14, 2018 • edited Loading

nyalldawson commented Oct 14, 2018

m-kuhn commented Oct 15, 2018

m-kuhn commented Oct 15, 2018

elpaso commented Oct 15, 2018 • edited Loading

m-kuhn commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elpaso commented Oct 15, 2018

m-kuhn commented Oct 15, 2018

gioman commented Oct 18, 2018

elpaso commented Oct 18, 2018 • edited Loading

gioman commented Oct 18, 2018

m-kuhn commented Oct 18, 2018

luipir commented Oct 18, 2018

elpaso commented Oct 14, 2018 •

edited

Loading

elpaso commented Oct 14, 2018 •

edited

Loading

elpaso commented Oct 15, 2018 •

edited

Loading

elpaso commented Oct 18, 2018 •

edited

Loading