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

Replace thread_local with generator resume arguments in box_region. #70622

Closed
wants to merge 1 commit into from

Conversation

gizmondo
Copy link
Contributor

Fixes #68922.

r? @ghost

@gizmondo
Copy link
Contributor Author

This is a plain replacement. @jonas-schievink, did you have something more comprehensive in mind?

@jonas-schievink
Copy link
Contributor

Seems like a good start, but to be honest, I have no idea what this code is supposed to do (other than that it can now use resume arguments to do it).

r? @Zoxc

@gizmondo gizmondo marked this pull request as ready for review March 31, 2020 19:00
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 1, 2020

⌛ Trying commit aa92d26 with merge eeebed0947b040d9b65683d52071a94d926a398b...

@bjorn3
Copy link
Member

bjorn3 commented Apr 1, 2020

box_region is only used in librustc_interface, so it is not performance critical.

declare_box_region_type!(
pub BoxedResolver,
for(),
(&mut Resolver<'_>) -> (Result<ast::Crate>, ResolverOutputs)
);

@bors
Copy link
Contributor

bors commented Apr 1, 2020

☀️ Try build successful - checks-azure
Build commit: eeebed0947b040d9b65683d52071a94d926a398b (eeebed0947b040d9b65683d52071a94d926a398b)

@rust-timer
Copy link
Collaborator

Queued eeebed0947b040d9b65683d52071a94d926a398b with parent 99009bf, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit eeebed0947b040d9b65683d52071a94d926a398b, comparison URL.

@gizmondo
Copy link
Contributor Author

gizmondo commented Apr 3, 2020

Btw there was a discussion that resume arguments can allow to get rid of unsafety in this module.
But I haven't figured out a way to do it. I think for that to be possible resume should be able to borrow its arguments for the duration of a single call. I.e. something like that should work:

let mut gen = Box::pin(static move |mut arg: &mut i32| {
    loop {
        *arg += 1;
        arg = yield;
    }
});
let mut a = 1;
Pin::new(&mut gen).resume(&mut a);

Currently it doesn't compile. I'm not sure if I'm doing something wrong or it's just not supported yet.

@petrochenkov
Copy link
Contributor

Since you are working on this, could you perhaps add some docs explaining what the "box region" does and why is it necessary? Right now it's entirely unclear.

@gizmondo
Copy link
Contributor Author

gizmondo commented Apr 3, 2020

I'll try.

@jonas-schievink
Copy link
Contributor

@gizmondo for that to work you'd need #68923, which is unfortunately very hard to solve

@nikomatsakis
Copy link
Contributor

r? @jonas-schievink

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Okay, looks basically good to me @gizmondo!

If you want to add some more comments to this module, feel free. Otherwise r=me with the more detailed panic message below.

};
}
$crate::box_region::Action::Complete => break,
$crate::box_region::Action::Initial => panic!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$crate::box_region::Action::Initial => panic!(),
$crate::box_region::Action::Initial => bug!("unexpected box_region action: {:?}", $action),

@jonas-schievink jonas-schievink added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 10, 2020
@joelpalmer joelpalmer added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2020
@joelpalmer
Copy link

Ping from Triage: Closed due to inactivity. Please re-open with updates. Thanks for the PR! @gizmondo

@joelpalmer joelpalmer closed this Apr 21, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 25, 2020
Replace thread_local with generator resume arguments in box_region.

Fixes rust-lang#68922.

Continuation of rust-lang#70622. Added a short doc, hope it makes sense.

r? @jonas-schievink
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 25, 2020
Replace thread_local with generator resume arguments in box_region.

Fixes rust-lang#68922.

Continuation of rust-lang#70622. Added a short doc, hope it makes sense.

r? @jonas-schievink
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite box_region.rs to use generator resume arguments
9 participants