Several enhancements to GuestMemory and VolatileSlice #101
Conversation
jiangliu
force-pushed
the
enhancement
branch
3 times, most recently
from
f1858d1 to
7d9b415
Compare
|
Looks great, but I would like to understand the actual performance benefit from "Optimize fast path for GuestMemory::read/write()", since it introduces a little more complexity. No objection at all about the other patches! |
|
@lauralt is working on adding performance tests for vm-memory. Should we first have some performance tests in place to actually see what is the impact of the patches? I'll let @lauralt and @alexandruag talk more about our findings here. |
|
Hi! Like Andreea mentioned, Laura wrote some tests and we've been playing around with them using the code available here. They are simple synthetic benchmarks, but paint an interesting initial picture to do some comparative analysis (btw, if you run them, there will be some graphs at the end somewhere in the It looks quite important to have established ways of detecting regressions and/or measuring improvements as changes to vm-memory continue to happen (the same goes for other crates as well). We'll start a discussion about it soon to gather feedback and ideas from ppl. Also, regarding vm-memory in particular, I think we should take a good look at the experiences we had so far, validate some of the previous assumptions, polish (simplify if possible) the interface, and then work on various optimizations in earnest. The interface part is particularly important, because we've began using it in other components that are being developed (i.e. Virtio stuff), and we should ensure we're happy with it while changes are still relatively straightforward to make. |
So let's move "Optimize fast path for GuestMemory::read/write()" out of this MR, and open another dedicated one for it:) |
jiangliu
force-pushed
the
enhancement
branch
2 times, most recently
from
5213f9c to
ac1901e
Compare
With the recently added benchmark cases, it turns out that the optimization doesn't help much, so I will remove this patch. |
jiangliu
force-pushed
the
enhancement
branch
2 times, most recently
from
0e6eef7 to
f00a14b
Compare
sboeuf
changed the title
jiangliu
force-pushed
the
enhancement
branch
2 times, most recently
from
9c98d21 to
9195782
Compare
jiangliu
force-pushed
the
enhancement
branch
3 times, most recently
from
f038dde to
710aed4
Compare
|
Looks good! I clicked update branch and it seems to have added a merge commit, sorry :( Also the changelog update should go into the |
jiangliu
force-pushed
the
enhancement
branch
from
534acd9 to
5e89e1b
Compare
Add benchmark cases for VolatileSlice/VolatileArrarRef interfaces. Signed-off-by: Liu Jiang <[email protected]>
Move implementation of copy_slice() into a submodule, so we could provide different implenations on different platforms. Signed-off-by: Liu Jiang <[email protected]>
It's a common case to use copy_to/from() to copy byte slice, so
optimize it by avoiding explicitly loop.
This optimization helps to reduce 90% time of byte stream copy.
VolatileSlice::copy_to_u8
time: [70.407 ns 70.728 ns 71.212 ns]
change: [-93.751% -93.706% -93.667%] (p = 0.00 < 0.05)
Performance has improved.
Found 16 outliers among 200 measurements (8.00%)
7 (3.50%) high mild
9 (4.50%) high severe
Benchmarking VolatileSlice::copy_to_u16:
VolatileSlice::copy_to_u16
time: [560.10 ns 562.15 ns 565.42 ns]
change: [-1.9847% -1.4879% -0.9829%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 11 outliers among 200 measurements (5.50%)
4 (2.00%) high mild
7 (3.50%) high severe
Benchmarking VolatileSlice::copy_from_u8:
VolatileSlice::copy_from_u8
time: [69.704 ns 69.925 ns 70.285 ns]
change: [-94.918% -94.873% -94.819%] (p = 0.00 < 0.05)
Performance has improved.
Found 14 outliers among 200 measurements (7.00%)
7 (3.50%) high mild
7 (3.50%) high severe
Benchmarking VolatileSlice::copy_from_u16:
VolatileSlice::copy_from_u16
time: [682.23 ns 685.20 ns 690.26 ns]
change: [-0.7430% -0.0145% +0.8123%] (p = 0.97 > 0.05)
No change in performance detected.
Found 13 outliers among 200 measurements (6.50%)
4 (2.00%) high mild
9 (4.50%) high severe
Signed-off-by: Liu Jiang <[email protected]>
Current implementation of GuestMemory::checked_offset() validates the GuestAddress(base + offset) is valid. By auditting invocations of GuestMemory::checked_offset(), most callers expect that the whole range [base, base + offset) is valid. So add a new interface check_range() to validate the address range. Signed-off-by: Liu Jiang <[email protected]>
warning: this import is redundant --> src/mmap_unix.rs:20:1 | 20 | use libc; | ^^^^^^^^^ help: remove it entirely Signed-off-by: Liu Jiang <[email protected]>
jiangliu
force-pushed
the
enhancement
branch
from
5e89e1b to
325ab8a
Compare
No problem, fixed and rebased to the latest master branch:) |
No description provided.