Skip to content

Conversation

@Veratil
Copy link
Contributor

@Veratil Veratil commented Oct 25, 2022

Did some digging on a common rpmalloc assert raised for me and found this instance of an MM_FREE not being called.

For the quick fix I just added the NotePlayHandle::freeAvailable function, since I didn't go through and see if anything else called NotePlayHandle::free.

@PhysSong
Copy link
Member

@Veratil I think this might not fix the assertion failure if you close LMMS after playing a lot of(like 2000 or more) notes simultaneously.

@Veratil
Copy link
Contributor Author

Veratil commented Oct 26, 2022

@Veratil I think this might not fix the assertion failure if you close LMMS after playing a lot of(like 2000 or more) notes simultaneously.

Right, I think that will have to be handled differently. This one at least fixes a blatant one that rpmalloc catches.

@Rossmaxx
Copy link
Contributor

Does this PR still have any effect after #7128?

@TechnoPorg
Copy link
Contributor

Does this PR still have any effect after #7128?

I think it does. I noticed this same memory leak while I was working on 7128 and was going to fix it, but this PR beat me to it. It just needs a rebase to not use the MM_FREE macros.

@Veratil
Copy link
Contributor Author

Veratil commented Feb 26, 2024

Does this PR still have any effect after #7128?

Yes, there's still two MM_ALLOC calls compared to one MM_FREE. I'll rebase this soon.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Mar 2, 2024

I'll rebase this soon.

Wouldn't it be better to close this one and make a new PR off of master instead?

@Veratil
Copy link
Contributor Author

Veratil commented Mar 2, 2024

I'll rebase this soon.

Wouldn't it be better to close this one and make a new PR off of master instead?

No. This PR is fine and there's no reason to close it just to open another doing the same thing. Rebasing is fairly easy and there's a reason it exists.

@Rossmaxx
Copy link
Contributor

Closing in favour of #7345

@Rossmaxx Rossmaxx closed this Jun 25, 2024
@Rossmaxx Rossmaxx mentioned this pull request Jun 25, 2024
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.

4 participants