perf(content): skip image traversal without imports - #17547
Conversation
🦋 Changeset detectedLatest commit: d1ff409 The changes in this PR will be included in the next version bump. This PR includes changesets to release 399 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will not alter performance
Comparing Footnotes
|
| ): T { | ||
| return entry.assetImports?.length | ||
| ? updateImageReferencesInData(entry.data, entry.filePath, imageAssetMap) | ||
| : structuredClone(entry.data); |
There was a problem hiding this comment.
Don't think we should clone it, for perf reasons. If this is about people pushing into arrays, they shouldn't be doing that, but more importantly, people who know not to do that should not suffer performance penalties to save those who do not know, if that makes sense.
There was a problem hiding this comment.
@matthewp I guess some kind of cloning is necessary, because we don't want to expose raw immutable data, and resolved image references need to modify the entry before returning. And structuredClone was added on purpose to fix Map and Set handling in Traverse.map, which was used before. See: #16701
I can follow up with another PR that builds on this one and adds internal caching for cloned and traversed entries.
There was a problem hiding this comment.
Ah, I forgot about that one. Ok, in that case, I think it's fine for now. I definitely do not want to document that we clone, so if there's a way in the future to prevent it we are able to do so.
I'll come back to reviewing this PR later.
Changes
getCollection()andgetEntry()now skip traversal whenassetImportsis empty, while still returning a freshstructuredClone.Related to #16297
Testing
Added tests covering the bypass, image resolution, and mutation isolation.
Tested on large collections:
assetImportsDocs
Internal optimisation change.