Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ryu for faster float-to-string conversion #118

Open
kylebarron opened this issue Sep 25, 2024 · 7 comments · May be fixed by #120
Open

Use ryu for faster float-to-string conversion #118

kylebarron opened this issue Sep 25, 2024 · 7 comments · May be fixed by #120

Comments

@kylebarron
Copy link
Member

kylebarron commented Sep 25, 2024

In geoarrow/geoarrow-rs#788 @b4l prototyped a custom implementation of writing to WKT, and he said

It is up to twice as fast as wkt and geo.

It doesn't look like this wkt crate uses ryu, and so I'm curious if that's a large part of why his implementation is faster. https://github.com/dtolnay/ryu seems to be a very popular library, and so it doesn't seem like an unreasonable dependency.

It looks like it should be relatively easy to swap in ryu and test if that's significantly faster? It looks like

wkt/src/types/coord.rs

Lines 37 to 44 in c3088cd

write!(f, "{} {}", self.x, self.y)?;
if let Some(z) = self.z {
write!(f, " {}", z)?;
}
if let Some(m) = self.m {
write!(f, " {}", m)?;
}
Ok(())
is the main place where floats are converted to strings.

I suppose the primary issue with using ryu is that ryu's Float trait supports only f32 and f64, while WktNum supports a broader range of types, including integers.

Assuming that ryu is indeed quite a bit faster, I think it's worth special casing f32 and f64 if we can, assuming that most real world data will be f64.

(For maintainability reasons, if geoarrow-rs can use wkt directly, that's much more appealing to me than rolling our own).

Any thoughts?

@michaelkirk
Copy link
Member

michaelkirk commented Sep 25, 2024 via email

@lnicola
Copy link
Member

lnicola commented Sep 25, 2024

This was discussed a little in rust-lang/rust#52811.

@kylebarron
Copy link
Member Author

I think that sounds really promising. I’d prefer not to lose support for integer geometries though. I wonder if there’s a way to accommodate both?

I'm not exactly sure how that would work because the existing implementation is using a blanket trait implementation.

So if I try to implement something also on ryu::Float:

impl<T> fmt::Display for Coord<T>
where
    T: WktNum + fmt::Display,
{
    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
        write!(f, "{} {}", self.x, self.y)?;
        if let Some(z) = self.z {
            write!(f, " {}", z)?;
        }
        if let Some(m) = self.m {
            write!(f, " {}", m)?;
        }
        Ok(())
    }
}

impl<T> fmt::Display for Coord<T>
where
    T: WktNum + ryu::Float + fmt::Display,
{
    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
        write!(f, "{} {}", self.x, self.y)?;
        if let Some(z) = self.z {
            write!(f, " {}", z)?;
        }
        if let Some(m) = self.m {
            write!(f, " {}", m)?;
        }
        Ok(())
    }
}

it errors with:

conflicting implementations of trait `std::fmt::Display` for type `types::coord::Coord<_>`

Is there some way to use a single implementation and tell whether that T implements ryu::Float?

@michaelkirk
Copy link
Member

michaelkirk commented Sep 25, 2024

I don't think we can solve it as phrased.

But maybe we could achieve what we want (fast float WKT while still allowing integer WKT) doing something like we did in geo for our integer vs. float Kernel implementations:
https://github.com/georust/geo/blob/68f80f851879dd58f146aae47dc2feeea6c83230/geo/src/lib.rs#L350

@kylebarron
Copy link
Member Author

That would essentially remove the blanket implementation and implement on the raw number types instead?

@michaelkirk
Copy link
Member

Right - I think that's a fair trade.

I have yet to encounter someone actually using any of our geo libraries for an exotic numeric type, but I'd be interested in seeing how hard it is to add support for them (or to give them the tools to add support for themselves) if we happen to break things for them.

@michaelkirk
Copy link
Member

To be clear, I'm suggesting we could break things for exotic numeric types and wait to see if anybody cares before we put a bunch of effort into supporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants