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

Disable emmalloc_trim #23344

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 8, 2025

I believe this function has not been working correctly since #13442 when sbrk stopped accepting negative values.

Maybe we can find a way to bring this functionality back but disabling it for now seems safest.

See: #23343

I believe this function has no been working correctly since emscripten-core#13442
when sbrk stopped accepting negative values.

Maybe we can find a way to bring this functionality back but disabling
it for now seems safest.

See: emscripten-core#23343
@sbc100 sbc100 requested review from juj and kripken January 8, 2025 21:37
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 8, 2025

If you look at the changes to test_emmalloc_trim.out which were part of #13442 you can see the sbrk() value stops changing. i.e. it could be reduces prior to #13442 but can no longer be reduces afterward.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 8, 2025

Since its been broken for ~4 years I guess nobody is using it ?

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely unused, yeah.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 8, 2025

This is why it would have been good to have #23330 all along. If we had that landed 4 years ago this breakage would have happened since that assert would have fired.

@sbc100 sbc100 merged commit 3fcc4d7 into emscripten-core:main Jan 8, 2025
28 of 29 checks passed
@sbc100 sbc100 deleted the disable_emmalloc_trim branch January 8, 2025 23:23
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