geos icon indicating copy to clipboard operation
geos copied to clipboard

WIP ConstCoordSeq

Open kylebarron opened this issue 2 years ago • 3 comments

Closes https://github.com/georust/geos/issues/137

This adds a new struct for ConstCoordSeq that contains a const pointer to the GEOS coord seq object.

The ideal API would probably be to have something like the Geom trait for coordinate sequences, but I assume that's backwards-incompatible, because users would then also have to import such a trait?

I didn't notice any performance improvement on my relevant benchmark in my geoarrow project, so it's possible I either implemented it wrong or this is such a small performance gain that it's immeasurably small.

kylebarron avatar Oct 01 '23 15:10 kylebarron

I don't see anything bad in particular in the implementation. So the relevant question here would be: are you sure this clone was this expensive for you? Do you have a flamegraph comparison between before and after?

GuillaumeGomez avatar Oct 01 '23 16:10 GuillaumeGomez

are you sure this clone was this expensive for you

No I'm not 😄 it was an (uneducated?) guess. I need to learn how to better profile rust code 😅

kylebarron avatar Oct 01 '23 16:10 kylebarron

Well in any case, this PR seems to be on good track so if you fix the errors, I'll gladly merge it. Take a look at how the other Const* types were implemented if you need to find out.

GuillaumeGomez avatar Oct 01 '23 16:10 GuillaumeGomez