Decommit instance memory after a runtime call on Linux #8998
Conversation
pepyakin
added
A0-please_review
pepyakin
removed
the
B0-silent
Also update the doc comment.
Instead of tracking RSS for the whole process we just look at the particular mapping that is associated with the linear memory of the runtime instance
|
Thanks for the review, that was a great catch! It should be fine now. |
| fn get_map(&self, addr: usize) -> &BTreeMap<String, usize> { | ||
| &self.0 | ||
| .iter() | ||
| .find(|(range, _)| addr >= range.start && addr < range.end) | ||
| .unwrap() | ||
| .1 | ||
| } | ||
|
|
||
| pub fn get_rss(&self, addr: usize) -> Option<usize> { | ||
| self.get_map(addr).get("Rss").cloned() | ||
| } |
There was a problem hiding this comment.
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.
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
| return; | ||
| } | ||
|
|
||
| cfg_if::cfg_if! { |
There was a problem hiding this comment.
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.
I mean we don't need the cfg_if.
Could be just cfg(unix)
There was a problem hiding this comment.
(Assuming you meant cfg(target_os = "linux"))
Yeah, true, not necessary. I figured I use it because:
- it's not a new dependency. We already use it and it is a very common dep that it occurs in our dep tree many times
- as I mentioned it's likely we want other branches for macOS and/or
cfg(unix)
It's not something I am married to, so can change to a plain cfg as well. Just let me know
|
bot merge |
|
Trying merge. |
ghost
merged commit 
Right now we have a wasm runtime cache. This cache keeps a number of instantiated runtimes around. That allows the executor to serve the requests with as little latency as possible. This improves latency on the critical paths significantly.
However, the instances retain their memory between executions. This was made as an optimization. This means that if a runtime wrote into some page some physical memory will stay attached to it.
In the light of increasing the heap pages (#8892), this problem becomes more severe. With the defaults, that can lead to up 2 GiB consumed by those dirty mappings.
This PR solves this problem on Linux. The solution is to tell the kernel that we are no longer interested in the contents of the specified address range. This will lead to demouting these pages and freeing up the corresponding areas of physical memory. This will not lead to decreasing of virtual memory usage by the instances.
As a side effect on Linux this brings us back in line with #3011 after we dropped it. There is no intention to bring this back as a guarantee as of yet. In other words, wasm runtimes must not access uninitialized memory.