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

core: Remove LoaderHandle when load is done #15115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Aaron1011
Copy link
Member

In almost all cases, we were letting completed LoaderHandles accumulate, leaking memory and wasting time in preload_tick

In almost all cases, we were letting completed LoaderHandles
accumulate, leaking memory and wasting time in `preload_tick`
@Dinnerbone
Copy link
Contributor

Sorry, maybe reviewed too quickly.

I notice that some places have the potential to return early with ? before getting to this line. Is that going to be an issue?

Is there another way we can do this?

@Dinnerbone Dinnerbone self-requested a review February 7, 2024 19:15
@@ -24,6 +24,12 @@ use super::Avm2;
#[collect(no_drop)]
pub struct Domain<'gc>(GcCell<'gc, DomainData<'gc>>);

impl std::fmt::Debug for Domain<'_> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidentally added?

Copy link
Member

Choose a reason for hiding this comment

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

My guess is Aaron added it, much like the Debug impls later on, while debugging the memory problems caused by not cleaning up loaders.

I'm personally fine with merging the debug impls alongside everything else. Not being able to trace something because someone forgot to add a Debug impl is frustrating.

@danielhjacobs danielhjacobs added the waiting-on-author Waiting on the PR author to make the requested changes label Mar 5, 2024
@kmeisthax
Copy link
Member

I'm wondering if we can abuse RAII/Drop to make loaders close themselves regardless of whether or not they exit normally or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting on the PR author to make the requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants