Skip to content

Conversation

@cart
Copy link
Member

@cart cart commented Mar 11, 2025

Objective

Fixes #18103

#17330 introduced a significant compile time performance regression (affects normal builds, clippy, and Rust Analyzer). While it did fix the type-resolution bug (and the general approach there is still our best known solution to the problem that doesn't involve significant maintenance overhead), the changes had a couple of issues:

  1. It used a Mutex, which poses a significant threat to parallelization.
  2. It externalized existing, relatively simple, performance critical Bevy code to a crate outside of our control. I am not comfortable doing that for cases like this. Going forward @bevyengine/maintainer-team should be much stricter about this.
  3. There were a number of other areas that introduced complexity and overhead that I consider unnecessary for our use case. On a case by case basis, if we encounter a need for more capabilities we can add them (and weigh them against the cost of doing so).

Solution

  1. I moved us back to our original code as a baseline
  2. I selectively ported over the minimal changes required to fix the type resolution bug
  3. I swapped Mutex<BTreeMap<PathBuf, &'static Mutex<CargoManifest>>> for RwLock<BTreeMap<PathBuf, CargoManifest>>. Note that I used the parking_lot RwLock because it has a mapping API that enables us to return mapped guards.

@cart cart added C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 11, 2025
@cart cart added this to the 0.16 milestone Mar 11, 2025
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Mar 11, 2025
@atlv24
Copy link
Contributor

atlv24 commented Mar 11, 2025

Its kind of scary that a crate dep was added that easily. I dont think it was malicious here but it shows this is a very possible attack vector. Sneak an official-sounding crate you made into macro path, then do a patch release that gets automatically picked up by cargo which has the macros insert malicious code invisibly.

@cart
Copy link
Member Author

cart commented Mar 11, 2025

My thoughts exactly. Dependencies should be a "last resort" sort of thing, and when we take them, they should be in a trusted (known actor / high traffic / high visibility / high review) context.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 11, 2025
@cart
Copy link
Member Author

cart commented Mar 12, 2025

I have removed the bevy crate remapping test as it is no longer supported in this impl. I did write code to re-add support, but supporting that means scanning and string comparing every dependency (and every dev dependency) on all macro executions that don't match the bevy or bevy_x branches. We'll hit that for every macro execution in all built-in crates, in addition to the "crate alias" case. I would rather drop support than pay this compile time cost, given that remapping is super uncommon (github global search revealed zero remapped macro executions), and given that workarounds exist (see the added code comment).

@cart cart enabled auto-merge March 12, 2025 00:38
@cart cart added this pull request to the merge queue Mar 12, 2025
Merged via the queue into bevyengine:main with commit e21dfe8 Mar 12, 2025
35 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Mar 12, 2025
# Objective

When reviewing #18263, I noticed that `BevyManifest` internally stores a
`DocumentMut`, a mutable TOML document, instead of an `ImDocument`, an
immutable one. The process of creating a `DocumentMut` first involves
creating a `ImDocument` and then cloning all the referenced spans of
text into their own allocations (internally referred to as `despan` in
`toml_edit`). As such, using a `DocumentMut` without mutation is
strictly additional overhead.

In addition, I noticed that the filesystem operations associated with
reading a manifest and parsing it were written to be completed _while_ a
write-lock was held on `MANIFESTS`. This likely doesn't translate into a
performance or deadlock issue as the manifest files are generally small
and can be read quickly, but it is generally considered a bad practice.

## Solution

- Switched to `ImDocument<Box<str>>` instead of `DocumentMut`
- Re-ordered operations in `BevyManifest::shared` to minimise time spent
holding open the write-lock on `MANIFESTS`

## Testing

- CI

---

## Notes

I wasn't able to measure a meaningful performance difference with this
PR, so this is purely a code quality change and not one for performance.

---------

Co-authored-by: Carter Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

Compile / typecheck perf regression in 0.16 from proc macro changes

4 participants