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

Only build cdylib when the FFI cfg is active #2763

Closed
wants to merge 1 commit into from

Conversation

lqd
Copy link
Contributor

@lqd lqd commented Feb 23, 2022

Hi,

as part of the initiative to improve compile times we've been looking at rustc benchmarks over a number of crates in the ecosystem.

We've noticed hyper was preventing pipelining opportunities, both for:

  • its own dependencies: compilation only starts when dependencies are done with codegen, not just when their metadata is available, e.g. for tokio or h2, depending on the chosen features.
  • its dependent crates: it seems crates depending on hyper also wait for codegen instead of its metadata, as we can see e.g. here in warp-0.3.2.

In this cargo timing example, hyper could start building 6.6s earlier with h2's metadata, and warp could start building as soon as hyper's metadata is available, around 6s earlier (from different measurements, hyper's metadata is not visible in the link above).

Adding both of these together, there could be a sizable opportunity to improve compile times in the whole hyper ecosystem, in addition to hyper contributors working on the crate itself.

I've discussed this with @nox and they've directed me at #2685 as the cause, with this cargo issue as a blocker. Until that is fixed however, I wanted to discuss this hack as an alternative, to re-enable pipelining (without creating a whole different FFI-binding crate) and improve compile times in the meantime.

The idea is to use #![crate_type] to build the cdylib only when the FFI cfg is active, in the source rather than via cargo. This works, I believe, on stable and on your MSRV of 1.46 (but I'm not sure I was able to fully test this, and hope CI will help). It has however been recently deprecated, and that deprecation will land on stable in the 1.59 release tomorrow. I don't think it'll go away before the cargo issue is fixed though, so it still seems pretty safe. (Also: I've had to allow the lint to build with deny warnings, and unknown lints to support 1.46+ which don't yet know about that new lint. Another note: maybe the ffi feature should be checked in addition to the hyper_unstable_ffi cfg ?).

I believe there's another cargo feature which could maybe help, to specify dependency crate types in the cargo manifest; all in all, it seems issues in cargo will be fixed and this is mostly a temporary measure to help things until those principled fixes are available.

Just to give you an idea of the impact this has, I've benchmarked building hyper (with the client,http1,http2 features) from -j2 to -j12: debug builds improve by 7-8%, and release builds by 18-29% (26% mean). This does translate to downstream crates.

I can provide precise numbers and cargo timing graphs if needed.

(Opening as draft for discussion whether something like this could work for you ? and to test it out on CI)

@seanmonstar
Copy link
Member

Oh interesting! This is a wonderful solution, assuming it works all the way through. rustc will know to compile its dependencies correctly? I assumed cargo needed to know.

src/lib.rs Outdated Show resolved Hide resolved
@lqd
Copy link
Contributor Author

lqd commented Feb 24, 2022

assuming it works all the way through. rustc will know to compile its dependencies correctly?

I hope so but I'm not actually sure yet.

The FFI tests done in the CI workflow seem to work locally for me, but I don't know exactly know the setup used in curl to test this. If you could point me at some documentation or have ideas on how to test it more thoroughly, it would be very helpful.

Maybe running the capi examples and just not building them would be enough ? Maybe having a different crate depending on this, much like your cargo issue example ?

@lqd
Copy link
Contributor Author

lqd commented Feb 24, 2022

After further testing I don't think this actually will work, CI will confirm that I'm sure. There were likely stray .sos locally that made the examples build. Re-doing the tests on a clean checkout now fails. Sorry about the noise.

It'll at least provide good numbers for the cargo fix. And I'll test those when they land. I will also test the WIP PR for the fix.

@lqd
Copy link
Contributor Author

lqd commented Feb 24, 2022

It's not much use keeping this open. I will help fix this though.

@lqd lqd closed this Feb 24, 2022
@lqd lqd deleted the pipelining branch February 24, 2022 09:01
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.

None yet

2 participants