theonlydude / RandomMetroidSolver

VARIA Randomizer, Solver and Tracker for Super Metroid
http://randommetroidsolver.pythonanywhere.com
MIT License
45 stars 27 forks source link

Avoid unnecessary deep copies to improve generation speed #133

Closed Zannick closed 4 weeks ago

Zannick commented 4 weeks ago

In Archipelago, Super Metroid's copy_mixin function showed up in profile results as 145s cumulative time, out of 639s (73 players). Most of that time is in deepcopy and my analysis showed the majority was spent copying the fields helpers and objectives, of which the latter breaks down further into goals and graph.

helpers just keeps a reference to the smbm, so we don't need to create another copy while we're copying, and graph is remarked as being something that doesn't change. As such, we can avoid making copies of those; helpers can point back at the new copy, and graph can be the same object.

Results:


I wrote and tested this in Archipelago (PR) without a ROM file, so please confirm that this works as expected here, especially as I did not confirm that the graph referenced by Objectives is not modified somewhere deeply hidden in the code, but since this did not break AP generation with the base settings I believe it works right.

flohgh commented 4 weeks ago

Hello, so just to be sure I understand, the deep copies themselves are in the Archipelago codebase copying VARIA objects? Or is it a performance issue when generating a seed in VARIA in general? I looked around for deepcopy in our code and the only intensive copies I found were of ItemLocContainer objects, so I'm really curious how graph/objectives objects would be copied.

We'll include the fix anyway if there's no regression, but I was wondering if there was a deeper issue to look into.

The graph is modified a lot at first, and a bit at the end if you randomize the escape sequence, and objectives are modified before generating but are not modified afterwards iirc.

Zannick commented 4 weeks ago

ItemLocContainer does contain an SMBoolManager but it only does a shallow copy from what I can find. You're right, the deep copies I was looking at are specific to Archipelago's VARIA implementation.

If you have a convenient way to run the randomizer with spoiler output only and no base ROM, it would be easier for me to profile it to confirm.

The concern over editing the graph would be that we couldn't keep a sole copy of the graph around if we're modifying it between attempts. The copies in question come from copying the state to test whether the seed is beatable, and I wouldn't think collecting items would alter the graph object, but it's good to be certain. If we do ever want to actually deep copy and include the graph we would need a different method.

flohgh commented 4 weeks ago

Alright, thanks for the explanations! I ran our test tool to generate/solve a couple thousands seeds, and everything seems fine, so I merged the PR to master.