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

Add bevy_dylib to force dynamic linking of bevy #808

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Nov 7, 2020

This easily improve compilation time by 2x.

Fixes #791

TODO

  • Mention bevy_dylib in docs.

Tip: You may want to review with git show --color-moved=dimmed-zebra to show moved code grey instead of red+green.

@karroffel karroffel added A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times labels Nov 7, 2020
@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 8, 2020

Ah, the docs live in a separate repo: https://github.com/bevyengine/bevy-website/blob/master/content/learn/book/getting-started/setup/_index.md

Should I make a PR or should I wait for this PR to be merged?

@cart
Copy link
Member

cart commented Nov 8, 2020

Feel free to make a pr whenever. We'll hold off on merging it until the next release (which will be in about a month) so there's no real time pressure.

@cart
Copy link
Member

cart commented Nov 9, 2020

First: this is super awesome ❤️

I have a proposal for a slight tweak to the crate structure, which allows for a better/clearer UX:

  • leave the current bevy crate in the root folder
  • add a bevy_internal crate, which pulls all of the crates together / defines the prelude (aka what the bevy crate does on the master branch)
  • the bevy crate re-exports the contents of bevy_internal. it also does the use bevy_dylib trick, but only if the dynamic feature is enabled. bevy_dylib would be an optional dependency.

This allows consumers of the bevy crate and examples to just import bevy stuff normally (without worrying about dylib tricks). If they want to force dynamic linking, they just need to do cargo run --features bevy/dynamic (for consumers) or cargo run --features dynamic (for examples). The default cargo run will exclude bevy_dylib from the build tree and build "statically"

I implemented this on my branch here: https://github.com/cart/bevy/tree/bevy_dylib

There are two downsides / caveats:

  1. for some reason this breaks imports in our proc macros (but only for examples). i'm trying to fix this now.
  2. we need to duplicate feature declarations: once in bevy and once in bevy_internal. i think the extra boilerplate is worth it

If we can fix (1) and you don't find any "gotchas" in this approach, I think I'll just push the changes to this pr's branch.

Thoughts?

@cart
Copy link
Member

cart commented Nov 9, 2020

I think I can fix (1) by making our proc_macro import inference a little smarter. I think using find-crate instead of proc-macro-crate will give me the tools i need.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 9, 2020

That approach looks better indeed.

@cart
Copy link
Member

cart commented Nov 10, 2020

Wow this plays nicely with the new ECS changes on master. The same example that could iterative compile (with dynamic linking) in 0.59 seconds now compiles in 0.43 seconds.

This easily improve compilation time by 2x
@cart cart merged commit 80a0448 into bevyengine:master Nov 10, 2020
@bjorn3 bjorn3 deleted the dylib branch November 10, 2020 06:43
Cargo.toml Show resolved Hide resolved
bjorn3 added a commit to bjorn3/bevy that referenced this pull request Nov 10, 2020
cart pushed a commit that referenced this pull request Nov 10, 2020
@joshuajbouw
Copy link
Member

Hold up, whats up with all the refactoring when #![cfg_attr(feature = "dynamic", crate_type = "dylib")] could've been used at lib.rs root? Would've accomplished the same thing without the refactor.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 28, 2020

Cargo passes --crate-type lib, so I don't think #![cfg_attr(feature = "dynamic", crate_type = "dylib")] would work.

@joshuajbouw
Copy link
Member

joshuajbouw commented Nov 28, 2020

But, that one of the 5 ways that the documentation says will work to change a crate type into dylib.

  • --crate-type=dylib
  • #![crate_type = "dylib"]
  • crate-type = "dylib" in Cargo.toml under [lib]
  • rustc -C target-feature=-crt-static foo.rs
  • RUSTFLAGS='-C target-feature=-crt-static' cargo build

The first 3 which are essentially the same thing, just overriding each other. Cargo shouldn't pass --crate-type lib by default. Internally a lib is default. Everything else will override it unless you actually pass in --crate-type lib manually.

Also the work done above has the added disadvantage of having to duplicate the module level documentation per crate. It doesn't concatenate I had found out.

So I am not exactly too certain why bevy needs to have it as a feature when you can tell the compiler directly.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 28, 2020

rustc -C target-feature=-crt-static foo.rs
RUSTFLAGS='-C target-feature=-crt-static' cargo build

These only control if the C standard library is statically linked or dynamically. Dynamically is the default for pretty much everything except musl.

crate-type = "dylib" in Cargo.toml under [lib]

You can't edit the Cargo.toml of dependencies.

So I am not exactly too certain why bevy needs to have it as a feature when you can tell the compiler directly.

The problem is that while you can tell it the compilet directly, you can't tell it cargo directly. When you use cargo, you can't override the crate type of arbitrary crates from your project. You can only set the crate type of your own project, which has to be an executable if you want to actually run your game.

@joshuajbouw
Copy link
Member

However, it is important to note that both the crate_type and crate_name attributes have no effect whatsoever when using Cargo, the Rust package manager. Since Cargo is used for the majority of Rust projects, this means real-world uses of crate_type and crate_name are relatively limited.

Just read that from: https://doc.rust-lang.org/beta/rust-by-example/attribute/crate.html. Weird that in their other reference seems to say otherwise. Anyways, yeah, makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the main bevy crate a dylib
4 participants