search

georust / geos

Rust bindings for GEOS
https://docs.rs/geos/
MIT License
122 stars 42 forks source link

WIP ConstCoordSeq #138

Open kylebarron opened 1 year ago

kylebarron commented 1 year ago

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.

GuillaumeGomez commented 1 year ago

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?

kylebarron commented 1 year ago

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 😅

GuillaumeGomez commented 1 year ago

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.