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

Port the bulk of the process_execution crate to async/await #9676

Merged

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented May 1, 2020

Problem

We're planning to instrument process execution with additional workunits, but it can be challenging to identify critical sections in the presence of combinators.

Solution

async/await for the bulk of the process_execution crate.

Stu Hood added 2 commits April 30, 2020 22:45
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
@stuhood stuhood requested review from illicitonion, pierrechevalier83, gshuflin and jsirois and removed request for illicitonion May 1, 2020 07:07
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Excellent!

@@ -24,11 +24,11 @@
#![allow(clippy::new_without_default, clippy::new_ret_no_self)]
// Arc<Mutex> can be more clear than needing to grok Orderings:
#![allow(clippy::mutex_atomic)]
#![type_length_limit = "7000100"]
#![type_length_limit = "32185118"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? It looks like we're now 32x times the default: https://doc.rust-lang.org/reference/attributes/limits.html#the-type_length_limit-attribute

Copy link
Sponsor Member Author

@stuhood stuhood May 1, 2020

Choose a reason for hiding this comment

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

async/await generates very large per-method types that describe an anonymous struct holding the state machine for the run... luckily you never see them (in error messages, for example). But they're there.

I went down a bit of a rabbit hole looking for something clearly stating that this is fine, but didn't find anything that definitive. The best I could find is that they're continuing to try and improve it: rust-lang/rust#46477

}
})
.and_then(
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. What a difference 👏

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The final boss, heh.

@stuhood stuhood merged commit 23cfc97 into pantsbuild:master May 1, 2020
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