This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
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.
Decommit instance memory after a runtime call on Linux #8998
Decommit instance memory after a runtime call on Linux #8998
Changes from 10 commits
c97a77e
74cf151
53e29c9
ec3cf69
5d07a20
2e07a0e
b4ee835
3943a69
dfe1170
75f04d5
56e165d
caaa7b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How sure are you that the whole instance memory is kept in one mapping? I think they can also be split up in multiple consecutive mappings (at least on macOS that is the case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Linux only code so we don't care about macOS here. Yes, potentially they can be split up on several mappings. The opposite is also possible AFAIU, when several instances are packed into a single VMA. Note though that the solution with
madvise
should work in these cases.We generally cannot be 100% sure what wasmtime does under the hood, so this is the best effort. But hey it's not the end of the world if this test fails. I see no reason it to be not a single mapping though, at least on Linux. It is simpler to implement, most efficient, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need
cfg_if
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that
madvise
is POSIX-compatible? Sure, but it's complicated. In Linux this is not merely an advice it has a very certain and widely relied semantics of eagerly freeing and zeroing the pages. macOS is, on the other hand, is very lazy about those. madvise(WONTNEED) won't have the same effect there.There are different options there to make it work on macOS, but those are out of scope for this PR. When the time comes though it will be clear where to integrate them : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we don't need the
cfg_if
.Could be just
cfg(unix)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Assuming you meant
cfg(target_os = "linux")
)Yeah, true, not necessary. I figured I use it because:
cfg(unix)
It's not something I am married to, so can change to a plain
cfg
as well. Just let me knowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah fine