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

Validate output of decoding #45

Merged
merged 2 commits into from
May 9, 2024
Merged

Validate output of decoding #45

merged 2 commits into from
May 9, 2024

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented May 8, 2024

update: merged! Depends on #43, so please review that first.

Supersedes #40

FIXES #39 and #37 with less performance regression.

$ cargo bench --bench="*" -- --baseline=sha-4cc9ff0
    Finished `bench` profile [optimized] target(s) in 0.03s
     Running benches/benchmarks.rs (target/release/deps/benchmarks-8b91c88197106af3)
encode 10_000 coordinates at precision 1e-5
                        time:   [106.15 µs 106.34 µs 106.53 µs]
                        change: [+0.3976% +0.6082% +0.8269%] (p = 0.00 < 0.05)
                        Change within noise threshold.

# I'm assuming this is spurious since I haven't touched the encode codepath but 🤷 
encode 10_000 coordinates at precision 1e-6
                        time:   [129.93 µs 130.66 µs 131.30 µs]
                        change: [+1.3900% +2.1013% +2.8347%] (p = 0.00 < 0.05)
                        Performance has regressed.

decode 10_000 coordinates at precision 1e-5
                        time:   [88.225 µs 89.156 µs 90.035 µs]
                        change: [+6.7607% +8.0710% +9.4026%] (p = 0.00 < 0.05)
                        Performance has regressed.

decode 10_000 coordinates at precision 1e-6
                        time:   [111.01 µs 111.91 µs 112.82 µs]
                        change: [+18.163% +19.518% +20.903%] (p = 0.00 < 0.05)
                        Performance has regressed.

@mattiZed - Thanks for the inspiration - I've incorporated some of your code into 0d827dd, but I've omitted the LUT stuff for now. I wasn't able to convince myself that that part was an improvement yet.

I will note: some of this code is super touchy. Tiny, seemingly irrelevant changes, would result in 50% swings in bench performance. Though my benchmark setup is crude, I've run the benchmarks back and forth enough to convince myself that this is not just noise.

@michaelkirk michaelkirk mentioned this pull request May 8, 2024
@michaelkirk michaelkirk marked this pull request as draft May 8, 2024 22:17
michaelkirk and others added 2 commits May 8, 2024 16:19
This introduces a serious performance regression for decode:

$ cargo bench --bench="*" -- --baseline=perf2
   Compiling polyline v0.10.2 (/Users/mkirk/src/georust/polyline)
    Finished `bench` profile [optimized] target(s) in 0.77s
     Running benches/benchmarks.rs (target/release/deps/benchmarks-cc8f3ea04be06866)
encode 10_000 coordinates at precision 1e-5
                        time:   [105.25 µs 105.30 µs 105.37 µs]
                        change: [-0.1263% -0.0235% +0.0878%] (p = 0.68 > 0.05)
                        No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
  2 (2.00%) high severe

encode 10_000 coordinates at precision 1e-6
                        time:   [129.27 µs 130.11 µs 130.80 µs]
                        change: [-0.9966% -0.2021% +0.6397%] (p = 0.63 > 0.05)
                        No change in performance detected.

decode 10_000 coordinates at precision 1e-5
                        time:   [164.64 µs 165.83 µs 167.02 µs]
                        change: [+109.57% +111.90% +114.24%] (p = 0.00 < 0.05)
                        Performance has regressed.

decode 10_000 coordinates at precision 1e-6
                        time:   [173.15 µs 174.21 µs 175.17 µs]
                        change: [+85.462% +87.344% +89.230%] (p = 0.00 < 0.05)
                        Performance has regressed.

Co-authored-by: mattiZed <[email protected]>
It's still slower than before the validation checks, but now only 10-20% slower rather than 90-110% slower

$ cargo bench --bench="*" -- --baseline=perf2
   Compiling polyline v0.10.2 (/Users/mkirk/src/georust/polyline)
    Finished `bench` profile [optimized] target(s) in 0.77s
     Running benches/benchmarks.rs (target/release/deps/benchmarks-cc8f3ea04be06866)
encode 10_000 coordinates at precision 1e-5
                        time:   [105.11 µs 105.16 µs 105.23 µs]
                        change: [-0.2235% -0.1070% +0.0226%] (p = 0.09 > 0.05)
                        No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

encode 10_000 coordinates at precision 1e-6
                        time:   [123.98 µs 124.32 µs 124.77 µs]
                        change: [-3.0982% -2.4787% -1.8225%] (p = 0.00 < 0.05)
                        Performance has improved.

decode 10_000 coordinates at precision 1e-5
                        time:   [86.887 µs 87.835 µs 88.768 µs]
                        change: [+10.484% +11.820% +13.207%] (p = 0.00 < 0.05)
                        Performance has regressed.

decode 10_000 coordinates at precision 1e-6
                        time:   [110.52 µs 111.44 µs 112.35 µs]
                        change: [+19.484% +20.773% +22.034%] (p = 0.00 < 0.05)
                        Performance has regressed.
@michaelkirk michaelkirk force-pushed the mkirk/correctness branch from 7c1dd92 to c57292a Compare May 8, 2024 23:19
@michaelkirk michaelkirk marked this pull request as ready for review May 8, 2024 23:24
@michaelkirk michaelkirk requested a review from urschrei May 8, 2024 23:24
Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that this is touchy – there's nothing obviously "weird" in the new code. I'm not at my machine, and in any case it's a similar setup to yours iirc, so I can't confirm.

@michaelkirk
Copy link
Member Author

michaelkirk commented May 9, 2024

Here's an example of the touchiness:

One tiny change to interpolate an existing var into the err message. We're not ever hitting this branch at runtime, so presumably it has some outsized effects on... locality... or register use.. ? 🤷 (surely not a compiler bug... right???).

Whatever it is, I can reliably reproduce it with this change. And there were a couple other small changes which you'd think innocuous, but tanked performance. So I had to be very careful while making these changes.

[17:15:26] mulberry arm64 ~/src/georust/polyline (mkirk/correctness)
$ GIT_PAGER=less git diff       
diff --git a/src/lib.rs b/src/lib.rs
index 09f9cea..4307337 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -151,7 +151,7 @@ fn decode_next(
     let mut result = 0;
     while let Some((idx, mut byte)) = chars.next() {
         if byte < 63 || (shift > 64 - 5) {
-            return Err(format!("Cannot decode character at index {idx}"));
+            return Err(format!("Cannot decode character at index {idx}, byte: {byte}"));
         }
         byte -= 63;
         result |= ((byte & 0x1f) as u64) << shift;
[17:15:29] mulberry arm64 ~/src/georust/polyline (mkirk/correctness)
$ cargo bench --bench="*" -- --baseline=sha-4cc9ff0

   Compiling polyline v0.10.2 (/Users/mkirk/src/georust/polyline)
    Finished `bench` profile [optimized] target(s) in 0.91s
     Running benches/benchmarks.rs (target/release/deps/benchmarks-8b91c88197106af3)
encode 10_000 coordinates at precision 1e-5
                        time:   [105.82 µs 106.10 µs 106.39 µs]
                        change: [+0.0992% +0.3137% +0.5416%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
  8 (8.00%) high mild

encode 10_000 coordinates at precision 1e-6
                        time:   [131.05 µs 131.50 µs 131.92 µs]
                        change: [+2.3540% +2.9801% +3.6276%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) low mild

# +75% vs. +10% 🔥🔥🔥 
decode 10_000 coordinates at precision 1e-5
                        time:   [143.63 µs 144.36 µs 145.13 µs]
                        change: [+73.357% +75.179% +77.021%] (p = 0.00 < 0.05)
                        Performance has regressed.

# +67% vs. +20% 🔥🔥🔥 
decode 10_000 coordinates at precision 1e-6
                        time:   [157.42 µs 158.13 µs 158.81 µs]
                        change: [+65.843% +67.560% +69.225%] (p = 0.00 < 0.05)
                        Performance has regressed.

I really don't understand it, but I don't understand a lot of things.

@michaelkirk michaelkirk added this pull request to the merge queue May 9, 2024
Merged via the queue into main with commit 451f638 May 9, 2024
1 check passed
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 this pull request may close these issues.

Error handling of bad polyline still broken
2 participants