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

Don't grow memory on first allocation #1712

Merged
merged 6 commits into from
Aug 6, 2023
Merged

Don't grow memory on first allocation #1712

merged 6 commits into from
Aug 6, 2023

Conversation

athei
Copy link
Contributor

@athei athei commented Mar 14, 2023

Right now the allocator always grows the memory on the first allocation. This is because it isn't aware of how much heap is available on initialization.

Growing the memory is a very expensive operation. I can imagine that this is especially bad for calls which do little work but still allocate some memory. Not sure if we have such a performance profile in our waterfall. But I can imagine that this impacts getter functions.

This solution avoids this by first using the available heap. It is not perfect as it performs a bit more work per allocation. We could probably get rid of that with enough willpower and unsafe.

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Couple questions around Wasm instructions

crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Merging #1712 (19c420b) into master (3d6784c) will decrease coverage by 0.02%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master    #1712      +/-   ##
==========================================
- Coverage   52.98%   52.97%   -0.02%     
==========================================
  Files         212      212              
  Lines        6726     6728       +2     
==========================================
  Hits         3564     3564              
- Misses       3162     3164       +2     
Files Changed Coverage Δ
crates/allocator/src/bump.rs 85.15% <37.50%> (-1.36%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@athei
Copy link
Contributor Author

athei commented Apr 30, 2023

Merged master and answered questions. Ready for another round.

Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

Did I get right that there is one available page by default, which is why this change will help?

crates/allocator/src/bump.rs Show resolved Hide resolved
@athei
Copy link
Contributor Author

athei commented Jul 21, 2023

Did I get right that there is one available page by default, which is why this change will help?

Yes. But not necessarily one. Could be more. Depends on the program. rustc will request as many initial pages as it needs to place its stack and other data. The point is that the last page will have some free space we can use without doing more allocations.

crates/allocator/src/bump.rs Show resolved Hide resolved
@athei athei merged commit c2af398 into master Aug 6, 2023
@athei athei deleted the at/avoid-grow branch August 6, 2023 19:08
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.

5 participants