georust / polyline Goto Github PK

The https://docs.rs/flexpolyline/0.1.0/flexpolyline/ crate runs the bench_encode benchmark in around 19 µs 330 ns on my machine, vs 77 µs for our decode function. That's in the 3x ballpark. They seem to use a lot of lookups, which I imagine accounts for the difference, but it would be good to investigate.

Hi!

First of, thanks a lot for your work on this crate!

I am experiencing a bug when using georust/polyline through a python wrapper called pypolyline.

If that's okay, I will link to the issue that I filed in their repository which contains the necessary information.

To repeat and summarize some of the information I gave on that other issue:

• decoding logic has been refactored from v0.7.1 to v0.10.0
• in v0.7.1, decoding logic would protect against an out-of-bounds access into the chars buffer
• in v0.10.0 this protection seems to be missing, so the rust binary that is utilized from pypolyline may crash unrecoverably

Any help very much appreciated.

Let me know if you need more information.

Best,
Stefan

Our fix in #37 has not worked: trying to decode the polyline 'ugh_ugh' gives an out of bounds error on line 142:

index out of bounds: the len is 7 but the index is 7.

Right now, encode_coordinates looks like:

pub fn encode_coordinates(coordinates: &[[f64; 2]], precision: u32) -> Result<String, String> {
    ...
}

We don't need the entire slice at once, we just need anything that can iterate over coordinates:

pub fn encode_coordinates<C>(coordinates: C, precision: u32) -> Result<String, String>
where
    C: Iterator<Item=[f64; 2]>
{
    ...
}

And maybe it makes sense to pull in the geo-types crate to reuse our common Coordinate construct:

pub fn encode_coordinates<C, T>(coordinates: C, precision: u32) -> Result<String, String>
where
    C: Iterator<Item=Coordinate<T>>
{
    ...
}

The geo_types should be re-exported so that client code easily can utilize those types.

👋 Hi @tmcw, you are the only "user-owner" of the polyline crate on crates.io, so only you can edit that crate's owners on crates.io.

Following up on georust/meta#21, I created a new polyline-publishers team for publishing to crates.io with you as maintainer.

Could you please update the crate owners? Specifically:

• update polyline crate owners on crates.io:

cd polyline

# 💥 remove large publishing group
cargo owner --remove github:georust:core

# 🔐 add small publishing group
cargo owner --add github:georust:polyline-publishers

# you're currently the only user-owner of the crate. Consider adding another user-owner, 
# that way if for some reason you become inaccessible we have someone else who can edit the owners. 
# See https://github.com/rust-lang/crates.io/issues/2906
cargo owner --add <username>

# make sure everything looks good 👀 
cargo owner --list

If there's anyone else you'd like to be able to publish the crate, feel free to add them to your publishing team, or as a user-owner. Consider that user-owners, as opposed to owners-via-team, can themselves edit the crate's owners, which could be desirable, or not, depending.

In particular, it looks like @urschrei has been actively contributing to GH. Once you switch the publishing team, @urschrei (or anyone else) won't be able to publish until either they are added back to the publishing team or added as a user owner.

Let me know if you have any questions, or you can review georust/meta#21.

I have a large polyline6 which seems to trigger an overflow in the decoder. It's recreated with the following patch
overflow.patch.txt

cargo test then produces

test tests::should_not_trigger_overflow ... FAILED

failures:

---- tests::should_not_trigger_overflow stdout ----
thread 'tests::should_not_trigger_overflow' panicked at 'attempt to subtract with overflow', src/lib.rs:129:20


failures:
    tests::should_not_trigger_overflow

test result: FAILED. 4 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 7.76s

error: test failed, to rerun pass `--lib````

Although polyline also uses LineStrings, these can be primitively represented without too much trouble. Should we thus adopt a similar strategy to the proj crate, enabling geo-types compatibility by default but allowing a more simple option for consumers who don't want or need the overhead?

rust-lang/crates.io@9b65b4c

https://doc.rust-lang.org/cargo/reference/manifest.html#the-categories-field

I regularly see 10% swings in performance. Typically if I run the benches 3 times, I'll see a spread of at least 10%.

Is anyone else seeing this? I haven't yet figured out why, but it renders the benchmarks nearly useless if you can't safely measure a 10% change in behavior.

P.S. I noticed this while trying to evaluate #37