Skip to content

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

SinZ163
Copy link
Contributor

@SinZ163 SinZ163 commented Feb 20, 2025

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
image
https://smapi.io/json/none/d8d91c7f9abd4ed281f1aaa6bff8e1bd

After:
https://smapi.io/log/b4382cce0d734661aa576080082920a9?Mods=SMAPI&Levels=trace%7Ecritical&Filter=MapProfiling
image
https://smapi.io/json/none/3b37058c818d4cd4b8797cbb94820031

@Pathoschild Pathoschild force-pushed the develop branch 2 times, most recently from 27fb753 to 8bbbf08 Compare April 13, 2025 23:13
@SinZ163 SinZ163 marked this pull request as ready for review May 5, 2025 23:44
@SinZ163
Copy link
Contributor Author

SinZ163 commented May 5, 2025

Multiple people having been daily driving this smapi build for months now and haven't reported issues with this shortcut.

@Pathoschild
Copy link
Owner

Hi! I'm worried this optimization would lead to cache mutation bugs.

For example, consider this scenario:

  1. Mod A applies a map patch.
  2. 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.

@SinZ163
Copy link
Contributor Author

SinZ163 commented May 15, 2025

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

@SinZ163
Copy link
Contributor Author

SinZ163 commented May 26, 2025

@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

@SinZ163 SinZ163 force-pushed the optimization/mapedit-texturecopy branch from a4326f5 to 7f7f0e5 Compare May 26, 2025 01:13
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
@SinZ163 SinZ163 force-pushed the optimization/mapedit-texturecopy branch from 7f7f0e5 to 86993db Compare May 26, 2025 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants