Switch to ryu for float to string #120
Conversation
|
I'm not convinced this is a good idea. From the rustc issue:
Also: https://xkcd.com/2170/. |
|
Since this is seemingly a straight forward performance proposal, we should be able to measure it. Can you include any benchmarks using plausible real world data that shows improvement? Related: #121 Note: There were no measurable perf differences for me with the existing write benchmark. It's likely that the existing data we've included in the benchmarks is not representative of real world data, so I'd be happy to include different data in the benchmarks if it's useful to show the improvements here. That said, if we have to contrive really crazy data to get a measurable perf improvement — e.g. if it's only an improvement for people providing gigabytes of molecular level GPS precision (as an extreme example), and a regression for everybody else, then obviously we shouldn't do it. |
| @@ -42,11 +43,12 @@ where | |||
| /// | |||
| /// let point: Point<f64> = point!(x: 1., y: 2.); | |||
| /// | |||
| /// assert_eq!(point.wkt_string(), "POINT(1 2)"); | |||
| /// assert_eq!(point.wkt_string(), "POINT(1.0 2.0)"); | |||
There was a problem hiding this comment.
I don't know how to feel about this change in behavior. On the one hand it makes our serialization less efficient which is bad, OTOH it conveys to the reader that this was serialized from floating point, so I guess that's useful.
There was a problem hiding this comment.
In the interest of not leaving ambiguous comments, I'll say I prefer the old version, which serialized efficiently, but I'd be willing to let that small demerit go if there are substantial other wins.
That said, if there's a way to use faster float serialization while still maintaining the integer display for .0, I think that'd be best.
There was a problem hiding this comment.
ryu doesn't have any options as far as I can tell. So if we switched to ryu I don't think we'd be able to serialized floats without the .0
There was a problem hiding this comment.
We can still check if the string ends with .0 and strip that off.
| let mut buffer = ryu::Buffer::new(); | ||
| let y = buffer.format(self.y); | ||
|
|
||
| write!(f, "{} {}", x, y)?; |
There was a problem hiding this comment.
Nit: I suspect a sequence of f.write_str calls would be a little faster here.
There was a problem hiding this comment.
Possibly, it might muddy the waters on perf improvements though. I'd prefer to keep everything except ryu the same in this PR.
We could set up Criterion, I guess. And it might be worth comparing to |
What do you mean? We're already using criterion AFAICT. |
Not my finest day 🤦. |
CHANGES.mdif knowledge of this change could be valuable to users.I'm not sure that this is using ryu for all geometry types, because I had to update all the point/linestring tests because ryu was printing out
1.0instead of1. But I didn't have to change the polygon tests:wkt/src/types/polygon.rs
Line 157 in e1d838d
Closes #118