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

[issues/3340] fix for leak #3410

Closed
wants to merge 3 commits into from

Conversation

MikkelHJuul
Copy link

This is a simple fix for the leak of memory. It is slightly unsatisfactory as the memory required for the extra node (especially in the #replay(1)case) is still very high. With the extra Node, taking up 24 unnecessary bytes (27% of the #replay(1)-buffer's memory)

I have also added some extra tests, that I initially added for my other attempts at fixing this issue.

I have also fixed the same issue for the SizeAndTimeBoundReplayBuffer.

finalize head and roll it over in stead of resetting head, this means the current head will always remain a value-null in stead of being reassigned.
I applied the fix for the TimeAndSizeBoundReplayBuffer
@MikkelHJuul MikkelHJuul requested a review from a team as a code owner March 22, 2023 07:42
@MikkelHJuul
Copy link
Author

@OlegDokuka - thanks for the review in the other PR, I guess I just completely changed the implementation right as you were reviewing. Anyway this PR only fix the leak. This should be simpler to review and accept :)

@MikkelHJuul
Copy link
Author

this is obviously related to #3340, and fixes the leak, for all cases, including TimeAndSizeBoundReplayBuffer

@MikkelHJuul
Copy link
Author

On a side-note; I have run the stress test that I also added in relation to the PR #3364, and this doesn't fix the concurrency bug, and will still throw null pointers during #add

@MikkelHJuul
Copy link
Author

@OlegDokuka or others is there anything more I can add here to get this PR through. I haven't had much time for a long time now but it would be nice to push this and possibly the other related one (for the single case)

It seems there needs be a license header in the files added, is this simply copy paste work? What about the other failing check I cannot see logs anymore

@OlegDokuka OlegDokuka closed this Jan 22, 2024
@OlegDokuka
Copy link
Contributor

Closed due to this comment. Let's continue further discussion in the main issue.

Thanks.
Oleh

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