Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In project with many category symbols in many layers, Layer Properties dialog takes very long time to open for vector layers #34942

Closed
SteveLowman opened this issue Mar 9, 2020 · 80 comments · Fixed by #35557 or #37431
Assignees
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Project Regression Something which used to work, but doesn't anymore

Comments

@SteveLowman
Copy link

SteveLowman commented Mar 9, 2020

Describe the bug

In QGIS 3.12 and beyond, in a project with many symbology categories in many vector layers, where the mathematical product of 'No. of Layers' * 'No. of categories' = a very high number, the Layer Properties dialog for vector layers is taking a very long time to open.

This does not seem to affect the Layer Properties dialog for raster layers, but it affects all of the vector layers. So far (this description edited 25th June 2020), the bug fixing discussion and process has found that the issue is due to the introduction of Selective Masking in QGIS 3.12. A partial fix has been implemented, involving the 'Edit Mask' checkbox, but there is still a substantial delay in the sample file, even with this box unchecked.

How to Reproduce

Dummy_Data.zip

  1. Open the dummy_project.qgz file which includes 64 duplicate layers, each with 235 categories (= 15040).
  2. In Layers Panel, double-click on a vector layer, or right-click and select 'Properties'.
  3. Record the time to open the Layer Properties dialog
    expected times:
    QGIS 3.10 < 2 seconds
    QGIS 3.12 37 seconds with Edit Mask unchecked
    QGIS 3.12 83 seconds with Edit Mask checked
    (comparison of Edit Mask checked and unchecked was on different machines, so it may be inaccurate)
  4. Open a project file that contains only a few vector layers with not many symbology categories.
  5. Repeat steps 2 and 3 for the smaller project file.
  6. Compare the times recorded to open the dialog in the smaller and larger project files.
  7. If the Layer Properties dialog takes much longer to open in the larger project than the smaller one in QGIS 3.12 (but no difference in QGIS 3.10), then you will have reproduced this bug.

QGIS and OS versions

QGIS version
3.12.0-București
QGIS code revision
cd14149
Compiled against Qt
5.11.2
Running against Qt
5.11.2
Compiled against GDAL/OGR
3.0.4
Running against GDAL/OGR
3.0.4
Compiled against GEOS
3.8.0-CAPI-1.13.1
Running against GEOS
3.8.0-CAPI-1.13.1
Compiled against SQLite
3.29.0
Running against SQLite
3.29.0
PostgreSQL Client Version
11.5
SpatiaLite Version
4.3.0
QWT Version
6.1.3
QScintilla2 Version
2.10.8
Compiled against PROJ
6.3.1
Running against PROJ
Rel. 6.3.1, February 10th, 2020
OS Version
Windows 10 (10.0)
Active python plugins
db_manager;
MetaSearch;
processing

Additional context

Before we were aware that the problem involved the number of symbology categories, we had been unable to create a functional test file.

A short-term fix was implemented, which involved the Edit Mask checkbox. Although this reduced the delay duration, it can still be very long.

This is a new bug in 3.12, and it seems to be due to the introduction of the Selective Masking feature.

I think it is probably a regression.

@SteveLowman SteveLowman added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Mar 9, 2020
@gioman
Copy link
Contributor

gioman commented Mar 9, 2020

@SteveLowman thanks for the report, this seems an hard one to troubleshoot/replicate.

A few questions:

  1. does the size of layers/tables matters (regarding how long the properties panel takes to open)?
  2. does the method used to load layers matter (drag and drop VS browser VS "add vector layer" ...)?
  3. does the type of the datasource matters (postgis vs shp vs gpkg, etc.)?

@gioman gioman added the Feedback Waiting on the submitter for answers label Mar 9, 2020
@gioman
Copy link
Contributor

gioman commented Mar 9, 2020

@SteveLowman looking at this comment

#34691 (comment)

I also wonder if it makes any difference using an existing project VS creating one from scratch

@SteveLowman
Copy link
Author

SteveLowman commented Mar 9, 2020

Answers to questions:

  1. I have found no correlation with the amount of geospatial/attribute data in the layers.
  2. I have found no correlation with the method used to add layers.
  3. I have found the same issue with geopackage layers and shapefiles
    shp_with_qml.zip (this one has a lot of empty shapefiles - the "TopoLine" ones have a categorised .qml);
    with gpkg layers, it is the same whether the styles are already saved as default in the gpkg
    geopck_with_default_styles.zip (the more new gpkg layers you add, the longer the delay will become), or whether they are applied as pasted styles and saved only with the project.
    I have not tested PostGIS layers.
  4. I have found no difference between previously created projects and newly created ones.

The one thing that makes the difference is when there are many layers with complex styles, e.g. styles with many categories. The problem is the same even if the layers contain no features at all.

E.g. 60 layers with a simple style --> Layer Properties opens quickly.
60 layers with a style including several categories --> Layer Properties dialog is delayed by a few seconds.

My daily work project file has hundreds of layers. Many of them have complex styles --> Layer Properties dialog is delayed by 26 seconds.

@nyalldawson
Copy link
Collaborator

@mhugo This is caused by the new selective masking widget:

image

@nyalldawson nyalldawson assigned nyalldawson and mhugo and unassigned nyalldawson Mar 9, 2020
@mhugo
Copy link

mhugo commented Mar 10, 2020

@nyalldawson Thanks for your analysis. It seems to come from QgsMaskSourceSelectionWidget::update(), where it looks for mask source candidates in all layers.
Ther may be a way to postpone this to when the user really needs it

@nyalldawson
Copy link
Collaborator

@mhugo yes, I suspect it's actually a limitation of the symbol visitor on complex projects, but I can't see that there's anyway to change that (and there's no way to thread this either). I think a deferred load might be the best approach too!

@mhugo
Copy link

mhugo commented Mar 10, 2020

@nyalldawson A short-term fix could be just to defer the loading when the widget is shown, but it will still be a bit strange in terms of UX.

A longer term fix could be a change in the logic of the mask source selection: after all, there is no need to look for mask sources in all layers, the user certainly knows which vector layer to look for first. Then mask sources inside this layer would be resolved at this moment.

@SteveLowman
Copy link
Author

Hi. This one still has a feedback label on it. Is any further feedback required? Is it relevant to be labelled as a regression for high-priority fixing?

@gioman gioman added Project and removed Feedback Waiting on the submitter for answers labels Mar 12, 2020
@gioman
Copy link
Contributor

gioman commented Mar 12, 2020

Is it relevant to be labelled as a regression for high-priority fixing?

it is, but don't expect to be so linear (I have been very vocal about fixing regressions first, but not all think the same). Anyway recently a discussion has started on this matter (bugs introduced by new features) and possibly things will change for the better.

@gioman gioman added the Regression Something which used to work, but doesn't anymore label Mar 12, 2020
@SteveLowman
Copy link
Author

Thanks @gioman
Yes, I have been following that interesting discussion, too.

@SteveLowman
Copy link
Author

SteveLowman commented Mar 24, 2020

Is it likely to be fixed by the 3.14.0 LR release, or before? If not, then would you consider withdrawing the widget for that release?

@northwestview
Copy link

I experience the same issue on 3.12 to 3.12.1, the delay to open the Layer Properties is over 5 seconds. I am reverting to 3.10.2 as there is no delay in this version using the same project.

@3nids
Copy link
Member

3nids commented Mar 27, 2020

@mhugo any news on that front?

@mhugo
Copy link

mhugo commented Mar 27, 2020

I'll try to work on it in the coming days

@roya0045
Copy link
Contributor

Can't we make the tab( and feature by extension) toggle-able? The number of people using it might no be that high but everyone will be affected by the downsides.

@mhugo
Copy link

mhugo commented Apr 3, 2020

Related PR: #35557

@SteveLowman I've tested with the project "shp_with_qml" you provided, and it seems a bit better, loading of the properties takes ~2s instead of ~3s before. It may be more significant on bigger projects.

@SteveLowman
Copy link
Author

@mhugo thanks, is this in the nightly build, or should I wait to install it next week for testing?

@mhugo
Copy link

mhugo commented Apr 3, 2020

@SteveLowman In nightly builds once it is merged

@gioman
Copy link
Contributor

gioman commented Jun 25, 2020

P.S. Perhaps the bug report title and description should be amended

@SteveLowman edited the title, maybe you can edit the description?

@SteveLowman
Copy link
Author

The number of people using it might no be that high but everyone will be affected by the downsides.

That's right - only people dealing with higher-quality cartography will use "selective masking" - everyone else (probably 95% of our users or more) will not use this feature.

So, if the fix includes an option to switch the feature on or off, then the switch should apply to all layers, not just the active layer, and perhaps the default should be 'off', too.

@andreasneumann
Copy link
Member

Excellent - having a project at hand always helps and increases the motivation of the devs to look at the problem!

Thanks @SteveLowman !

@SteveLowman
Copy link
Author

SteveLowman commented Jun 25, 2020

In QGIS 3.10, the delay to open Layer Properties is < 2 seconds
In QGIS 3.12, the delay to open Layer Properties is 37 seconds (I cannot seem to make a difference with the 'Edit Mask' checkbox)

I tried on a different machine with the 3.12 version before Edit Mask got introduced (3.12.1), and I got a delay of 83 seconds, but the comparison with 37 seconds with the Edit Mask may be inaccurate, as they were done on different machines.

@SteveLowman
Copy link
Author

P.S. Perhaps the bug report title and description should be amended

@SteveLowman edited the title, maybe you can edit the description?

Thanks @gioman. I edited the description now, and I included a link to the same sample file. I hope it is okay, but let me know if I left in any errors or typos.

@roya0045
Copy link
Contributor

@obrix I assume that you have the mean to recreate the analysis carried out by Nyall here to see if the issue is caused by something else after the fix was introduced.

@obrix
Copy link

obrix commented Jun 25, 2020

@SteveLowman Thanks for the project, I will give it a look and see If I can find out what is taking so much time.

@roya0045 I don't recognize the tool he is using in the screenshot, but I can probably produce a similar report using valgrind/callgrind and any front to analyze the data.

obrix pushed a commit to obrix/QGIS that referenced this issue Jun 26, 2020
…tor. Update is done automatically when the mask widget is opened. Should fix qgis#34942.
@obrix
Copy link

obrix commented Jun 26, 2020

I managed to make some tests to see if I could reproduce the performances issues :

I also have a long delay when opening the layer properties view on the latest master. With the dummy project provided by @SteveLowman its a ~6 seconds wait on master, whereas on the 3.10 with the same project opening the view is under 1 second.

I did some profiling on the latest master and found out that QgsMaskSourceSelectionWidget constructor was still responsible for the delay.
After some investigations with @troopa81 it seems the latest changes introduced by PR 36366 removed the check box "Edit mask settings" and changed the behavior to an automatic update of the masking symbol when the "mask" widget is opened within the property view.
But the "update" function was still called when QgsMaskSourceSelectionWidget is constructed at the opening of the layer property view. This update call is causing the delay and is unnecessary because the update is now done automatically when the mask view is opened.

So removing this update make the the layer property view open in under 1 second again for me. The costly process is now done when the mask widget is opened. I will make a pull request so @SteveLowman can test it.

@andreasneumann
Copy link
Member

Thank you so much @obrix for this fast investigation. It sounds promising. I will be able to test this weekend and will report here.

@roya0045
Copy link
Contributor

@obrix Thanks for confirming what I was thinking but couldn't confirm, without any call to update there was no reason for the long delay but I couldn't find rapidly where the call was still coming from.

@andreasneumann
Copy link
Member

andreasneumann commented Jun 29, 2020

@obrix - I tested this PR and can confirm that the layer properties dialog now opens in about a second or less with this project with many layers/symbol layers.

The only "issue" is, that when switching to the "Masks" panel, now the dialog hangs for some seconds (expected by me), without any indication that QGIS is loading mask definitions. If we could issue some warning/indication that QGIS is currently populating the mask s dialog and isn't "dead" it would be great for such large projects.

But then I don't know how much of these large/huge projects are really out there in the wild ...

@roya0045
Copy link
Contributor

@andreasneumann in that case the most common error might be a missclick as the symbol is similar to the one above. Clicking the wrong button and having qgis 'hang' might be annoying at most. Though this makes me wonder if the populating could be done in another thread rather than the main one.

@troopa81
Copy link
Contributor

If we could issue some warning/indication that QGIS is currently populating the mask s dialog and isn't "dead" it would be great for such large projects.

Seems a good idea

Though this makes me wonder if the populating could be done in another thread rather than the main one.

And another good one, so the user could still switch properties tab

@obrix
Copy link

obrix commented Jun 30, 2020

@troopa81 @andreasneumann Popping a warning here seems to be a good idea. Now threading the populate process is something else, @nyalldawson seems to indicate this is not easy to thread

@mhugo yes, I suspect it's actually a limitation of the symbol visitor on complex projects, but I can't see that there's anyway to change that (and there's no way to thread this either). I think a deferred load might be the best approach too!

I do not immediately see why, and if this apply to the whole process or only the visitors, but it could potentially avoid the unwanted wait on a missclick in huge project.

@roya0045
Copy link
Contributor

This is what I though with the visitor, I couldn't remember if it was possible to simply have the populating start in the task manager and all will be fine or if all the sub process would also need to be 'contained' in another thread, if even possible.

@troopa81
Copy link
Contributor

I do not immediately see why,

Maybe because if you're modifying the layer renderer configuration while you running through the visitor in a thread, you'll get into trouble.

@SteveLowman
Copy link
Author

Hi. Which version should I install to test this? Should it be a nightly build, or is the fix there in the current point release?

@roya0045
Copy link
Contributor

roya0045 commented Jul 8, 2020

Not merged yet, though you could try the mxe build artefact here https://github.com/qgis/QGIS/suites/889934897/artifacts/10561954
Unzip twice, click qgis.exe and open one of your project

nyalldawson pushed a commit that referenced this issue Jul 13, 2020
…tor. Update is done automatically when the mask widget is opened. Should fix #34942.
@SteveLowman
Copy link
Author

Can I ask again please, which version should we now install to get beyond this bug?

@roya0045
Copy link
Contributor

Master or an mxe

@obrix
Copy link

obrix commented Jul 16, 2020

@SteveLowman The fix has been merged and is available on the nightly of the master branch (3.15 master).

@jjk0giap
Copy link

Any chance to backport fix to 3.14?

@roya0045
Copy link
Contributor

@jjk0giap 3.16 is the next release. I doubt a backport will occur.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Project Regression Something which used to work, but doesn't anymore
Projects
None yet