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

[server] parallel map rendering #3886

Merged
merged 1 commit into from Jan 9, 2017

Conversation

Projects
None yet
10 participants
@pblottiere
Copy link
Member

pblottiere commented Dec 19, 2016

This PR allows to activate the parallel map rendering in QGIS Server.

By default, global QGIS QSettings are used to switch between parallel and sequential rendering.

But the QGIS_OPTIONS_PATH environment variable can be defined before spawning the server and the $QGIS_OPTIONS_PATH/QGIS/QGIS3.ini file is then read to retrieve rendering configuration.

For example:

[qgis]
max_threads=4
parallel_rendering=true

Filtering expressions coming with QgsAccessControl can now be resolved before the rendering step. Thus, when the parallel map rendering is activated, filtering epressions are resolved within the main thread. Otherwise, the server hangs when multiple C++ threads are calling the same Python instance.

Let me know what you think.

@rldhont rldhont requested a review from elpaso Dec 19, 2016

@rldhont rldhont modified the milestones: 2.12, QGIS 3 Dec 19, 2016

@rldhont rldhont requested review from mhugent , rldhont and sbrunner Dec 19, 2016

@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Dec 19, 2016

I think this PR will interest @dmarteau, @mdouchin and @yjacolin

@elpaso
Copy link
Contributor

elpaso left a comment

This is a general consideration that does not only apply to this particular PR which I think is a very good job!

I think that in the server we should try to stick to the 12 factors philosophy for the server configuration: pass all configuration through environment variables.
I think it's ok to have the configuration in the settings but I'd prefer if the settings could also be overridden by environment variables.

My suggestion is to add a new environment variables for each new configuration parameter and use the settings as a fallback in case the enviroment var is not set.

Ideally, we should be able to configure the server completely from the environment.

See: https://12factor.net/

* reading qsettings.
* @note added in QGIS 3.0
*/
class QgsMapRendererJobProxy

This comment has been minimized.

@elpaso

elpaso Dec 19, 2016

Contributor

Maybe worth moving QgsMapRendererJobProxy into it's own file?

@wonder-sk

This comment has been minimized.

Copy link
Member

wonder-sk commented Dec 19, 2016

I am not sure this is the right approach. If I understand correctly, the proxy is introduced just to work around the fact that python cannot be used from multiple threads at the same time. In QGIS application however we do not have this problem, so I believe the issue comes from the fact that the server python support just does not release the python interpreter lock, so that other threads can run python code.

So in summary I think we do not need renderer job proxy, we just need to fix python support in the server.

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Dec 19, 2016

@wonder-sk

Actually, the proxy is just here to hide the parallel/sequential render job configuration.

It's the new function QgsAccessControl::resolveFilterFeatures( ... ) wich fixes the issue by allowing to resolve expression filtering in the main thread instead of doing it in threads.

But I can take a look on how QGIS application does.

@wonder-sk

This comment has been minimized.

Copy link
Member

wonder-sk commented Dec 19, 2016

Ideally just like in rendering in QgsMapCanvas, the only difference between parallel/sequential rendering job should be whether one uses QgsMapRendererParallelJob or QgsMapRendererSequentialJob. Everything else should be the same, otherwise we are risking rendering bugs that are hard to track down.

For access control, it would be worthwhile to write a test to ensure that it's working correctly with parallel/sequential jobs without extra workarounds.

@yjacolin

This comment has been minimized.

Copy link
Member

yjacolin commented Dec 20, 2016

As soon as this PR is merger, I will run a performance test and will share you on qgis-dev ML!

@pblottiere pblottiere force-pushed the pblottiere:servermultithread branch from 9f4310d to e512679 Dec 20, 2016

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Dec 20, 2016

@elpaso Thank you for your comment!

I think that in the server we should try to stick to the 12 factors philosophy for the server configuration: pass all configuration through environment variables.
I think it's ok to have the configuration in the settings but I'd prefer if the settings could also be overridden by environment variables.

I added a QgsServerSettings class to centralize and manage the priority between environment variable and global settings.

I only did it with two new environment variables : QGIS_SERVER_PARALLEL_RENDERING and QGIS_SERVER_MAX_THREADS. If you think it's a good idea, I can extend the behaviour with others variables too.

@elpaso

This comment has been minimized.

Copy link
Contributor

elpaso commented Dec 20, 2016

@pblottiere thanks for implementation!
Yes, I think that ideally all config vars should be treated the same.
We currently have, at least:
QGIS_DEBUG (this is in core)
QGIS_CUSTOM_CONFIG_PATH (also in core, if I'm not wrong)
QGIS_SERVER_LOG_LEVEL (is it just me finding that counter intuitive? this should probably be changed to 0=no logging, 3=max logging)
QGIS_SERVER_LOG_FILE (evil! logs should probably be just printed to stderr and let apache/nginx handle the logs)
QGIS_OPTIONS_PATH (this is also in core)
QGIS_PLUGINPATH (bad named by me, I'd rename it to QGIS_PLUGINS_PATH for QGIS 3 and maybe support the QGIS_PLUGINPATH deprecated naming for a while)

@Gustry

This comment has been minimized.

Copy link
Contributor

Gustry commented Dec 20, 2016

I added a QgsServerSettings class to centralize and manage the priority between environment variable and global settings.

It looks good. Does it log in the QGIS log that one setting has been overwrite by the environment ? This is very useful for debugging.

@haubourg

This comment has been minimized.

Copy link
Contributor

haubourg commented Dec 21, 2016

@yjacolin Testing that in your benchmark tool could help tracking if we really gain what expected, and if we don't face issues with multiple concurrent access. On my tests with only one user, I found really good performance improvements. In the other way, there is still something more CPU consuming in server context than in desktop, since I just ear the CPU fan blowing for the same request :) Maybe the png issue you tracked?

@pblottiere pblottiere force-pushed the pblottiere:servermultithread branch 3 times, most recently from 92714a5 to dc6b794 Dec 21, 2016

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Dec 26, 2016

Can the singleton introduced here be avoided? We are trying not to add anymore to the code - see qgis/qgis4.0_api#42

@haubourg

This comment has been minimized.

Copy link
Contributor

haubourg commented Dec 26, 2016

whatwhatwhat?! @pblottiere @vmora @mhugo hey the singleton killer team, is Nyall right? We might do something here :)

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Dec 26, 2016

Can the singleton introduced here be avoided? We are trying not to add anymore to the code - see qgis/qgis4.0_api#42

@haubourg Yes, @nyalldawson is perfectly right. The settings management class was initially just a proposition, so I made it simple. But if we want to keep this idea, indeed, the singleton has to be removed :)

@pblottiere pblottiere force-pushed the pblottiere:servermultithread branch from 07f8b00 to 4395d59 Jan 2, 2017

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 2, 2017

@wonder-sk

we just need to fix python support in the server.

I looked into the issue causing the parallel render job to hang.

Actually, without the changes I made to resolve filtering expressions in the main thread, the server hangs only when these conditions are met:
1 - QgsServer is used in Python
2 - AccessControl plugins are registered
3 - a parallel map rendering is done

And these conditions are typically encountered during tests.

But everything is working well when the server runs "for real", so the python support in the server seems OK.

I think it's due to the initPython function called in the server's main (and not called in unit tests).

In short, the server hangs when a Python instance is using a QgsServer that calls C++ threads that call Python classes (thanks to an inherited class and a SIP binding).

I do not have a clue on how it can be resolved. However, in any case, it seems more reasonable to avoid this configuration by always resolving filtering expressions in main thread (not time consuming since we just want to retrieve a string).

Ideally just like in rendering in QgsMapCanvas, the only difference between parallel/sequential rendering job should be whether one uses QgsMapRendererParallelJob or QgsMapRendererSequentialJob. Everything else should be the same, otherwise we are risking rendering bugs that are hard to track down.

Moreover, to avoid differences between parallel and sequential rendering, filtering expressions are always resolved in the main thread. Thus, everything is exactly the same except we use QgsMapRendererParallelJob in one case and QgsMapRendererSequentialJob in another.

Thanks a lot for your comments. Tell me what you think.

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 2, 2017

@elpaso

I have updated the QgsServerSettings class to manage these environment variables:

  • QGIS_OPTIONS_PATH
  • QGIS_SERVER_PARALLEL_RENDERING (new)
  • QGIS_SERVER_MAX_THREADS (new)
  • QGIS_SERVER_LOG_LEVEL
  • QGIS_SERVER_LOG_FILE
  • QGIS_PROJECT_FILE
  • MAX_CACHE_LAYERS
  • QGIS_SERVER_CACHE_DIRECTORY (new)
  • QGIS_SERVER_CACHE_SIZE (new)

@Gustry

It looks good. Does it log in the QGIS log that one setting has been overwrite by the environment ? This is very useful for debugging.

A logSummary function is called to print the current values of each settings as well as the origin (default value, ini file or environment variable).

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 2, 2017

@haubourg

There's no more singleton!

@pblottiere pblottiere force-pushed the pblottiere:servermultithread branch from 4395d59 to f7729b3 Jan 2, 2017

@m-kuhn m-kuhn added the Server label Jan 8, 2017

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 9, 2017

@elpaso @rldhont
Are you agree for merging this or maybe do you have other comments?

@elpaso

This comment has been minimized.

Copy link
Contributor

elpaso commented Jan 9, 2017

@pblottiere looks good to me!

@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 9, 2017

@pblottiere @elpaso This certainly should be merged before #3866 because qgswmsserver files has been removed from the server code unit and it it will be a bit messy if you try to merge into removed files ! I will manage to port the changes to wms server before the merging of #3866.

@rldhont

rldhont approved these changes Jan 9, 2017

@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Jan 9, 2017

@dmarteau if you're agree, I will merge.

@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 9, 2017

@rldhont ok to merge, I will sync #3866.

@rldhont rldhont merged commit 350a2b5 into qgis:master Jan 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Jan 9, 2017

@pblottiere there are some errors and warnings to fixe!

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 9, 2017

@rldhont
Strange, everything was green on Travis...

I'm gonna take a look.

@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Jan 9, 2017

@pblottiere I revert the merge

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 9, 2017

@rldhont
Do you have an idea why Travis was not complaining?

@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 9, 2017

@pblottiere I have tried to merge your code into #3866: there is a lot of conflicts and it's somewhat painful to rebase. I'm going to try the other way and see if it is cleaner: so there will only need to migrate changes to wmsserver.

rldhont added a commit that referenced this pull request Jan 9, 2017

Revert "Merge pull request #3886 from pblottiere/servermultithread"
This reverts commit 350a2b5, reversing
changes made to 590a981.
@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 9, 2017

@pblottiere Merging your code into #3866 was somewhat painful, I'm going to try the other way.

@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Jan 9, 2017

@pblottiere after the merge here are the errors. https://dash.orfeo-toolbox.org/viewBuildError.php?type=0&buildid=257417
There is also warnings.
But #3969 also introduces warnings.

@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Jan 9, 2017

No more warnings on master!

@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 9, 2017

@pblottiere @elpaso @rldhont: I have managed to merge #3886 onto #3866 which is much easier than the opposite. The result is here https://github.com/dmarteau/QGIS/tree/pblottiere-servermultithread. The code compiles and test are passed.
So I suggest to merge #3866 first. Then you can merge #3866 by eventually backporting merge from https://github.com/dmarteau/QGIS/tree/pblottiere-servermultithread.

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 9, 2017

@dmarteau
Thanks for having tested this! I still have to do a fresh compilation to take a look on warnings/errors previously described.

@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 9, 2017

@pblottiere I think this is because you missed a rebase with master.

@rldhont

This comment has been minimized.

Copy link
Contributor

rldhont commented Jan 10, 2017

@pblottiere #3866 has been merged, so you have to update this PR.

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 10, 2017

@dmarteau @rldhont Errors was indeed due to a recent commit in master which introduced a qgis_server.h header. So, I just have to include it in qgsserversettings.h and everything is fine...

I'm gonna take a look to rebase with master now that the PR #3866 has been merged.

@dmarteau

This comment has been minimized.

Copy link
Contributor

dmarteau commented Jan 10, 2017

@pblottiere: look at https://github.com/dmarteau/QGIS/tree/pblottiere-servermultithread, may be you will find most of the rebasing done here.

@pblottiere

This comment has been minimized.

Copy link
Member Author

pblottiere commented Jan 11, 2017

@dmarteau

I mainly took your work for rebasing. Everything seems OK but I still got the next error from Travis "The job exceeded the maximum time limit for jobs, and has been terminated".

I'll restart the build later.

nyalldawson added a commit to nyalldawson/QGIS that referenced this pull request Jan 15, 2017

@rldhont rldhont referenced this pull request Mar 30, 2017

Open

QEP 74: QGIS server code refactoring for QGIS 3.0 #74

9 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment