Skip to content

Commit

Permalink
Clean up QgsExpressionContext in QgsServer::handleRequest
Browse files Browse the repository at this point in the history
  • Loading branch information
@dmarteau @rldhont
dmarteau authored and rldhont committed Jun 7, 2016
1 parent 65aa860 commit 5c3aa51
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/server/qgsserver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,13 @@ QPair<QByteArray, QByteArray> QgsServer::handleRequest( const QString& queryStri
int logLevel = QgsServerLogger::instance()->logLevel();
QTime time; //used for measuring request time if loglevel < 1
QgsMapLayerRegistry::instance()->removeAllMapLayers();

// Clean up Expression Context
// because each call to QgsMapLayer::draw add items to QgsExpressionContext scope
// list. This prevent the scope list to grow indefinitely and seriously deteriorate
// performances and memory in the long run
sMapRenderer->rendererContext()->setExpressionContext( QgsExpressionContext() );

sQgsApplication->processEvents();
if ( logLevel < 1 )
{
Expand Down

14 comments on commit 5c3aa51

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where exactly are the items added?
This is probably a workaround that only masks the real issue.
They probably should be pushing a new scope and popping it after usage.

@rldhont
Copy link
Contributor

@rldhont rldhont commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit commes from a PR #3181

with this description:

Each call to QgsMapLayer::draw add items to QgsExpressionContext scope.
The scope is not cleaned between request and thus the scope list to grow indefinitely.

Having the list growing indefinitely deteriorate seriously performances in the long run. It also increase the memory footprint but in a less noticiable way.

This behavior has been checked by monitoring reponse time of several ten of thousand identical requests during server long runs.

Not sure it this is the most elegant way of fixing this but it does the job.

I'm writing an email about t and other degradation of performance in QGIS Server.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have copied the question there so we can track the issue to its roots.

@dmarteau
Copy link
Contributor Author

@dmarteau dmarteau commented on 5c3aa51 Jun 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have the felling that there is not legitimate reason for the item not to be removed at some point.
QgsVectorLayer::draw creates a QgsVectorLayerRenderer. The item is added in QgsVectorLayerRenderer constructor (https://github.com/qgis/QGIS/blob/master/src/core/qgsvectorlayerrenderer.cpp#L103) but seems to be never removed.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling that QgsVectorLayerRenderer::mContext should be a copy and not a reference to the context. https://github.com/qgis/QGIS/blob/master/src/core/qgsvectorlayerrenderer.h#L101

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmarteau can you check if this approach also works:
#3182

@dmarteau
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mine is that the the layer scope is misplaced: it should be added/popped in the draw() function
adding/removing scope should be QgsVectorLayerRenderer::draw function (the same way the function QgsVectorLayerRenderer::drawRendererV2 add/pop 'symbolScope').
IMHO, there is no point to add a scope in the constructor.

@dmarteau
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3182: I like enforcing immutability as much as possible but actually drawRendererV2 mutate the context.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mine is that the the layer scope is misplaced: it should be added/popped in the draw() function

But the context is also used in other methods like render()

3182: I like enforcing immutability as much as possible but actually drawRendererV2 mutate the context.

Then the question is probably, why? :)

@dmarteau
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, It was about render(), not draw().
I don't know enough about the purpose of the context in this game to have a clear idea of what to do but:
the context is passed around in lot of method and object and I'm affraid that passing that object by reference somewhere and by copy elsewhere may be confusing. Furthermore, I have concerns about the overhead of copying object at each invocation.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copy happens once per layer rendering. Adding the scope in render() would be once for each feature being rendered, if there's actually an impact on performance, I'd be more afraid of the second one.

Is it still passed as reference in other places?

@dmarteau
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in a lot of places (as mutable object), mostly rendering functions.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 5c3aa51 Jun 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what @wonder-sk and @nyalldawson think

@wonder-sk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say ``QgsRenderContext` is a kind of class where nobody has defined its rules for using it properly, so we use it in various ways. It is kind of a place where to dump random stuff needed during rendering :-)

In this particular case, wouldn't it be the best to just clean up the expression context in the rendering class that added items?

The problem would also go away with a switch to the renderer job classes introduced in 2.4, but I understand it may take a while to switch to those. (the renderer jobs create their own contexts when needed).

Finally, it would probably make sense to look at changing QgsRenderContext for 3.0. It has fair amount of redundancy with QgsMapSettings and in various places (e.g. labeling) these two are used together. We could embed QgsMapSettings into QgsRenderContext and get rid of the duplicated variables.

Please sign in to comment.