[Marble-devel] [marble] src/lib: LatLonAltBox: returning it by value is a perfo hit. return by ref instead.

[Bernhard Beschow]

Hi,

On Mittwoch, 18. Juli 2012 23:28:34 Thibaut Gridel wrote:
> Shared d-pointer is complex indeed to implement.

Complexity can be dealt with by unit tests. See e.g. TestGeoDataCoordinates.cpp.

> For the record not all geodata classes use cow in a d-pointer, only those 
> inheriting geodatafeature and geodatageometry.
> It was a major issue back in 2009-2010 when cow did not play nice at all 
> in a QAbstractItemModel with pointers semantic.

Given that the old code already had value semantics, implicit sharing (cow) would have preserved just that behavior, while improving performace virtually as much as the new code. The new code, however, doesn't return const-references, which allows uncontrolled modifications from outside.

> More to the point, the rationale for the commit is that valgrinding 
> GeometryLayer::render in an osm file had the following results:
> 
> 25 calls to render for about 4000 graphicsItems had
> 5% of cycles doing 469 730 ctor of GeoDataLatLonAltBox
> 3% of cycles doing same amount of dtor
> 
> As a comparison in that run atanh as used in 
> MercatorProjection::screenCoordinates used
> 3% of cycles also for 2 920 076 calls.
> and QPainter::drawPolyline did account for 54% of cycles.

These are interesting numbers indeed which were nice to have the commit message. That way, we all get an idea *why* the code was modified this way and what difference it makes.

> All this ctor/dtor dance disappeared while returning references instead of 
> returning by value. 
> 
> Typical use case of bbox are tests like:
> if ( !viewport->resolves( lineString.latLonAltBox() ) )
> thus a real throwaway anonymous...
> 
> To me the ideal api is returning a reference.

The problem with returning references is that clients of this API may be surprised by a) potential lifetime issues and b) references changing values behind the scenes. In the example you provided above these surprises may not be very present. But who knows where this API will be used in the future? Maybe in message-passing between threads? Both issues can be avoided by implicit sharing, so I think it's well worth to consider its use here.

Greetings,
Bernhard

> On Wednesday 18 July 2012, you wrote:
> > The small problem there would be that GeoDataLatLonAltBox inherits 
> from
> > GeoDataLatLonBox. So in that regard it's rather like
> > GeoDataLineString/LinearRing/Polygon, since it means implicit sharing
> > across inheritance hierarchies. So it's a bit more complex to implement
> > IIRC and does a few uncommon things with C++ .... This isn't meant to
> > oppose your idea but I just wanted to point it out.
> > 
> > BR,
> > Torsten
> > 
> > On Mittwoch, 18. Juli 2012 13:58:31 Bernhard Beschow wrote:
> > > Hi Thibaut,
> > > 
> > > why not make GeoDataLatLon(Alt)Box implicitly shared, like e.g.
> > > GeoDataCoordinate? IMO, that would add for a more coherent API 
> with less
> > > surprises.
> > > 
> > > Greetings,
> > > Bernhard
> > > 
> > > On Dienstag, 17. Juli 2012 22:20:44 Thibaut Gridel wrote:
> > > > Git commit ff6a02a7bbf47c1195d183da2233653260084c3e by 
> Thibaut Gridel.
> > > > Committed on 17/07/2012 at 19:31.
> > > > Pushed by tgridel into branch 'master'.
> > > > 
> > > > LatLonAltBox: returning it by value is a perfo hit. return by ref
> > > > instead.
> 
>