Use smallvec for message arguments #268
Conversation
Codecov Report
@@ Coverage Diff @@
## master #268 +/- ##
==========================================
- Coverage 82.79% 82.75% -0.05%
==========================================
Files 48 48
Lines 4604 4604
==========================================
- Hits 3812 3810 -2
- Misses 792 794 +2
Continue to review full report at Codecov.
|
|
I am not sure why Rust 1.21 tests fail, |
|
On principle I approve this PR 👍 , we just need to figure out why rust 1.21 fails. Looks like the generated scanner code has an issue somehow, unfortunately travis truncated the log, so this will need to be run locally. Given this is going to be a breaking change, and with the possibility of going to rust 2018, this will likely happen, but I need to decide a few things, depending on if / how I want to address #189. So I'll likely let this PR sit a few days while I decide the path I want to take. |
|
For now I've bumped MSRV to 1.24, which is required by |
|
Thinking back about this, I have one slight source of uneasiness: this would make smallvec part of the public API of |
Any message with 4 argument or less will have these arguments stored inline rather than in an heap-allocated vec. The number 4 was chose because almost all messages have 4 arguments or less, and some potentially very spammy messages (wl_touch.move) have exactly 4 arguments. Avoiding allocations in these cases should generally be a gain. Moreother, to avoid bloating the size of Message due to this, String and Array arguments are further boxed, reducing the size of Argument from 4*usize to 2*usize. These kind of arguments are generally pretty rare, so the double allocation should overall not counter the size gain. Closes #268, #249
Any message with 4 argument or less will have these arguments stored inline rather than in an heap-allocated vec. The number 4 was chose because almost all messages have 4 arguments or less, and some potentially very spammy messages (wl_touch.move) have exactly 4 arguments. Avoiding allocations in these cases should generally be a gain. Moreother, to avoid bloating the size of Message due to this, String and Array arguments are further boxed, reducing the size of Argument from 4*usize to 2*usize. These kind of arguments are generally pretty rare, so the double allocation should overall not counter the size gain. Closes #268, #249
Any message with 4 argument or less will have these arguments stored inline rather than in an heap-allocated vec. The number 4 was chose because almost all messages have 4 arguments or less, and some potentially very spammy messages (wl_touch.move) have exactly 4 arguments. Avoiding allocations in these cases should generally be a gain. Moreother, to avoid bloating the size of Message due to this, String and Array arguments are further boxed, reducing the size of Argument from 4*usize to 2*usize. These kind of arguments are generally pretty rare, so the double allocation should overall not counter the size gain. Closes #268, #249
Any message with 4 argument or less will have these arguments stored inline rather than in an heap-allocated vec. The number 4 was chose because almost all messages have 4 arguments or less, and some potentially very spammy messages (wl_touch.move) have exactly 4 arguments. Avoiding allocations in these cases should generally be a gain. Moreother, to avoid bloating the size of Message due to this, String and Array arguments are further boxed, reducing the size of Argument from 4*usize to 2*usize. These kind of arguments are generally pretty rare, so the double allocation should overall not counter the size gain. Closes #268, #249
Do you plan to release v1.0 versions in a near future? Even if you don't think that UPD: Relevant issue: servo/rust-smallvec#73. Citing it:
UPD2: Ah, I haven't noticed #273. |
|
Yes, I've integrated it in #273. Did it in a way so minimize the API impact of upgrading smallvec : the code generated by wayland-scanner directly uses the smallvec re-exported by wayland-commons. So people using wayland-rs with their own wayland extensions should not have any issue with updates. EDIT: also yes, I'm hoping to release a 1.0 in the nearish future, if no big issues are found with the 0.24 API. |
Closes #249
Unfortunately it's a backwards incompatible change, at lease for
wayland-commonsand probably for other crates which use it in public API.Maybe use this chance to update to 2018 edition?