[Qgis-developer] Potentially serious performance regression in new geometry - should 2.10 be delayed?

[Matthias Kuhn]

Hi,

I don't think reverting the geometry work is an option.
It works well in general and offers really nice new possibilities with z
and m values and circular arcs despite it was merged a bit late, did not
receive a lot of attention in the freeze period and lacks some unit tests.

But then, should we spoil CPU cycles and memory just because it's
present on modern machines?
The way I see it, QGIS should also be able to run on limited hardware
like portable devices or older hardware.

IIRC implicitly shared geometries have been one of the features which
were an integral part of this from the beginning. If we don't have it
now but are able to add it later on I don't mind so much but I do mind
if we miss the oportunity to add it now.

So the question is, is this something that we can fix later on, or would
fixing it require an API break and therefore we need to wait for 3.0 if
we don't fix it now?

Regards,
Matthias

On 06/23/2015 11:37 AM, Marco Hugentobler wrote:
> Hi Martin
>
> The new geometries uses more memory for backward compatibility. In the
> mid/long term, the cached wkb will be removed, so QgsAbstractGeometry
> will be the only represention. The provider can feed wkb (e.g.
> database providers) or construct the geometry by feeding QgsPointV2s.
>
> I'm however surprised that this should have such an impact with the
> attached dataset. Because the shp has size 588MByte, you could load
> that in memory several times (and we don't load everything into memory
> in QGIS in case the dataset is even bigger). Looking on the attached
> file, I think the problem is not the memory, but rather some round
> trips in the closestVertex function.
>
> But it is far from something that would justify a release delay or
> even a geometry rollback.
>
> Regards,
> Marco
>
> On 23.06.2015 05:42, Martin Dobias wrote:
>> Hi
>>
>> Agreed with Nyall. Apart from the above mentioned problems, there are
>> newly introduced performance regressions with snapping: the cached
>> geometries in 2.10 take double the amount of memory they used in 2.8,
>> and snapping got much slower with more complex layers (compared to
>> 2.8) - e.g. with [1]. The extra memory consumption is caused by the
>> fact that providers provide geometry in WKB representation, which is
>> then converted to new representation and WKB is kept. We should use
>> just one representation and drop the usage of the other one - that
>> means stop using WKB in providers and common code paths, so the WKB
>> representation does not even get created (and cached).
>>
>> One heretic idea at the end - what others think about postponing the
>> release of the new geometry architecture to 2.12 so that there is more
>> time to address the current issues (fix performance, fix high memory
>> consumption, clean up API, write unit tests). It seems to me that some
>> of the issues would be difficult to address even if the release of
>> 2.10 is moved by another week or so.
>>
>> Regards
>> Martin
>>
>> [1] http://www.iucnredlist.org/spatial-data/MAMMALS_TERRESTRIAL.zip
>>
>>
>> On Tue, Jun 23, 2015 at 10:09 AM, Nyall Dawson
>> <nyall.dawson at gmail.com> wrote:
>>> Hi all,
>>>
>>> Unfortunately, we've become aware of a serious performance
>>> regression caused
>>> by the new geometry engine. Basically, the situation is that for all
>>> geometry operations which rely on geos (think buffers, splits, spatial
>>> relation operations such as intersects and within,... ) the geometry
>>> now
>>> needs to be converted into a geos representation with *every*
>>> operation. In
>>> the old geometry engine this conversion was done once, and the
>>> result stored
>>> so that follow up operations would not need to recalculate it. This
>>> potentially has huge impacts on the performance of common tasks such as
>>> selecting all features which intersect a geometry.
>>>
>>> I've had a look, and unfortunately it's not trivial to fix this. I
>>> think the
>>> correct solution to this is to:
>>>
>>> - make QgsGeometryEngine accept and return QgsGeometry containers, not
>>> QgsAbstractGeometryV2
>>> - store the generated geos representation of geometries within
>>> QgsGeometryPrivate inside the QgsGeometry container. This way it
>>> will be
>>> reusable between different geometry operations, and shared when
>>> QgsGeometry
>>> objects are copied. This will also have the benefit that if a
>>> geometry is
>>> prepared using geos then subsequent geos operations performed on that
>>> QgsGeometry and its shared copies will be much faster.
>>> - make QgsGeometry a friend class of QgsGeo, so that it can access
>>> QgsGeometryPrivate to retrieve or set the geos representation of the
>>> geometry as required
>>>
>>> An alternative (short term) solution would be to just cache the geos
>>> representation when geometry operations are called through the older
>>> QgsGeometry modification/relationship operations. This would be
>>> easier, but
>>> means that the API of QgsGeometryEngine will be stuck with the current
>>> design, and we won't be able to properly fix this until breaking the
>>> api for
>>> 3.0.
>>>
>>> Either way, I doubt this can be addressed within the remaining 3
>>> days we
>>> have until release. Should we delay to address this? Release with the
>>> regression? Or am I missing something and there's an easier solution we
>>> could implement? Or even possibly this additional cost of
>>> recalculating the
>>> geos representation is trivial and can be ignored (maybe someone
>>> could test
>>> this with a little repeated intersection script)?
>>>
>>> Thoughts?
>>>
>>> Nyall
>>>
>>>
>>> _______________________________________________
>>> Qgis-developer mailing list
>>> Qgis-developer at lists.osgeo.org
>>> http://lists.osgeo.org/mailman/listinfo/qgis-developer
>> _______________________________________________
>> Qgis-developer mailing list
>> Qgis-developer at lists.osgeo.org
>> http://lists.osgeo.org/mailman/listinfo/qgis-developer
>
>