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

Stop emitting drop loops for every array length #134297

Closed
wants to merge 2 commits into from

Conversation

scottmcm
Copy link
Member

We can just unsize the array to a slice and drop that instead, reusing the same slice loop for every array length.

As part of this (and how I originally noticed this), use PtrMetadata instead of Len for the slice length, for another step closer to being able to remove Rvalue::Len.

Demonstration that yes, every slice length gets its own loop today: https://rust.godbolt.org/z/5EsPjPWv4

@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 14, 2024
@scottmcm
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
…<try>

Stop emitting drop loops for every array length

We can just unsize the array to a slice and drop that instead, reusing the same slice loop for every array length.

As part of this (and how I originally noticed this), use `PtrMetadata` instead of `Len` for the slice length, for another step closer to being able to remove `Rvalue::Len`.

Demonstration that yes, every slice length gets its own loop today: <https://rust.godbolt.org/z/5EsPjPWv4>
@bors
Copy link
Contributor

bors commented Dec 14, 2024

⌛ Trying commit f6cd47a with merge 41701b3...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 14, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2024
We can just unsize the array to a slice and drop that instead, reusing the same loop.

As part of this (and how I originally noticed this), use `PtrMetadata` instead of `Len` for the slice length, for another step closer to being able to remove `Rvalue::Len`.
@scottmcm scottmcm force-pushed the drop-arrays-by-unsizing branch from f6cd47a to aac57cf Compare December 14, 2024 07:03
@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Dec 14, 2024

⌛ Trying commit aac57cf with merge 4ce3fa5...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
…<try>

Stop emitting drop loops for every array length

We can just unsize the array to a slice and drop that instead, reusing the same slice loop for every array length.

As part of this (and how I originally noticed this), use `PtrMetadata` instead of `Len` for the slice length, for another step closer to being able to remove `Rvalue::Len`.

Demonstration that yes, every slice length gets its own loop today: <https://rust.godbolt.org/z/5EsPjPWv4>
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 14, 2024

💔 Test failed - checks-actions

@lqd
Copy link
Member

lqd commented Dec 14, 2024

(These failures within opt-dist running rustc-perf aren't super clear... I'll try to see how to improve that.)

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the drop-arrays-by-unsizing branch from f1c87ea to a5f9505 Compare December 15, 2024 04:03
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling mdbook-trpl v0.1.0 (/checkout/src/doc/book/packages/mdbook-trpl)
   Compiling rustbook v0.1.0 (/checkout/src/tools/rustbook)
    Finished `release` profile [optimized] target(s) in 25.92s
##[endgroup]
error: process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc /checkout/obj/build/bootstrap/debug/rustc -vV` (exit status: 101)
thread 'main' panicked at compiler/rustc_driver_impl/src/lib.rs:1587:6:
thread 'main' panicked at compiler/rustc_driver_impl/src/lib.rs:1587:6:
Unable to install ctrlc handler: System(Custom { kind: Other, error: EBADF })
   0:     0x7f5569025131 - <<std[dfe8e69f7cd2cb9c]::sys::backtrace::BacktraceLock>::print::DisplayBacktrace as core[390652ced2a3c436]::fmt::Display>::fmt
   1:     0x7f55690840b3 - core[390652ced2a3c436]::fmt::write
   2:     0x7f5569018009 - <std[dfe8e69f7cd2cb9c]::sys::pal::unix::stdio::Stderr as std[dfe8e69f7cd2cb9c]::io::Write>::write_fmt
   3:     0x7f5569024fd2 - <std[dfe8e69f7cd2cb9c]::sys::backtrace::BacktraceLock>::print
   3:     0x7f5569024fd2 - <std[dfe8e69f7cd2cb9c]::sys::backtrace::BacktraceLock>::print
   4:     0x7f5569027a31 - std[dfe8e69f7cd2cb9c]::panicking::default_hook::{closure#1}
   5:     0x7f55690277af - std[dfe8e69f7cd2cb9c]::panicking::default_hook
   6:     0x7f5564773518 - <alloc[c0599e64d5b5f875]::boxed::Box<rustc_driver_impl[efef9de6942c42c2]::install_ice_hook::{closure#0}> as core[390652ced2a3c436]::ops::function::Fn<(&dyn for<'a, 'b> core[390652ced2a3c436]::ops::function::Fn<(&'a std[dfe8e69f7cd2cb9c]::panic::PanicHookInfo<'b>,), Output = ()> + core[390652ced2a3c436]::marker::Sync + core[390652ced2a3c436]::marker::Send, &std[dfe8e69f7cd2cb9c]::panic::PanicHookInfo)>>::call
   7:     0x7f5569028448 - std[dfe8e69f7cd2cb9c]::panicking::rust_panic_with_hook
   8:     0x7f556902803e - std[dfe8e69f7cd2cb9c]::panicking::begin_panic_handler::{closure#0}
   9:     0x7f5569025759 - std[dfe8e69f7cd2cb9c]::sys::backtrace::__rust_end_short_backtrace::<std[dfe8e69f7cd2cb9c]::panicking::begin_panic_handler::{closure#0}, !>
  11:     0x7f556907fb00 - core[390652ced2a3c436]::panicking::panic_fmt
  12:     0x7f55690800f6 - core[390652ced2a3c436]::result::unwrap_failed
  13:     0x7f55646a1995 - rustc_driver_impl[efef9de6942c42c2]::main
  14:     0x55a786bda8a7 - rustc_main[22b90118f880c90b]::main
---
warning: the ICE couldn't be written to `/checkout/rustc-ice-2024-12-15T04_21_58-26907.txt`: Read-only file system (os error 30)

note: rustc 1.85.0-nightly (dd429d1e6 2024-12-15) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z on-broken-pipe=kill -Z unstable-options
query stack during panic:
end of query stack

Build completed unsuccessfully in 0:00:30
---
##[group]Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
WARN: currently no CI rustc builds have rustc debug assertions enabled. Please either set `rust.debug-assertions` to `false` if you want to use download CI rustc or set `rust.download-rustc` to `false`.
ERROR: Tool `book` was not recorded in tool state.
ERROR: Tool `nomicon` was not recorded in tool state.
ERROR: Tool `reference` was not recorded in tool state.
ERROR: Tool `rust-by-example` was not recorded in tool state.
ERROR: Tool `edition-guide` was not recorded in tool state.
ERROR: Tool `embedded-book` was not recorded in tool state.
  local time: Sun Dec 15 04:21:58 UTC 2024
  network time: Sun, 15 Dec 2024 04:21:59 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@scottmcm
Copy link
Member Author

scottmcm commented Dec 15, 2024

I have no idea what's happening here :/

Replaced with #134326

@scottmcm scottmcm closed this Dec 15, 2024
@scottmcm scottmcm deleted the drop-arrays-by-unsizing branch December 15, 2024 05:43
@lqd
Copy link
Member

lqd commented Dec 15, 2024

@scottmcm The collector failed with collector error: Cannot read lib dir to find components (the error handling is buggy there, I'll fix it in rust-lang/rustc-perf#2021). It failed because it was trying to read the "lib" directory in the sysroot but couldn't: stage2 rustc panicked when asked to print the sysroot:

collector error: Cannot find libdir for rustc

Caused by:
    rustc failed to provide sysroot, error: exit status: 101, stderr:
    thread 'main' panicked at compiler/rustc_driver_impl/src/lib.rs:1587:6:
    Unable to install ctrlc handler: System(Custom { kind: Other, error: EBADF })
    stack backtrace:
       0: rust_begin_unwind
       1: core::panicking::panic_fmt
       2: core::result::unwrap_failed
       3: rustc_driver_impl::install_ctrlc_handler
       4: rustc_driver_impl::main
       5: rustc_main::main
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

    error: the compiler unexpectedly panicked. this is a bug.

    note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

    note: please make sure that you have updated to the latest nightly

    note: please attach the file at `/tmp/tmp-multistage/opt-artifacts/rustc-perf/rustc-ice-2024-12-15T17_44_07-54539.txt` to your bug report

    query stack during panic:
    end of query stack

(I've launched a try build with the other PR, to see if these errors reproduce)

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2024
…, r=<try>

Use `PtrMetadata` instead of `Len` in slice drop shims

I tried to do a bigger change in rust-lang#134297 which didn't work, so here's the part I really wanted: Removing another use of `Len`, in favour of `PtrMetadata`.

Reusing the same reviewer from the last one:
r? BoxyUwU
@scottmcm
Copy link
Member Author

Thanks for digging in, @lqd ! I think I'm not monoing enough here, or something :/

@lqd
Copy link
Member

lqd commented Dec 16, 2024

The good news is this behavior doesn’t seem to reproduce in the #134326 subset 🎉

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
…, r=saethlin

Use `PtrMetadata` instead of `Len` in slice drop shims

I tried to do a bigger change in rust-lang#134297 which didn't work, so here's the part I really wanted: Removing another use of `Len`, in favour of `PtrMetadata`.

Split into two commits where the first just adds a test, so you can look at the second commit to see how the drop shim for an array changes with this PR.

Reusing the same reviewer from the last one:
r? BoxyUwU
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
…, r=saethlin

Use `PtrMetadata` instead of `Len` in slice drop shims

I tried to do a bigger change in rust-lang#134297 which didn't work, so here's the part I really wanted: Removing another use of `Len`, in favour of `PtrMetadata`.

Split into two commits where the first just adds a test, so you can look at the second commit to see how the drop shim for an array changes with this PR.

Reusing the same reviewer from the last one:
r? BoxyUwU
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants