Merged
Conversation
sbc100
reviewed
Nov 21, 2023
| // it will not return contiguous regions, which leads to use creating more | ||
| // root regions below, inefficiently. (Note that we assume our alignment is | ||
| // identical to sbrk's, see the assumptions at the start of this file.) | ||
| numBytes = (size_t)ALIGN_UP(numBytes, MALLOC_ALIGNMENT); |
Collaborator
There was a problem hiding this comment.
Is the cast needed here? numBytes is already size_t.. and I imagine that MALLOC_ALIGNMENT is too, no?
Member
Author
There was a problem hiding this comment.
The cast is needed as the output of the macro is a pointer,
emscripten/system/lib/emmalloc.c
Line 138 in b81e2a3
sbc100
approved these changes
Nov 21, 2023
Member
Author
|
This increases code size by a few bytes (the extra alignment enforcement), but it seems worth it to avoid possibly large amounts of wasted memory at runtime (or even infinite looping as found in #20704). |
sbc100
approved these changes
Nov 21, 2023
Collaborator
|
Great fix 👍 |
br0nk
pushed a commit
to br0nk/emscripten
that referenced
this pull request
Nov 29, 2023
If we give it an unaligned size, then it will align it, and then memory will seem non-contiguous. That is, if we sbrk 4 bytes from address 0 then it returns 0 (the start of the allocation), and logically we reserved the range 0-4. Our next sbrk of any amount will return 8, because sbrk aligns up the address to multiples of 8. So it seems like we have a region 0-4 and another 8-. Because of this emmalloc would generate separate root regions for them, that is, they would not get merged because of the 4 bytes of dead space in the middle. With this PR, the amount of new regions created in test_emmalloc_memvalidate_verbose goes from 1311 to 1234. I'm not sure if it's worth testing that, as it would make updating the test difficult, and this is such an internal detail of emmalloc. Also add some verbose logging that helps debug this. Noticed in emscripten-core#20704 (comment)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If we give it an unaligned size, then it will align it, and then memory will
seem non-contiguous. That is, if we sbrk 4 bytes from address 0 then it
returns 0 (the start of the allocation), and logically we reserved the range
0-4. Our next sbrk of any amount will return 8, because sbrk aligns up the
address to multiples of 8. So it seems like we have a region 0-4 and another
8-. Because of this emmalloc would generate separate root regions for
them, that is, they would not get merged because of the 4 bytes of dead
space in the middle.
With this PR, the amount of new regions created in
test_emmalloc_memvalidate_verbosegoes from 1311 to 1234. I'm not sureif it's worth testing that, as it would make updating the test difficult, and this
is such an internal detail of emmalloc.
Also add some verbose logging that helps debug this.
Noticed in #20704 (comment)