Reduce GEOS conversions and preparations of geometries #45195

strk · 2021-09-22T08:47:02Z

This is a spin-off of PR #45057 which surfaced a problem with duplicated GEOS conversions and preparations.

In this PR the aim is to reduce the number of GEOS preparation as much as possible.
For testing I'm using a polygon layer containing 310 features and running the "Extract Within Distance" processing algorithm to extract features being within 10 units distance from features of a 1-feature point layer.

It was found, in #45057 (comment) that QGIS is running 1550 prepare operations.

The first commit in this PR (using shared_ptr for the GeometryEngine member of QgsFeatureRequest) reduces the preparation to 620 (less than half).

The 620 number probably means that for each of the 310 polygons we're re-preparing the same POINT from the points layer.

strk · 2021-09-22T08:55:12Z

The idea proposed by @nyalldawson to put the GeometryEngine inside the QgsGeometry object sounds good to me, because it opens the door to allowing a QgsGeometry to be always used as prepared once it was prepared, in its whole lifetime.

m-kuhn

+1

strk · 2021-09-22T09:47:41Z

With next commit I've used a shared_ptr for the mDistanceWithinEngine member of the QgsVectorLayerFeatureIterator class as well, and exposed a referenceGeometryEngine() method to the QgsFeatureRequest class. Doing so the number of GEOS preparation go down to 310 (one per feature in the 310 features layer). I'm not sure why it's not 311 (for the preparation of the point geometry) but maybe that's because QGIS avoids preparing POINTS (would make sense to avoid that)

nirvn · 2021-09-22T09:49:36Z

@strk , do you have numbers in seconds saved? :) nice work!

strk · 2021-09-22T09:57:07Z

Inverting the inputs to the algorithm results in a single preparation, so points are STILL prepared by QGIS, just evidently the layer to extract the geometries from is prepared, and the other is not (shouldn't it be the other way around ?)

nyalldawson · 2021-09-22T10:42:04Z

python/core/auto_generated/qgsfeaturerequest.sip.in

@@ -405,6 +405,8 @@ will use the same CRS as the source layer/provider.
 .. versionadded:: 3.22
 %End

+    std::shared_ptr< QgsGeometryEngine > referenceGeometryEngine() const;


This shouldn't be exposed to sip... it's the sort of API we should reserve the ability to revise in future

(also keeping in mind that if we do end up storing prepared instances in QgsGeometryPrivate then this method will no longer be required)

Hidden in next commit

strk · 2021-09-22T12:33:32Z

@nirvn for timing I'm going to change dataset.
These numbers are with use of a LINESTRING layer containing 43336 features and a POINT layer containing 10000 features.
The changes in this PR seem to actually make the operation SLOWER, despite running 1/5 of the preparations

Extracting LINESTRINGs within 0.05 units from the POINTs :
Execution completed in 112.37 seconds (1 minute 52 seconds) -- BEFORE PR with debug logging to file, showing 50000 preparations
Execution completed in 115.15 seconds (1 minute 55 seconds) -- WITH PR with debug logging to file, showing 10000 preparations (1/5)
Execution completed in 71.33 seconds (1 minute 11 seconds) -- BEFORE PR (no debug logging)
Execution completed in 74.03 seconds (1 minute 14 seconds) -- WITH PR

Extracted features: 13,382

Extracting POINTs within 0.05 units from the LINESTRINGs:
Execution completed in 3.21 seconds BEFORE PR
Execution completed in 3.38 seconds WITH PR
Extracted features: 7,979

Note how much faster it is to extract POINT withinDistance of LINES. This is most likely due to the algorithm preparing only the "query layer" and not the "extraction layer" (might benefit from a re-thinking of it).

strk · 2021-09-30T10:37:27Z

I've removed the non-topical commits from this PR, so it only contains the actual code to reduce conversions and preparations.
The other changes are now in their own PRs:

Use GEOSDistanceWithin when available #45340 for using GEOSDistanceWithin when available
More heuristics to determine whether to iterate over target source #45339 for improving the heuristic of the DistanceWithin processing algorithm to avoid preparing points

This reduces the number of GEOS conversions and preparations upon copying the request (for example by QgsDistanceWithinAlgorithm)

Does so by exposing a referenceGeometryEngine method to QgsFeatureRequest and copying the engine rather than creating a new one on each iteration.

strk · 2021-10-01T16:08:19Z

Rebased to current master to pass CI.

github-actions · 2021-10-16T02:41:42Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

strk · 2021-10-16T22:41:40Z

This is still to be merged, but I wanted to see actual better numbers or it wouldn't make sense.
It would maybe help if @nyalldawson can find his test dataset to check this code against

nyalldawson · 2021-10-17T22:37:48Z

This looks good to me -- I'd be happy to see it merged now!

strk · 2021-10-18T13:25:19Z

Oh well, let's merge it then, and delegate performance testing to the heuristics PR...

strk · 2021-10-21T20:48:27Z

For the record: I just re-run the test with current PR #45384 and the timing to extract the LINESTRING goes down to <16 seconds

strk self-assigned this Sep 22, 2021

strk requested a review from nyalldawson September 22, 2021 08:47

github-actions bot added this to the 3.22.0 milestone Sep 22, 2021

m-kuhn approved these changes Sep 22, 2021

View reviewed changes

nyalldawson reviewed Sep 22, 2021

View reviewed changes

strk marked this pull request as draft September 22, 2021 12:33

strk force-pushed the shared-engine-in-request branch 2 times, most recently from 2bd4d74 to 88d7563 Compare September 22, 2021 13:58

strk force-pushed the shared-engine-in-request branch from 88d7563 to 65d7f80 Compare September 30, 2021 09:01

strk mentioned this pull request Sep 30, 2021

More heuristics to determine whether to iterate over target source #45339

Merged

strk force-pushed the shared-engine-in-request branch from 6619260 to b7ea9a7 Compare September 30, 2021 10:35

strk added 2 commits October 1, 2021 16:07

Have QgsFeatureRequest store ref geometry engine by shared pointer

6e495de

This reduces the number of GEOS conversions and preparations upon copying the request (for example by QgsDistanceWithinAlgorithm)

Avoid re-preparing geos geometry from QgsVectorLayerFeatureIterator

d917a75

Does so by exposing a referenceGeometryEngine method to QgsFeatureRequest and copying the engine rather than creating a new one on each iteration.

strk force-pushed the shared-engine-in-request branch from b7ea9a7 to d917a75 Compare October 1, 2021 14:07

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 16, 2021

github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 16, 2021

strk removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 16, 2021

nyalldawson approved these changes Oct 17, 2021

View reviewed changes

strk marked this pull request as ready for review October 18, 2021 13:25

strk merged commit 27ae2c9 into qgis:master Oct 18, 2021

strk deleted the shared-engine-in-request branch October 18, 2021 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce GEOS conversions and preparations of geometries #45195

Reduce GEOS conversions and preparations of geometries #45195

strk commented Sep 22, 2021

strk commented Sep 22, 2021

m-kuhn left a comment

strk commented Sep 22, 2021

nirvn commented Sep 22, 2021

strk commented Sep 22, 2021

nyalldawson Sep 22, 2021

nyalldawson Sep 22, 2021

strk Sep 22, 2021

strk commented Sep 22, 2021

strk commented Sep 30, 2021

strk commented Oct 1, 2021

github-actions bot commented Oct 16, 2021

strk commented Oct 16, 2021

nyalldawson commented Oct 17, 2021

strk commented Oct 18, 2021

strk commented Oct 21, 2021

Reduce GEOS conversions and preparations of geometries #45195

Reduce GEOS conversions and preparations of geometries #45195

Conversation

strk commented Sep 22, 2021

strk commented Sep 22, 2021

m-kuhn left a comment

Choose a reason for hiding this comment

strk commented Sep 22, 2021

nirvn commented Sep 22, 2021

strk commented Sep 22, 2021

nyalldawson Sep 22, 2021

Choose a reason for hiding this comment

nyalldawson Sep 22, 2021

Choose a reason for hiding this comment

strk Sep 22, 2021

Choose a reason for hiding this comment

strk commented Sep 22, 2021

strk commented Sep 30, 2021

strk commented Oct 1, 2021

github-actions bot commented Oct 16, 2021

strk commented Oct 16, 2021

nyalldawson commented Oct 17, 2021

strk commented Oct 18, 2021

strk commented Oct 21, 2021