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

[Merged by Bors] - Decouple some dependencies #3886

Closed
wants to merge 4 commits into from

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Feb 7, 2022

Objective

Reduce from scratch build time.

Solution

Reduce the size of the critical path by removing dependencies between crates where not necessary. For cargo check --no-default-features this reduced build time from ~51s to ~45s. For some commits I am not completely sure if the tradeoff between build time reduction and convenience caused by the commit is acceptable. If not, I can drop them.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 7, 2022
@TheRawMeatball TheRawMeatball added A-Build-System Related to build systems or continuous integration C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Feb 7, 2022
@alice-i-cecile
Copy link
Member

I'm unsure about:

  • Removing thiserror usage in bevy_reflect

I agree with all other changes. I am particularly happy with the changes to bevy_derive; I was poking my nose into that code recently and this is a more sensible approach.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change and removed C-Feature A new feature, making something new possible labels Feb 7, 2022
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Feb 8, 2022
@bors
Copy link
Contributor

bors bot commented Feb 8, 2022

try

Build failed:

@bjorn3
Copy link
Contributor Author

bjorn3 commented Feb 8, 2022

I dropped "Remove the thiserror usage in bevy_reflect", enabled the full syn feature for bevy_reflect_derive too (which should fix CI) and removed another unused dependency.

@mockersf
Copy link
Member

mockersf commented Feb 9, 2022

I would prefer to have less different crates depending on uuid, that's kind of a pain... but I guess that's part of the speedup

bors bot pushed a commit that referenced this pull request Apr 20, 2022
This a commit I think would be uncontroversial that has been cherry-picked from #3886
bors bot pushed a commit that referenced this pull request Apr 25, 2022
A couple more uncontroversial changes extracted from #3886.

* Enable full feature of syn
  
   It is necessary for the ItemFn and ItemTrait type. Currently it is indirectly
   enabled through the tracing dependency of bevy_utils, but this may no
   longer be the case in the future.
* Remove unused function from bevy_macro_utils
@bjorn3 bjorn3 force-pushed the decouple_deps branch 2 times, most recently from aee38c6 to 9e3b42c Compare April 27, 2022 09:23
@bjorn3
Copy link
Contributor Author

bjorn3 commented Apr 27, 2022

I dropped the uuid change. This reduces the savings for cargo check --no-default-features a bit, but this PR still gives a worthwhile improvement from 48s to 40s. Ready for review.

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 27, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

LGTM. That's a sizable win on build times.

@alice-i-cecile
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 27, 2022

Merge conflict.

This allows tracing to be built long before syn is built, which allows
for more build parallelism.
Instead add an inherent implementation. This allows removing the
bevy_derive dependency from bevy_utils.
@bjorn3
Copy link
Contributor Author

bjorn3 commented Apr 27, 2022

Rebased

@alice-i-cecile
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Apr 27, 2022
# Objective

Reduce from scratch build time.

## Solution

Reduce the size of the critical path by removing dependencies between crates where not necessary. For `cargo check --no-default-features` this reduced build time from ~51s to ~45s. For some commits I am not completely sure if the tradeoff between build time reduction and convenience caused by the commit is acceptable. If not, I can drop them.
@bors bors bot changed the title Decouple some dependencies [Merged by Bors] - Decouple some dependencies Apr 27, 2022
@bors bors bot closed this Apr 27, 2022
@bjorn3 bjorn3 deleted the decouple_deps branch April 28, 2022 08:06
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
This a commit I think would be uncontroversial that has been cherry-picked from bevyengine#3886
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
A couple more uncontroversial changes extracted from bevyengine#3886.

* Enable full feature of syn
  
   It is necessary for the ItemFn and ItemTrait type. Currently it is indirectly
   enabled through the tracing dependency of bevy_utils, but this may no
   longer be the case in the future.
* Remove unused function from bevy_macro_utils
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
# Objective

Reduce from scratch build time.

## Solution

Reduce the size of the critical path by removing dependencies between crates where not necessary. For `cargo check --no-default-features` this reduced build time from ~51s to ~45s. For some commits I am not completely sure if the tradeoff between build time reduction and convenience caused by the commit is acceptable. If not, I can drop them.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
This a commit I think would be uncontroversial that has been cherry-picked from bevyengine#3886
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
A couple more uncontroversial changes extracted from bevyengine#3886.

* Enable full feature of syn
  
   It is necessary for the ItemFn and ItemTrait type. Currently it is indirectly
   enabled through the tracing dependency of bevy_utils, but this may no
   longer be the case in the future.
* Remove unused function from bevy_macro_utils
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Reduce from scratch build time.

## Solution

Reduce the size of the critical path by removing dependencies between crates where not necessary. For `cargo check --no-default-features` this reduced build time from ~51s to ~45s. For some commits I am not completely sure if the tradeoff between build time reduction and convenience caused by the commit is acceptable. If not, I can drop them.
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-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants