Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The traversal epoch fallback needs mutation coverage, and several additions violate repository naming, import, and documentation conventions.
Review effort: Balanced
Findings: 1
Open (18)
What changed in this PR
Improves stage opening, traversal, status queries, and asset memory usage without intended behavior changes.
Changes:
- Shares asset and path storage to reduce copying.
- Reuses prepared layers and combined cache queries.
- Threads inherited prim status through traversal with epoch validation.
| File | Description |
|---|---|
crates/openusd/src/ar.rs |
Adds shared asset bytes and cursor handoff. |
crates/openusd/src/sdf/file_format.rs |
Supports shared-byte decoding. |
crates/openusd/src/sdf/layer_registry.rs |
Reuses prepared root layers. |
crates/openusd/src/sdf/path.rs |
Makes cloned paths share text. |
crates/openusd/src/usdc/mod.rs |
Decodes USDC from shared bytes. |
crates/openusd/src/pcp/index_cache.rs |
Combines and localizes status queries. |
crates/openusd/src/usd/prim.rs |
Reuses status and property cache work. |
crates/openusd/src/usd/stage.rs |
Optimizes opening and traversal. |
crates/openusd/tests/stage.rs |
Adds performance-path regressions. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3456
to
+3459
| let status = if parent == Some(epoch) { | ||
| self.child_status_masked(&path, needed)? | ||
| } else { | ||
| self.prim_status_masked(&path, needed)? |
Comment on lines
+121
to
+124
| /// The complete asset as bytes shared with the caller, whatever the | ||
| /// cursor position, for an asset already held in memory (C++ | ||
| /// `ArAsset::GetBuffer`). A format that decodes in place keeps them | ||
| /// instead of copying; `None`, the default, has the asset read instead. |
| /// cursor position, for an asset already held in memory (C++ | ||
| /// `ArAsset::GetBuffer`). A format that decodes in place keeps them | ||
| /// instead of copying; `None`, the default, has the asset read instead. | ||
| fn shared_bytes(&self) -> Option<std::sync::Arc<[u8]>> { |
| self.read_to_end(&mut buf)?; | ||
| return Ok(buf); | ||
| } | ||
| Ok(std::mem::take(self.get_mut())) |
| /// A cursor at its start hands its buffer over; one already read into | ||
| /// returns the rest. | ||
| #[test] | ||
| fn cursor_asset_read_all_moves_buffer() { |
Comment on lines
+3220
to
+3223
| /// [`prim_status_masked`](Self::prim_status_masked) for a prim whose | ||
| /// parent resolved active, loaded, defined and not abstract. Those bits | ||
| /// then come from the prim's own opinions, with no walk up its ancestors, | ||
| /// as C++ `Usd_PrimData` composes a prim's flags from its parent's. |
|
|
||
| fn read_shared_bytes( | ||
| &self, | ||
| bytes: std::sync::Arc<[u8]>, |
Comment on lines
+462
to
+463
| /// A crate layer read from an asset that shares its bytes decodes from | ||
| /// those bytes, holding a reference rather than a copy. |
| /// Opening a stage reads its root and session layers once each: the read that | ||
| /// composes their expression variables is the one their stacks are built from. | ||
| #[test] | ||
| fn root_and_session_layers_read_once() -> Result<()> { |
Comment on lines
+3229
to
+3230
| std::fs::write(&root, "#usda 1.0\ndef \"World\" {}\n")?; | ||
| std::fs::write(&session, "#usda 1.0\n")?; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Last one from the batch (after #139, #140 and #141): performance. No behaviour change, one thing per commit, and every commit is measured below so you can take only the ones you like. 7 sits on 6 and 9 on 5 and 7, the rest are standalone.
perf(ar): hand over cursor buffer in read_all: a package entry is read into aCursor<Vec<u8>>, andread_allthen copied it once more before decoding. A cursor still at its start now hands its buffer over. Openingoxbo_harvester.usdz(one 166 MB text layer) peaks at 273 MiB instead of 319.perf(ar): share asset bytes with file formats:Asset::shared_bytes(C++ArAsset::GetBuffer) lets a resolver that already holds an asset in memory share it, and the crate format decodes from those bytes instead of a copy. Through an in-memory resolver, a stage open on a 30 MB.usdcholds 18.6 MiB instead of 48.2. The default resolver doesn't use it; it's for resolvers like mine that keep documents in memory.perf(sdf): share a path's text between clones:Pathkeeps its text in anArc, so a clone is a pointer copy (C++SdfPathcopies share a pooled node). Traversal and status queries get 6 to 14% faster, while building a new path costs one more allocation, so opening a scene is a little slower.perf(usd): read root layers once when opening: theTODO(perf)inown_expression_variables. The root and session layers were parsed once for their expression variables and again for the layer stack, and now the first read is handed to the stack. Opening the scene takes half the time, and opening the 18 machines about 11% less.perf(usd): reuse the active check for loaded: asking for active and loaded together walked the ancestors for active twice. 14% off a warm traversal.perf(usd): resolve abstract ancestry in one query:is_abstractwent through the stage once per ancestor, now it is one cache query likeis_defined, with the same prototype root rule. 14% offis_abstract.perf(usd): resolve defined and abstract in one walk: every default traversal asks both, so they now share one ancestor walk. 29% off a warm traversal.perf(usd): sort properties in one cache query:attributes()andrelationships()asked the stage for each property's spec type separately, now it is one query. 12% off listing them.perf(usd): carry parent status through traversal: theTODO(perf)intraverse. A prim whose parent resolved active, loaded, defined and not abstract now reads only its own opinions instead of walking all its ancestors, which is how C++Usd_PrimDatacomposes a prim's flags from its parent's. It only takes that shortcut while the population epoch is unchanged and falls back to the full walk otherwise. A warm default traversal goes from 257 to 107 ms.How I measured: release build, median of 7 runs, on 18 real machine packages (889 MB of
.usdz, mostly text layers) and a generated scene of 65,556 prims (10,000 groups four levels deep, each referencing a five-prim part, plus inactive,overandclassbranches). Milliseconds after each commit:prim_statusevery primis_abstractevery primThe machine opens move by up to about 5% from run to run, so only the root layer commit is a real change there; the first two commits show up in memory rather than time. The benchmark program isn't in the PR, I'm happy to add it as an example or a bench if you want one.