From dc7cb2a3e17be7ff576dad65cf1a1e05722c59e7 Mon Sep 17 00:00:00 2001
From: Zachary Harrold <zac@harrold.com.au>
Date: Thu, 13 Mar 2025 07:15:39 +1100
Subject: [PATCH] Switch to `ImDocument` in `BevyManifest` (#18272)

# 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 <mcanders1@gmail.com>
---
 crates/bevy_macro_utils/src/bevy_manifest.rs | 27 ++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/crates/bevy_macro_utils/src/bevy_manifest.rs b/crates/bevy_macro_utils/src/bevy_manifest.rs
index 813f8533c9..b0d321ba22 100644
--- a/crates/bevy_macro_utils/src/bevy_manifest.rs
+++ b/crates/bevy_macro_utils/src/bevy_manifest.rs
@@ -8,12 +8,12 @@ use std::{
     path::{Path, PathBuf},
     time::SystemTime,
 };
-use toml_edit::{DocumentMut, Item};
+use toml_edit::{ImDocument, Item};
 
 /// The path to the `Cargo.toml` file for the Bevy project.
 #[derive(Debug)]
 pub struct BevyManifest {
-    manifest: DocumentMut,
+    manifest: ImDocument<Box<str>>,
     modified_time: SystemTime,
 }
 
@@ -34,14 +34,15 @@ impl BevyManifest {
                 return manifest;
             }
         }
+
+        let manifest = BevyManifest {
+            manifest: Self::read_manifest(&manifest_path),
+            modified_time,
+        };
+
+        let key = manifest_path.clone();
         let mut manifests = MANIFESTS.write();
-        manifests.insert(
-            manifest_path.clone(),
-            BevyManifest {
-                manifest: Self::read_manifest(&manifest_path),
-                modified_time,
-            },
-        );
+        manifests.insert(key, manifest);
 
         RwLockReadGuard::map(RwLockWriteGuard::downgrade(manifests), |manifests| {
             manifests.get(&manifest_path).unwrap()
@@ -69,11 +70,11 @@ impl BevyManifest {
         std::fs::metadata(cargo_manifest_path).and_then(|metadata| metadata.modified())
     }
 
-    fn read_manifest(path: &Path) -> DocumentMut {
+    fn read_manifest(path: &Path) -> ImDocument<Box<str>> {
         let manifest = std::fs::read_to_string(path)
-            .unwrap_or_else(|_| panic!("Unable to read cargo manifest: {}", path.display()));
-        manifest
-            .parse::<DocumentMut>()
+            .unwrap_or_else(|_| panic!("Unable to read cargo manifest: {}", path.display()))
+            .into_boxed_str();
+        ImDocument::parse(manifest)
             .unwrap_or_else(|_| panic!("Failed to parse cargo manifest: {}", path.display()))
     }