-
Notifications
You must be signed in to change notification settings - Fork 64
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
vhost-user-backend: simplify the use of generics #155
Conversation
CI seems to have timed out? Probably only needs a retrigger... |
Once upon a time, we have struggled with rust generic associated types when developing the vm-memory crate, not sure about the latest progess about support of GAT. One good news is that vhost-user-backend is not as |
@jiangliu hm, do you recall the concrete problems? Maybe it was with older rust versions? The only catch that I can think of is that you cannot use a normal generic type parameter in an associated type (I think that only works with dynamic dispatch). But that is a pretty weird case anyway (why not simply leave the type open then and set it on a higher level)? |
Do you have any examples of the conversion needed to show how its an improvement? |
@stsquad: The changes done to I can draft a proposal how I was planning the simplify vhost-user in the next days. |
@stsquad here is an example train of thought for vhost-user:
The benefits may be larger if code is generic over a VhostUserBackend, but does not need to fulfill further requirements (unlike what Let me know if anything is unclear :). |
https://github.com/rust-vmm/vhost-device/pull/362/files#diff-1d8b7844066af51d6bb14aa4305e0bb53c38989c7d0da3a6765e50eccbee8e15 would be another example of code where this could simplify things. |
d4c20d8
to
8b2d589
Compare
rebased |
8b2d589
to
0f48e5f
Compare
@Ablu Sorry to just get here now, but I really like this cleanup! Would you like to rebase? |
0f48e5f
to
799ed59
Compare
rebased |
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.
LGTM, just some minor comments.
799ed59
to
e172c55
Compare
Changelog
|
@Ablu do you plan to work on https://github.com/rust-vmm/vhost-device Draft PR to be merged when we do the next release of this crate? |
Will prepare one! |
|
In order to allow zero-cost exchanging of the concrete bitmap and vring types, a lot of the generic code required using a tuple of `<VringType, BitmapType>` for parameterizing the impl's. Once code is also oblivious of the concrete backend (and a lot of code is), this tuple turns into a triplet. Juggling these three single letter generic parameters while making sure that all the type constraints (that are also changing depending on the abstraction layer) does not feel very ergonomic. While one can argue that within this crate, this would be fine since people probably know the internals, the design choice is leaking out into every consumer of vhost-user-backend. Instead of just being able to reference "some backend" one needs to copy a lot of boilerplate code for also passing the other type parameters (again, while making sure that all the constraints are met). Instead, this commit changes things to utilize associated types [1]. This makes the Bitmap and Vring types part of the backend trait and requires the implementations to spell them out. Code that just wants to use the backend without needing to know the details can now just use the trait without needing to specify the Bitmap and Vring types again. Where needed, restricting Bitmap and Vring further is still possible (though one no longer needs to copy all the existing restrictions and can keep the code more maintainable by only listing new ones). Overall, my main target was to improve the ergonomics of the consumers of the crate. But I think the change also improves the readability and maintainability within this crate. Combined, this hopefully justifies the small code breakage in consumers. No functional changes intended. No change in type flexibility intended. BREAKING CHANGE, consumers of the lib will need to adjust their code (but it should improve the general readability). Signed-off-by: Erik Schilling <erik.schilling@linaro.org> [1] https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#specifying-placeholder-types-in-trait-definitions-with-associated-types
f88b4fc
1b6f9c3
to
f88b4fc
Compare
In order to allow zero-cost exchanging of the concrete bitmap and vring
types, a lot of the generic code required using a tuple of
<VringType, BitmapType>
for parameterizing the impl's. Once code isalso oblivious of the concrete backend (and a lot of code is), this
tuple turns into a triplet. Juggling these three single letter generic
parameters while making sure that all the type constraints (that are
also changing depending on the abstraction layer) does not feel very
ergonomic.
While one can argue that within this crate, this would be fine since
people probably know the internals, the design choice is leaking out
into every consumer of vhost-user-backend. Instead of just being able
to reference "some backend" one needs to copy a lot of boilerplate code
for also passing the other type parameters (again, while making sure
that all the constraints are met).
Instead, this commit changes things to utilize associated types [1].
This makes the Bitmap and Vring types part of the backend trait and
requires the implementations to spell them out. Code that just wants to
use the backend without needing to know the details can now just use
the trait without needing to specify the Bitmap and Vring types again.
Where needed, restricting Bitmap and Vring further is still possible
(though one no longer needs to copy all the existing restrictions and
can keep the code more maintainable by only listing new ones).
Overall, my main target was to improve the ergonomics of the consumers
of the crate. But I think the change also improves the readability and
maintainability within this crate. Combined, this hopefully justifies
the small code breakage in consumers.
No functional changes intended.
No change in type flexibility intended.
BREAKING CHANGE, consumers of the lib will need to adjust their code
(but it should improve the general readability).
Signed-off-by: Erik Schilling erik.schilling@linaro.org
[1] https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#specifying-placeholder-types-in-trait-definitions-with-associated-types
Summary of the PR
Please summarize here why the changes in this PR are needed.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.