Skip to content

Conversation

ChibiDenDen
Copy link
Contributor

@ChibiDenDen ChibiDenDen commented Jan 13, 2020

This change is a second attempt to fix #34161
first attempt is here: #35027

Bugsquad edit: also fixes #30557.

In this change, when a GDScript is destroyed, an ObjectID is kept on GDScriptLanguage for each subclass that is still in use.
when the GDScript is loaded again, it checks if an orphan subclass is still available and reuses it.

To be considered:
the ObjectID is kept in a Map where the Key is a string in the format CLASS:SUBCLASS, this means that there is no support for multiple levels of subclasses, only one.
IMO, every class should keep a fully qualified name, or some other UNIQUE_ID that is stable and reproducible without requiring a full load of the subclass.

@ChibiDenDen
Copy link
Contributor Author

I have tested and this seems to fix #30557 as well

@akien-mga akien-mga added this to the 3.2 milestone Jan 14, 2020
@akien-mga
Copy link
Member

akien-mga commented Jan 14, 2020

See also #35096 which is @vnen's attempt at solving the same issue (I don't know if it addresses #30557 too).

@vnen
Copy link
Member

vnen commented Jan 14, 2020

This is better but I fear it has the same problem as the other PR: references are kept alive forever. The difference here is that only some inner classes will be kept. But once they are in that list, they'll only be freed when the application ends.

Maybe we can do a similar thing but only in smaller contexts. For instance, keep this list for the parser while it's running. Or for the compiler it can keep a list of external references in the actual GDScript, so it can take them away when it's freed.

Still, I'm not confident in merging such a solution for 3.2 this close to the release.

@ChibiDenDen
Copy link
Contributor Author

Why do you think that they are kept alive forever?
this is just the objectid, this does not keep reference.
I can create a test case that shows that.

@vnen
Copy link
Member

vnen commented Jan 14, 2020

I see, I should have another coffee before reading the code.

I guess this is a good solution, since it avoids recreating the inner classes when the owner script is reloaded, so the memory address don't change.

@vnen
Copy link
Member

vnen commented Jan 14, 2020

Also, #35122 should be merged first, so this PR can be updated to use the fully qualified names.

@ChibiDenDen ChibiDenDen requested a review from vnen January 14, 2020 22:20
@vnen
Copy link
Member

vnen commented Jan 15, 2020

Looks good, thanks. Just need to squash the commits into one. See http://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase

@ChibiDenDen ChibiDenDen force-pushed the reuse_orphaned_subclass branch from c3b66e0 to e3fa516 Compare January 15, 2020 20:51
@ChibiDenDen ChibiDenDen requested review from neikeq, reduz and a team as code owners January 15, 2020 20:51
@ChibiDenDen ChibiDenDen force-pushed the reuse_orphaned_subclass branch from e3fa516 to 86aa12e Compare January 15, 2020 20:54
@ChibiDenDen
Copy link
Contributor Author

Looks good, thanks. Just need to squash the commits into one. See http://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase

Done, its a single commit now

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It's a clever solution while we don't have the proper ScriptCache.

@akien-mga akien-mga merged commit cd7b51b into godotengine:master Jan 16, 2020
@akien-mga
Copy link
Member

Thanks!

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