Optimize map edits by skipping expensive tile index property copies
TODO: do some extra checks to ensure that this optimization doesn't have negative side effects with the proposed optimization in Content Patcher.
Notes: The before/after include extra instrumentation that did not change between the builds. "tileSheets" tracked L72 to L96 "sourceToTarget" tracked L101 to L111 "tileEdits" tracked L112 to the end. there was an independent timer for the entire method contents and the logging only occured if that outer timer took >5ms
Before:
https://smapi.io/log/0e18a2cf478d4e0a9f1d66b2eaff697e?Mods=SMAPI&Levels=trace%7Ecritical&Filter=MapProfiling
https://smapi.io/json/none/d8d91c7f9abd4ed281f1aaa6bff8e1bd
After:
https://smapi.io/log/b4382cce0d734661aa576080082920a9?Mods=SMAPI&Levels=trace%7Ecritical&Filter=MapProfiling
https://smapi.io/json/none/3b37058c818d4cd4b8797cbb94820031
Multiple people having been daily driving this smapi build for months now and haven't reported issues with this shortcut.
Hi! I'm worried this optimization would lead to cache mutation bugs.
For example, consider this scenario:
- Mod A applies a map patch.
- Mod B edits tile properties within the target map's edited area.
Since the target map and map patch now share the same tile property collections for the patched area, mod B's edits are now part of mod A's map patch (and would thus be applied whenever it uses that patch, even if mod B's edits change or no longer apply).
So it would work in most cases, but would introduce subtle and hard-to-troubleshoot bugs.
In isolation, I don't think that scenario would occur, as the sourece and target Map instances are both new instances every iteraton of the asset loading cycle.
There can be some potential issues when coupled with Pathoschild/StardewMods#1085 which would cause the target side to get cached and so it's propertyCollection would be long lived and could inherit changes.
I'll experiment with seeing if doing
targetSheet.Properties.CopyFrom(sourceSheet.Properties)
gets a similar ballpark performance, as it would continue to skip all the index key schenanigans that the TileIndexProperties accessor schenanigans has, but isn't sharing a propertyCollection reference
@Pathoschild Updated the PR to use the Properties Copy which would remove the Cache mutability concerns and is pretty much just as fast anyway. (The old overhead was in the weird keying behaviour that TileIndexProperties used which always used the underlying Properties as the actual backing storage anyway)
Before: https://smapi.io/log/dc1e09fc852f4a9e831244ed1b529374?Filter=%5BMapProfiling%5D&Levels=trace%7Edebug%7Einfo%7Ewarn%7Eerror%7Ealert%7Ecritical
After: https://smapi.io/log/6f2ac1345a9a4bc287fd20d42296e7e1?Filter=%5BMapProfiling%5D&Levels=trace%7Edebug%7Einfo%7Ewarn%7Eerror%7Ealert%7Ecritical
Merged into develop for the upcoming SMAPI 4.3.1. Thanks for the help!