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

minor memory leaks fixes #4840

Merged
merged 1 commit into from Oct 10, 2022
Merged

Conversation

max-au
Copy link
Contributor

@max-au max-au commented May 17, 2021

Running "erl -emu_type asan -eval "erlang:halt()." shows several minor issues straight away. First, init_misc_aux_work is called twice (and first allocation is made orphan), second, when shutting down, async threads don't clean up thread progress data, and - an expected leak, even documented - signal stack.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label May 17, 2021
@rickard-green rickard-green self-assigned this May 17, 2021
@max-au
Copy link
Contributor Author

max-au commented Jul 9, 2021

Would it work now, with alloca avoiding sharing signal buffer?

@rickard-green
Copy link
Contributor

Would it work now, with alloca avoiding sharing signal buffer?

No, since the memory allocated by alloca() is free:d as soon as the sys_thread_init_signal_stack() call returns. This memory block needs to be available as long as the thread is alive.

If I read the code correct it is only used by scheduler threads and the main thread. None of these threads terminates unless the whole vm terminates, so it is no actual memory leak malloc:ing this memory and never free:ing it.

@max-au
Copy link
Contributor Author

max-au commented Jan 14, 2022

I kept the suppression in sys_signal_stack.c, let's address it separately. I think now the PR is ready for review (I also rebased it on top of master branch for merge convenience).

@garazdawi garazdawi added the bug Issue is reported as a bug label Apr 6, 2022
@max-au
Copy link
Contributor Author

max-au commented Sep 30, 2022

Is there any interest in merging this PR? If not, I'm fine with just closing it.

@rickard-green
Copy link
Contributor

Is there any interest in merging this PR? If not, I'm fine with just closing it.

Yes, there is interest. I just forgot about this one, sorry.

@rickard-green rickard-green changed the base branch from master to maint October 4, 2022 07:39
@rickard-green rickard-green added the testing currently being tested, tag is used by OTP internal CI label Oct 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

CT Test Results

       3 files     125 suites   43m 3s ⏱️
1 491 tests 1 442 ✔️ 49 💤 0
1 802 runs  1 736 ✔️ 66 💤 0

Results for commit 3d2b7b6.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@rickard-green
Copy link
Contributor

@max-au

I've added a commit that doesn't remove the wakup callback when unregistering from thread progress as well as making the async threads wakup callback prepared for a NULL argument. This since, otherwise an outstanding wakup request could trigger after the unregistration and cause an emulator crash. An alternative could be to be prepared for NULL wakeup callbacks instead of requiring the wakup callbacks of potentially unregistered threads being prepared for a NULL argument. wakup callbacks are however executed to a much higher degree than the async thread callback; therefore, I don't like such an approach.

Preferably you squash these commits.

erl_process: do not repeat init_misc_aux_work call, avoids small memory leak for misc_aux_work_queues.
erl_async: clean up thread progress data on exit
@max-au
Copy link
Contributor Author

max-au commented Oct 6, 2022

Thanks for noticing that, I completely missed the problem (due to lack of async threads usage, I think).
Squashed the commits and rebased to fresh maint branch.

@rickard-green rickard-green merged commit a8f3755 into erlang:maint Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants