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

io: remove unsafe pin-projections and remove manual Unpin implementations #1588

Merged
merged 7 commits into from
Sep 24, 2019
Merged

io: remove unsafe pin-projections and remove manual Unpin implementations #1588

merged 7 commits into from
Sep 24, 2019

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Sep 22, 2019

Based on #1586 (First 2 commits are the same as #1586)

This removes most pin-projection related unsafe code: 16b06e3

And removes manual Unpin implementations and add tests to check that Unpin requirement does not change accidentally. see commit message for more: 7ad962d

As references always implement Unpin, there is no need to implement
Unpin manually.

Also, changing Unpin requirements will be breaking changes, so add tests
to check for Unpin requirements.
@taiki-e taiki-e marked this pull request as ready for review September 22, 2019 16:56
@carllerche
Copy link
Member

Thanks for this. Do we know how much this impacts compile time?

@taiki-e
Copy link
Member Author

taiki-e commented Sep 22, 2019

I didn’t actually time it, but the master branch already depends on pin-project, so I think merging this should have little impact on compile time.

Impact of adding pin-project to dependencies: As pin-project is a proc-macro, it increases the compilation time. If the dependencies have other proc-macro crates, the impact will not be so big. (As far as I know, whether or not the "full" feature of syn is enabled will particularly affect compilation time. In fact, a crate has been created to handle this issue. pin-project enables "full" feature.)

@taiki-e
Copy link
Member Author

taiki-e commented Sep 22, 2019

@carllerche I found the useful tool -Z timings: https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975

Build times (clean build):

master:
master

this pr:
pr

without pin-project (613fde2)
613fde2

Dependencies are compiled in parallel, so I don't think this has much impact if another proc-macro crate (tokio-macros, tracing-attribute, etc.) is already enabled.

tokio-io/src/io/take.rs Outdated Show resolved Hide resolved
tokio-io/src/io/chain.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e merged commit c81447f into tokio-rs:master Sep 24, 2019
@taiki-e taiki-e deleted the io-safe-pin-projection branch September 24, 2019 16:17
@taiki-e
Copy link
Member Author

taiki-e commented Oct 22, 2019

FWIW: I have written a lightweight version of pin-project written with declarative macros: https://github.com/taiki-e/pin-project-lite

@carllerche
Copy link
Member

@taiki-e btw, how did you gen that graph?

@taiki-e
Copy link
Member Author

taiki-e commented Oct 22, 2019

@carllerche I used -Z timings flag of cargo: cargo build -Z timings

@carllerche
Copy link
Member

Oh, nice, i missed all that 👍

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.

3 participants