Switch to ImDocument in BevyManifest

[bushrat011899]

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.

[MrGVSV]

Nit: is there a reason the definition of key is up here and not by its usage?

[bushrat011899]

Purely because it's a boxed string (so cloning requires allocation), and we take a write lock. Similar to the filesystem operations, allocating involves syscalls and can block, so in general you want to do as little within the locked scope as possible.

[cart]

I moved it right above the write lock to satisfy both constraints!

[alice-i-cecile]

Leaving this for @cart to merge for now, mostly because I think it's good for him to see it :)

[cart]

Good calls all around!