Skip to content
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

Avoid unnecessary deep copies to improve generation speed #133

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

Zannick
Copy link
Contributor

@Zannick Zannick commented Nov 4, 2024

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:

  • while profiling: 528s -> 412s (~22% improved)
  • with spoiler=1: 213s -> 182s (~15% improved)
  • with spoiler=3: 837s -> 742s (~11% improved)

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
Copy link
Collaborator

flohgh commented Nov 5, 2024

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.

@flohgh flohgh changed the base branch from master to pr-133 November 5, 2024 22:30
@flohgh flohgh merged commit 04ed5be into theonlydude:pr-133 Nov 5, 2024
@Zannick
Copy link
Contributor Author

Zannick commented Nov 5, 2024

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
Copy link
Collaborator

flohgh commented Nov 5, 2024

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.

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