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

Initialize channel Blocks directly on the heap #132738

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Nov 7, 2024

The channel's Block::new was causing a stack overflow because it held
32 item slots, instantiated on the stack before moving to Box::new.
The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

Fixes #102246

try-job: test-various

@rustbot

This comment was marked as outdated.

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 7, 2024
@cuviper
Copy link
Member Author

cuviper commented Nov 7, 2024

r? libs

@rustbot rustbot assigned thomcc and unassigned estebank Nov 7, 2024
@cuviper cuviper removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 7, 2024
@rust-log-analyzer

This comment has been minimized.

The channel's `Block::new` was causing a stack overflow because it held
32 item slots, instantiated on the stack before moving to `Box::new`.
The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

Fixes rust-lang#102246
@ibraheemdev
Copy link
Member

Can a corresponding PR be opened in the crossbeam-channel repository to avoid things getting out of sync?

@cuviper
Copy link
Member Author

cuviper commented Nov 7, 2024

Well, I think crossbeam may not like MSRV 1.82 for Box::assume_init, and Box::new_zeroed is still unstable. The latter could otherwise use Box::new_uninit and write a few manual zeros though.

edit: I've got something working with manual alloc_zeroed, so yes, I'll send a PR to crossbeam.

@ibraheemdev
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 7, 2024

📌 Commit 03383ad has been approved by ibraheemdev

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2024
cuviper added a commit to cuviper/crossbeam that referenced this pull request Nov 7, 2024
The list channel's `Block::new` was causing a stack overflow because it
held 32 item slots, instantiated on the stack before moving to
`Box::new`. The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

References from the `std` channel implementation:
* rust-lang/rust#102246
* rust-lang/rust#132738
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2024
…eemdev

Initialize channel `Block`s directly on the heap

The channel's `Block::new` was causing a stack overflow because it held
32 item slots, instantiated on the stack before moving to `Box::new`.
The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

Fixes rust-lang#102246
cuviper added a commit to cuviper/crossbeam that referenced this pull request Nov 7, 2024
The list channel's `Block::new` was causing a stack overflow because it
held 32 item slots, instantiated on the stack before moving to
`Box::new`. The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

References from the `std` channel implementation:
* rust-lang/rust#102246
* rust-lang/rust#132738
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 7, 2024
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee
Copy link
Member

@bors r-

failed in #132751 (comment)

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2024
@workingjubilee
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Nov 8, 2024

⌛ Trying commit 03383ad with merge 9564052...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
Initialize channel `Block`s directly on the heap

The channel's `Block::new` was causing a stack overflow because it held
32 item slots, instantiated on the stack before moving to `Box::new`.
The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

Fixes rust-lang#102246

try-job: test-various
taiki-e pushed a commit to crossbeam-rs/crossbeam that referenced this pull request Nov 8, 2024
The list channel's `Block::new` was causing a stack overflow because it
held 32 item slots, instantiated on the stack before moving to
`Box::new`. The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

References from the `std` channel implementation:
* rust-lang/rust#102246
* rust-lang/rust#132738
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 8, 2024

💔 Test failed - checks-actions

@cuviper
Copy link
Member Author

cuviper commented Nov 8, 2024

Added needs-threads to that test.

@bors r=ibraheemdev

@bors
Copy link
Contributor

bors commented Nov 8, 2024

📌 Commit 9827c6d has been approved by ibraheemdev

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2024
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason this can't be a std unit test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get the old code to fail that way, I think because std tests are still built with optimization, which may be able avoid the stack slot altogether.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#132161 ([StableMIR] A few fixes to pretty printing)
 - rust-lang#132389 (coverage: Simplify parts of coverage graph creation)
 - rust-lang#132452 (coverage: Extract safe FFI wrapper functions to `llvm_cov`)
 - rust-lang#132590 (Simplify FFI calls for `-Ztime-llvm-passes` and `-Zprint-codegen-stats`)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4b904ce into rust-lang:master Nov 8, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
Rollup merge of rust-lang#132738 - cuviper:channel-heap-init, r=ibraheemdev

Initialize channel `Block`s directly on the heap

The channel's `Block::new` was causing a stack overflow because it held
32 item slots, instantiated on the stack before moving to `Box::new`.
The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

Fixes rust-lang#102246

try-job: test-various
@rustbot rustbot added this to the 1.84.0 milestone Nov 8, 2024
@cuviper cuviper deleted the channel-heap-init branch November 8, 2024 17:36
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…eemdev

Initialize channel `Block`s directly on the heap

The channel's `Block::new` was causing a stack overflow because it held
32 item slots, instantiated on the stack before moving to `Box::new`.
The 32x multiplier made modestly-large item sizes untenable.

That block is now initialized directly on the heap.

Fixes rust-lang#102246

try-job: test-various
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#132161 ([StableMIR] A few fixes to pretty printing)
 - rust-lang#132389 (coverage: Simplify parts of coverage graph creation)
 - rust-lang#132452 (coverage: Extract safe FFI wrapper functions to `llvm_cov`)
 - rust-lang#132590 (Simplify FFI calls for `-Ztime-llvm-passes` and `-Zprint-codegen-stats`)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow for big types in mpsc channels on separate thread
9 participants