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

Add missed clear to unsafe_free_list_storage #294

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jj683
Copy link
Contributor

@jj683 jj683 commented Oct 24, 2024

We encountered different behavior of thread_local_free_list_heap and unsafe_free_list_heap data structures even when used in the single thread environment.

Unfortunately I am still in the process of building a small test to reproduce it, but while reading the code I noticed that the clear function is not called for unsafe_free_list_storage similarly to the thread_local_free_list_storage container.
Is this expected behavior?

@arximboldi
Copy link
Owner

Hmmmm interesting.

I think my reasoning here is as follows:

  • The unsafe free list is assumed to be used in a single threaded program. Therefore the destruction of the heap means the end of the program lifetime. Therefore clearing is just unnecessary work. Note that at this level of abstraction these lists hold plain memory with no destructor or no further nested resources so it is safe and not a leak to not return them to the language runtime heap (this may not be 100% true if the heap is composed with another heap that falsifies this statement, none exist however in the Immer codebase at the moment though).

  • In the thread local list case, however, the thread may end before the program ends. Therefore we should return the blocks to the previous heap to avoid leaks and so that it can make use of those in any useful way.

With these assumptions the current code was correct. But maybe you were using the code in some way that negates some of the assumptions? What prompted you to make these changes?

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