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.

Sign up for GitHub

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

Jump to bottom

Exponential Performance Regression in Rust 1.73 Read API from BorrowedCursor::ensure_init #117545

Closed
PSeitz opened this issue Nov 3, 2023 · 5 comments · Fixed by #117576
Closed

Exponential Performance Regression in Rust 1.73 Read API from BorrowedCursor::ensure_init #117545

PSeitz opened this issue Nov 3, 2023 · 5 comments · Fixed by #117576
Assignees
@the8472
Labels
C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@PSeitz
Copy link
Contributor

PSeitz commented Nov 3, 2023 •
edited by rustbot
Loading

Reported by a lz4_flex user PSeitz/lz4_flex#147

Code

I created a small repo to demonstrate on a minimal example.
https://github.com/PSeitz/copy_regression_1.73

BorrowedCursor::ensure_init from this PR #97015 seems exhibit exponential cost.

The program creates a Vec and reads it in chunks of 32kb. Size is specified as a command line argument (100MB-400MB).

time cargo run --release 100000000 (If your default is 1.73)

Executed in    3.84 secs    fish           external
   usr time    3.80 secs  268.00 micros    3.80 secs
   sys time    0.03 secs  262.00 micros    0.03 secs

time cargo run --release 200000000

Executed in   16.90 secs    fish           external
   usr time   16.82 secs  626.00 micros   16.82 secs
   sys time    0.04 secs    0.00 micros    0.04 secs

time cargo run --release 400000000

Executed in   72.84 secs    fish           external
   usr time   72.65 secs  586.00 micros   72.65 secs
   sys time    0.06 secs  138.00 micros    0.06 secs

time cargo +1.72 run --release 100000000

Executed in   57.23 millis    fish           external
   usr time   16.83 millis  404.00 micros   16.43 millis
   sys time   37.02 millis    0.00 micros   37.02 millis

time cargo +1.72 run --release 400000000

Executed in  257.30 millis    fish           external
   usr time  150.46 millis  477.00 micros  149.98 millis
   sys time  106.64 millis  112.00 micros  106.53 millis
@PSeitz PSeitz added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Nov 3, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Nov 3, 2023
@lqd
Copy link
Member

lqd commented Nov 3, 2023

Maybe try cargo-bisect-rustc (tutorial: https://rust-lang.github.io/cargo-bisect-rustc/) to find which PR caused this?

@saethlin saethlin added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 3, 2023
@PSeitz
Copy link
Contributor Author

PSeitz commented Nov 3, 2023 •
edited
Loading 

********************************************************************************
Regression in nightly-2023-07-10
********************************************************************************

fetching https://static.rust-lang.org/dist/2023-07-09/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2023-07-09: 40 B / 40 B [==============================================================================================================================================] 100.00 % 807.24 KB/s 
converted 2023-07-09 to 83964c156db1f444050a38b2498dbd0da6d5d503
fetching https://static.rust-lang.org/dist/2023-07-10/channel-rust-nightly-git-commit-hash.txt
nightly manifest 2023-07-10: 40 B / 40 B [==============================================================================================================================================] 100.00 % 571.61 KB/s 
converted 2023-07-10 to 1065d876cdbc34a872b9e17c78caaa59ea0c94d4
looking for regression commit between 2023-07-09 and 2023-07-10
cloning rust repository
...
searched nightlies: from nightly-2022-11-01 to nightly-2023-11-01
regressed nightly: nightly-2023-07-10
searched commit range: https://github.com/rust-lang/rust/compare/83964c156db1f444050a38b2498dbd0da6d5d503...1065d876cdbc34a872b9e17c78caaa59ea0c94d4
regressed commit: https://github.com/rust-lang/rust/commit/a9eba8d793696d75c51a87ca731983bf5967028f

Really nice bisect tool.
From the commit list in that Auto merge it seems to be #113493

@lqd
Copy link
Member

lqd commented Nov 3, 2023

cc author @the8472

@the8472
Copy link
Member

the8472 commented Nov 3, 2023 •
edited
Loading

The loop creates a new BorrowedBuf on each iteration, which is fine if the reader can fill the provided buffer in one go but does unnecessary work on short reads. Should be easy to fix.

@rustbot claim

@the8472 the8472 mentioned this issue Nov 4, 2023
Merged
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 4, 2023
@bors bors closed this as completed in 7a892ab Nov 6, 2023
@bonsairobo
Copy link

Any chance of getting this fix in a backport to 1.73?

@mikeder mikeder mentioned this issue Nov 19, 2023
Merged
mikeder added a commit to mikeder/turtletime that referenced this issue Nov 19, 2023
@mikeder
feat: update bevy to 0.12 (#3)
8f0e974 
1. Update Bevy to 0.12
2. Update related deps ( ggrs, bevy_ggrs, matchbox, etc. )
3. Log warning if winit cannot find main window to avoid panic - I don't
understand why this changed see below.
4. Switch to `1.72` toolchain due to Rust compiler regression.

### Winit Panic

Some new issue was introduced with the Bevy 0.12 upgrade, this line
started panicking, so we just log a warning and abort if we can't get
the primary window.


https://github.com/mikeder/turtletime/pull/3/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcL83

NiklasEi/bevy_game_template#80

### Compiler Regression

I was unable to compile `bevy-egui-inspector` after updating to Rust to
the 1.74 toolchain, it turns out there was a compiler regression so I've
pinned my toolchain to 1.72 ( there was [another
regression](rust-lang/rust#117545) in 1.73
that sounds nasty enough to avoid for now too )

jakobhellermann/bevy-inspector-egui#163
rust-lang/rust#117976

#### Segmentation Fault

Originally I tried to use the `nightly` toolchain as recommended in the
above `bevy-egui-inspector` issue, but when I did, my game instantly
crashed with a segmentation fault. I tried to use the
[sanitizer](https://doc.rust-lang.org/beta/unstable-book/compiler-flags/sanitizer.html)
features to debug it, but the game compiled and ran just fine with the
sanitizer flags. Ultimately I just downgraded to `1.72` and moved on
with my life.
@bonsairobo bonsairobo mentioned this issue Jun 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@the8472 the8472

Labels
C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
7 participants
@lqd @the8472 @PSeitz @bonsairobo @apiraino @saethlin @rustbot