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

[Thibaut Gridel]

Hi,

Shared d-pointer is complex indeed to implement.
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.

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.

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.

Best Regards,
Thibaut

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.