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

Fix slow load/save of scenes with many instances of the same script #49570

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

RandomShaper
Copy link
Member

This is my hypothesis and approach to fix #43156.

@reduz did c79be97 quite a long time ago to fix one of these: #756, #859, #914

There, some changed condition was removed so _update_exports() would update every script placeholder instance regardless the script had changed or not. One effect I can confirm if the condition is re-added is that the property inspector won't show the exported variables anymore (however, I'm not sure if the removal fixes something else in addition, and that's why I've listed all the three potential issues that may be addressed by the change).

However, that change also introduced the behavior that when the scene edit state is generated –which happens when saving or loading a scene–, every placeholder instance of a given script triggers the update of the exports of all the others that have been found so far, which may even happen several times because the same code is run for the whole inheritance chain.

This PR fixes that by making such updates conditional again, this way:

  • If the update happens because of a script change (so the check for changed is back), all the instances are updated, which is what was happening unconditionally and causing the slowness.
  • But, in order to avoid resurrecting a bug, now there's the option to ask explicitly for the update of a single placeholder instance, so the code that creates a instance can ask to update its, and only its, exports, instead of doing that on every instance.

Lastly, I think that C# was added when change had already been removed from the condition a long time ago, so it would be affected by the same bug. For that reason, I've applied the patch there as well.

Disclaimer:
As I said above, this is based on my hypothesis about the bug, but it's hard to be sure. Also, testing to ensure all the aforementioned bugs (or any other, for that matter) aren't resurrected would be very difficult,

@akien-mga, being that the case, I'm not sure if this should get the Needs testing tag or, considering that specific testing for regressions is impractical or nearly impossible, just let users do it via alphas/betas.

Supersedes #47828.

Fixes #43156.

@RandomShaper RandomShaper added this to the 4.0 milestone Jun 13, 2021
@RandomShaper RandomShaper requested review from vnen, reduz and neikeq June 13, 2021 11:55
@RandomShaper RandomShaper requested review from a team as code owners June 13, 2021 11:55
@akien-mga
Copy link
Member

CC @lekoder @johnfn if you can test that this solves your issues.

@lekoder
Copy link
Contributor

lekoder commented Jun 13, 2021

Yes it does, excellent work @RandomShaper! One of scenes that took over 15 mins to open now opens and saves in less than a second. I still have scenes that open 10-15 seconds, but that is manageable and I can work with that.

@akien-mga akien-mga merged commit cbcdda6 into godotengine:master Jun 15, 2021
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the fix_slow_scene_io branch June 15, 2021 13:57
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 17, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.3.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving complex scene with multiple exports var statements and instanced sub-scenes takes up to minutes
3 participants