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

Update dependencies #48

Merged
merged 3 commits into from
Apr 9, 2023
Merged

Conversation

luke-biel
Copy link
Contributor

I went over and updated quick-xml dependency. Should fix issue #46 . There are also few clippy warnings, though they are regarding #[default] usage, which was stabilised quite recently, so I omited these.

Results of local benchmarks:

Main

Benchmarking parse (countries.kml): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 9.2s, enable flat sampling, or reduce sample count to 50.
parse (countries.kml)   time:   [1.8128 ms 1.8155 ms 1.8181 ms]                                   
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

parse (sample.kml)      time:   [200.94 µs 201.19 µs 201.43 µs]                               
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high severe

Updated dependencies:

Benchmarking parse (countries.kml): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 8.7s, enable flat sampling, or reduce sample count to 50.
parse (countries.kml)   time:   [1.7568 ms 1.7585 ms 1.7603 ms]
                        change: [-3.2529% -3.0962% -2.9302%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

parse (sample.kml)      time:   [184.79 µs 185.04 µs 185.30 µs]
                        change: [-8.2117% -8.0063% -7.8325%] (p = 0.00 < 0.05)
                        Performance has improved.

Ran on ryzen 5950x and 64 gigs of 3200 MT/s ram. Updated dependencies were consistently outperforming main branch.

I'd make sure that there are no issues / missed changes. Tests pass, and I tried to go through every change, but they were rather automatic so something might've slipped.

@pjsier
Copy link
Member

pjsier commented Apr 6, 2023

@luke-biel thanks for this! Looks like the only CI issue is the clippy warning you mentioned, and I'd be fine to ignore that for now and address in a separate ticket. I'm open to updating the CI command to ignore it explicitly so CI passes, but not essential

I think you're likely right that it would be possible to miss something so I'll be taking a closer look over the next couple days. At first glance it looks great, so it shouldn't be long, but feel free to remind me here if I haven't updated in a week or so

@luke-biel
Copy link
Contributor Author

luke-biel commented Apr 7, 2023

@pjsier So I checked and it appears that #[default] on enums was stabilized in 1.62 (https://blog.rust-lang.org/2022/06/30/Rust-1.62.0.html). It's much older than I initially remembered.

If you'd like to set MSRV policy, we should be able to fix clippy issues without silencing them. I don't mind doing this within this PR, just will need clarification on your side.
Usually rust crates decide for "rolling" MSRV, eg.: support last 3 stable releases. I'd suggest doing something similar. It's extremally difficult to keep a static MSRV as most of the ecosystem doesn't consider MSRV bump a breaking change.
If the MSRV would be set to current stable - 3, we're set, as it's 1.65, and rust already supports #[default].

@pjsier
Copy link
Member

pjsier commented Apr 9, 2023

Thanks for researching that! Let’s keep it separate for now and just ignore the warnings here. I’m fine with updating that in CI temporarily

I’d want to look more into an MSRV policy and otherwise pending a closer look this seems good to go

Copy link
Member

@pjsier pjsier left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'm gonna merge with the lints ignored, but you're welcome to open up another PR to address those

@pjsier pjsier merged commit 2c028ff into georust:main Apr 9, 2023
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.

2 participants