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

dlmalloc: account the footprint of the initial heap #496

Merged
merged 1 commit into from
May 17, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented May 10, 2024

While malloc_stats and friends are disabled and unused for wasi-libc, it's neater to be consistent.

Background: My colleagues for some reasons enabled malloc_stats and asked me why it reports negative values.

Note: Depending __heap_base, init_top() adjusts the address for alignment. I think the amount of this adjustment is reported as "used" by malloc_stats. I don't bother to "fix" it.

While malloc_stats and friends are disabled and unused for wasi-libc,
it's neater to be consistent.

Background: My colleagues for some reasons enabled malloc_stats
and asked me why it reports negative values.

Note: Depending __heap_base, init_top() adjusts the address for
alignment. I think the amount of this adjustment is reported as
"used" by malloc_stats. I don't bother to "fix" it.
@sunfishcode
Copy link
Member

This looks reasonable to me. It doesn't even look wrong that this doesn't account for init_top adjusting for alignment, because it's about the amount of memory malloc has "obtained" from the system, whether it's used for user data or not.

We may want to #ifdef out the footprint and max_footprint fields altogether by default, to save code size, because we don't expose that functionality in the default wasi-libc configuration. But we can do that separately.

@sbc100
Copy link
Member

sbc100 commented May 13, 2024

Do you know why upstream dlmalloc doesn't do this? Is this a bug in dlmalloc?

Can you mark these lines with __wasilibc_unmodified_upstream like we do elsewhere?

@sunfishcode
Copy link
Member

This code is code that we added for WASI, so it's already under __wasilibc_unmodified_upstream guards. And we don't expose the footprint functionality publicly, which is why we didn't notice this before.

@yamt
Copy link
Contributor Author

yamt commented May 13, 2024

This looks reasonable to me. It doesn't even look wrong that this doesn't account for init_top adjusting for alignment, because it's about the amount of memory malloc has "obtained" from the system, whether it's used for user data or not.

i agree it isn't necessarily wrong. i mentioned it just because my colleagues will likely have a question.

We may want to #ifdef out the footprint and max_footprint fields altogether by default, to save code size, because we don't expose that functionality in the default wasi-libc configuration. But we can do that separately.

maybe.

@sunfishcode sunfishcode merged commit a3ef152 into WebAssembly:main May 17, 2024
8 checks passed
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