-
-
Notifications
You must be signed in to change notification settings - Fork 282
Optimize map edits by skipping expensive tile index property copies #981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize map edits by skipping expensive tile index property copies #981
Conversation
f7c917f
to
340ff4e
Compare
27fb753
to
8bbbf08
Compare
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:
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 There can be some potential issues when coupled with Pathoschild/StardewMods#1085 which would cause the 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 |
@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) |
a4326f5
to
7f7f0e5
Compare
This is pretty much just as efficient as all the old overhead was in the TileIndexProperties logic, but means its always a copy and wont have cache mutability problems
7f7f0e5
to
86993db
Compare
cfac33e
to
6c0c089
Compare
Merged into |
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