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

Fix(global_allocator): Switch wee_alloc for dlmalloc #594

Closed
wants to merge 1 commit into from

Conversation

RomanHodulak
Copy link
Contributor

No description provided.

@mrLSD
Copy link
Member

mrLSD commented Aug 31, 2022

Please provide a full description of PR>
Why and how is solved?

@mrLSD
Copy link
Member

mrLSD commented Aug 31, 2022

Well, this memory leak bug is quite old. And my question is, how much does it affect us?

Also, by the way, wee_alloc is a standard allocator in near_sdk.

Also, I would like to understand - what are the advantages and disadvantages of dlmalloc compared to wee_alloc. Let me remind you that in WebAssembly, memory works on a different principle than under the control of the operating system. This is a completely different model.

Replacing an allocator without research, just based on a post on Reddit - sounds extremely unconvincing. I repeat - we have been with this allocator for 1.5 years. And if we believe that this is really critical for us, then we need to conduct research, comparisons and show the effectiveness of the decision made.

Where is the proof that the new allocator is better? Looks harmless - simply because it looks better.

It may turn out that the total wee_alloc is still more efficient. Plus, keep in mind that memory leaks are not of such a critical nature, since we do not have a common stack and application execution state, due to the very specifics of WebAssembly.

@joshuajbouw
Copy link
Contributor

Closing given Pagoda's comments: near/near-sdk-rs#910

@joshuajbouw joshuajbouw closed this Sep 2, 2022
@joshuajbouw joshuajbouw deleted the global-allocator branch September 5, 2022 10:42
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.

3 participants