Update bincode, reduce code size of script by ~11%
#27063
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… as measured in lines of unoptimized LLVM IR by
```
cargo llvm-lines --manifest-path ports/winit/Cargo.toml \
-p script --features layout-2013 | head -n 30
```
… with https://github.com/dtolnay/cargo-llvm-lines
CC #26713
One of the top functions (in lines contribution) was a
`Visitor::visit_enum` method generated by `serde_derive` for deserializing
`keyboard_types::key::Key`, which is an enum with many variants.
I had filed serde-rs/serde#1824 to discuss this,
and dtolnay proposed pyfisch/keyboard-types#6
manually implementing `Deserialize` for that enum instead of deriving it.
However another point of note is that this function had four copies with
monomorphization. I could reproduce this in a program that does nothing
but deserialize an enum with bincode:
serde-rs/serde#1824 (comment)
This suggests that bincode uses instanciates the `Deserialize` impls
with four different `Deserializer` types which is three more than necessary.
https://dos.cafe/blog/bincode-1.3.html mentions changes to `Deserializer`
types in a new version of bincode. And sure enough, this issue is now fixed.
Before:
```
Lines Copies Function name
----- ------ -------------
7022161 (100%) 180367 (100%) (TOTAL)
178816 (2.5%) 15437 (8.6%) core::ptr::drop_in_place
151022 (2.2%) 6634 (3.7%) core::ops::function::FnOnce::call_once
122012 (1.7%) 1000 (0.6%) <<&mut bincode::de::Deserializer<R,O> as serde::de::Deserializer>::deserialize_tuple::Access<R,O> as serde::de::SeqAccess>::next_element_seed
101731 (1.4%) 1627 (0.9%) core::option::Option<T>::map
83196 (1.2%) 1192 (0.7%) core::result::Result<T,E>::map
51215 (0.7%) 6111 (3.4%) core::ops::function::FnOnce::call_once{{vtable.shim}}
47048 (0.7%) 4 (0.0%) <keyboard_types::key::_IMPL_DESERIALIZE_FOR_Key::<impl serde::de::Deserialize for keyboard_types::key::Key>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
45388 (0.6%) 296 (0.2%) <&mut bincode::de::Deserializer<R,O> as serde::de::Deserializer>::deserialize_enum::<impl serde::de::EnumAccess for &mut bincode::de::Deserializer<R,O>>::variant_seed
44973 (0.6%) 647 (0.4%) core::result::Result<T,E>::map_err
39185 (0.6%) 516 (0.3%) std::thread::local::LocalKey<T>::try_with
39130 (0.6%) 215 (0.1%) alloc::raw_vec::RawVec<T,A>::grow_amortized
35334 (0.5%) 867 (0.5%) alloc::alloc::box_free
34788 (0.5%) 609 (0.3%) crossbeam_channel::context::Context::with::{{closure}}
33810 (0.5%) 646 (0.4%) core::ptr::swap_nonoverlapping_one
33610 (0.5%) 552 (0.3%) core::result::Result<T,E>::unwrap_or_else
32144 (0.5%) 392 (0.2%) <*const T as core::fmt::Pointer>::fmt
32062 (0.5%) 782 (0.4%) script::dom::bindings::root::Root<T>::new
31589 (0.4%) 600 (0.3%) core::result::Result<T,E>::expect
29364 (0.4%) 116 (0.1%) <<script_traits::webdriver_msg::_IMPL_DESERIALIZE_FOR_WebDriverScriptCommand::<impl serde::de::Deserialize for script_traits::webdriver_msg::WebDriverScriptCommand>::deserialize::__Visitor as serde::de::Visitor>::visit_enum::__Visitor as serde::de::Visitor>::visit_seq
26996 (0.4%) 346 (0.2%) core::option::Option<T>::map_or
26168 (0.4%) 92 (0.1%) <<script_traits::_IMPL_DESERIALIZE_FOR_ConstellationControlMsg::<impl serde::de::Deserialize for script_traits::ConstellationControlMsg>::deserialize::__Visitor as serde::de::Visitor>::visit_enum::__Visitor as serde::de::Visitor>::visit_seq
25996 (0.4%) 388 (0.2%) script::dom::bindings::root::Root<script::dom::bindings::root::MaybeUnreflectedDom<T>>::reflect_with
25833 (0.4%) 641 (0.4%) core::result::Result<T,E>::unwrap
25180 (0.4%) 4 (0.0%) <keyboard_types::code::_IMPL_DESERIALIZE_FOR_Code::<impl serde::de::Deserialize for keyboard_types::code::Code>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
24751 (0.4%) 197 (0.1%) core::iter::traits::iterator::Iterator::try_fold
22216 (0.3%) 376 (0.2%) bincode::internal::deserialize_seed
22185 (0.3%) 145 (0.1%) hashbrown::raw::RawTable<T>::find
```
After:
```
Lines Copies Function name
----- ------ -------------
6239581 (100%) 169803 (100%) (TOTAL)
178770 (2.9%) 15434 (9.1%) core::ptr::drop_in_place
151022 (2.4%) 6634 (3.9%) core::ops::function::FnOnce::call_once
101146 (1.6%) 1618 (1.0%) core::option::Option<T>::map
76938 (1.2%) 1090 (0.6%) core::result::Result<T,E>::map
51215 (0.8%) 6111 (3.6%) core::ops::function::FnOnce::call_once{{vtable.shim}}
44973 (0.7%) 647 (0.4%) core::result::Result<T,E>::map_err
39185 (0.6%) 516 (0.3%) std::thread::local::LocalKey<T>::try_with
39130 (0.6%) 215 (0.1%) alloc::raw_vec::RawVec<T,A>::grow_amortized
35334 (0.6%) 867 (0.5%) alloc::alloc::box_free
34788 (0.6%) 609 (0.4%) crossbeam_channel::context::Context::with::{{closure}}
33810 (0.5%) 646 (0.4%) core::ptr::swap_nonoverlapping_one
33610 (0.5%) 552 (0.3%) core::result::Result<T,E>::unwrap_or_else
32144 (0.5%) 392 (0.2%) <*const T as core::fmt::Pointer>::fmt
32062 (0.5%) 782 (0.5%) script::dom::bindings::root::Root<T>::new
31589 (0.5%) 600 (0.4%) core::result::Result<T,E>::expect
30069 (0.5%) 250 (0.1%) <<&mut bincode::de::Deserializer<R,O> as serde::de::Deserializer>::deserialize_tuple::Access<R,O> as serde::de::SeqAccess>::next_element_seed
26996 (0.4%) 346 (0.2%) core::option::Option<T>::map_or
25996 (0.4%) 388 (0.2%) script::dom::bindings::root::Root<script::dom::bindings::root::MaybeUnreflectedDom<T>>::reflect_with
25833 (0.4%) 641 (0.4%) core::result::Result<T,E>::unwrap
24751 (0.4%) 197 (0.1%) core::iter::traits::iterator::Iterator::try_fold
22185 (0.4%) 145 (0.1%) hashbrown::raw::RawTable<T>::find
22000 (0.4%) 80 (0.0%) hashbrown::raw::RawTable<T>::rehash_in_place
20808 (0.3%) 289 (0.2%) core::alloc::layout::Layout::array
20031 (0.3%) 308 (0.2%) core::option::Option<T>::ok_or_else
19732 (0.3%) 2 (0.0%) html5ever::tree_builder::TreeBuilder<Handle,Sink>::step
18951 (0.3%) 1014 (0.6%) core::ptr::read
17261 (0.3%) 201 (0.1%) core::iter::traits::iterator::Iterator::fold
```
That `visit_enum` for `Key` is not in the first 30 lines, I find it after
changing the filtering command to `head -n 60`:
```
11762 (0.2%) 1 (0.0%) <keyboard_types::key::_IMPL_DESERIALIZE_FOR_Key::<impl serde::de::Deserialize for keyboard_types::key::Key>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
```
It has one copy / instantiation instead of four, which contributes exactly
four times fewer lines of IR.
The crate total reduced by ~782k lines, a lot more than ~35k for this
`visit_enum` function. We see that `next_element_seed` (also for bincode
deserialization) now has 250 copies instead of 1000. So it looks like
*everything* bincode related was reduced by 4×
|
@bors-servo r+ |
|
📌 Commit a215ef3 has been approved by |
highfive
added
the
S-awaiting-merge
bors-servo
added a commit
that referenced
this pull request
Jun 24, 2020
Update bincode, reduce code size of `script` by ~11%
… as measured in lines of unoptimized LLVM IR by
```
cargo llvm-lines --manifest-path ports/winit/Cargo.toml \
-p script --features layout-2013 | head -n 30
```
… with https://github.com/dtolnay/cargo-llvm-lines
CC #26713
One of the top functions (in lines contribution) was a `Visitor::visit_enum` method generated by `serde_derive` for deserializing `keyboard_types::key::Key`, which is an enum with many variants. I had filed serde-rs/serde#1824 to discuss this, and dtolnay proposed pyfisch/keyboard-types#6 manually implementing `Deserialize` for that enum instead of deriving it.
However another point of note is that this function had four copies with monomorphization. I could reproduce this in a program that does nothing but deserialize an enum with bincode: serde-rs/serde#1824 (comment) This suggests that bincode uses instanciates the `Deserialize` impls with four different `Deserializer` types which is three more than necessary.
https://dos.cafe/blog/bincode-1.3.html mentions changes to `Deserializer` types in a new version of bincode. And sure enough, this issue is now fixed.
Before:
```
Lines Copies Function name
----- ------ -------------
7022161 (100%) 180367 (100%) (TOTAL)
178816 (2.5%) 15437 (8.6%) core::ptr::drop_in_place
151022 (2.2%) 6634 (3.7%) core::ops::function::FnOnce::call_once
122012 (1.7%) 1000 (0.6%) <<&mut bincode::de::Deserializer<R,O> as serde::de::Deserializer>::deserialize_tuple::Access<R,O> as serde::de::SeqAccess>::next_element_seed
101731 (1.4%) 1627 (0.9%) core::option::Option<T>::map
83196 (1.2%) 1192 (0.7%) core::result::Result<T,E>::map
51215 (0.7%) 6111 (3.4%) core::ops::function::FnOnce::call_once{{vtable.shim}}
47048 (0.7%) 4 (0.0%) <keyboard_types::key::_IMPL_DESERIALIZE_FOR_Key::<impl serde::de::Deserialize for keyboard_types::key::Key>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
45388 (0.6%) 296 (0.2%) <&mut bincode::de::Deserializer<R,O> as serde::de::Deserializer>::deserialize_enum::<impl serde::de::EnumAccess for &mut bincode::de::Deserializer<R,O>>::variant_seed
44973 (0.6%) 647 (0.4%) core::result::Result<T,E>::map_err
39185 (0.6%) 516 (0.3%) std::thread::local::LocalKey<T>::try_with
39130 (0.6%) 215 (0.1%) alloc::raw_vec::RawVec<T,A>::grow_amortized
35334 (0.5%) 867 (0.5%) alloc::alloc::box_free
34788 (0.5%) 609 (0.3%) crossbeam_channel::context::Context::with::{{closure}}
33810 (0.5%) 646 (0.4%) core::ptr::swap_nonoverlapping_one
33610 (0.5%) 552 (0.3%) core::result::Result<T,E>::unwrap_or_else
32144 (0.5%) 392 (0.2%) <*const T as core::fmt::Pointer>::fmt
32062 (0.5%) 782 (0.4%) script::dom::bindings::root::Root<T>::new
31589 (0.4%) 600 (0.3%) core::result::Result<T,E>::expect
29364 (0.4%) 116 (0.1%) <<script_traits::webdriver_msg::_IMPL_DESERIALIZE_FOR_WebDriverScriptCommand::<impl serde::de::Deserialize for script_traits::webdriver_msg::WebDriverScriptCommand>::deserialize::__Visitor as serde::de::Visitor>::visit_enum::__Visitor as serde::de::Visitor>::visit_seq
26996 (0.4%) 346 (0.2%) core::option::Option<T>::map_or
26168 (0.4%) 92 (0.1%) <<script_traits::_IMPL_DESERIALIZE_FOR_ConstellationControlMsg::<impl serde::de::Deserialize for script_traits::ConstellationControlMsg>::deserialize::__Visitor as serde::de::Visitor>::visit_enum::__Visitor as serde::de::Visitor>::visit_seq
25996 (0.4%) 388 (0.2%) script::dom::bindings::root::Root<script::dom::bindings::root::MaybeUnreflectedDom<T>>::reflect_with
25833 (0.4%) 641 (0.4%) core::result::Result<T,E>::unwrap
25180 (0.4%) 4 (0.0%) <keyboard_types::code::_IMPL_DESERIALIZE_FOR_Code::<impl serde::de::Deserialize for keyboard_types::code::Code>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
24751 (0.4%) 197 (0.1%) core::iter::traits::iterator::Iterator::try_fold
22216 (0.3%) 376 (0.2%) bincode::internal::deserialize_seed
22185 (0.3%) 145 (0.1%) hashbrown::raw::RawTable<T>::find
```
After:
```
Lines Copies Function name
----- ------ -------------
6239581 (100%) 169803 (100%) (TOTAL)
178770 (2.9%) 15434 (9.1%) core::ptr::drop_in_place
151022 (2.4%) 6634 (3.9%) core::ops::function::FnOnce::call_once
101146 (1.6%) 1618 (1.0%) core::option::Option<T>::map
76938 (1.2%) 1090 (0.6%) core::result::Result<T,E>::map
51215 (0.8%) 6111 (3.6%) core::ops::function::FnOnce::call_once{{vtable.shim}}
44973 (0.7%) 647 (0.4%) core::result::Result<T,E>::map_err
39185 (0.6%) 516 (0.3%) std::thread::local::LocalKey<T>::try_with
39130 (0.6%) 215 (0.1%) alloc::raw_vec::RawVec<T,A>::grow_amortized
35334 (0.6%) 867 (0.5%) alloc::alloc::box_free
34788 (0.6%) 609 (0.4%) crossbeam_channel::context::Context::with::{{closure}}
33810 (0.5%) 646 (0.4%) core::ptr::swap_nonoverlapping_one
33610 (0.5%) 552 (0.3%) core::result::Result<T,E>::unwrap_or_else
32144 (0.5%) 392 (0.2%) <*const T as core::fmt::Pointer>::fmt
32062 (0.5%) 782 (0.5%) script::dom::bindings::root::Root<T>::new
31589 (0.5%) 600 (0.4%) core::result::Result<T,E>::expect
30069 (0.5%) 250 (0.1%) <<&mut bincode::de::Deserializer<R,O> as serde::de::Deserializer>::deserialize_tuple::Access<R,O> as serde::de::SeqAccess>::next_element_seed
26996 (0.4%) 346 (0.2%) core::option::Option<T>::map_or
25996 (0.4%) 388 (0.2%) script::dom::bindings::root::Root<script::dom::bindings::root::MaybeUnreflectedDom<T>>::reflect_with
25833 (0.4%) 641 (0.4%) core::result::Result<T,E>::unwrap
24751 (0.4%) 197 (0.1%) core::iter::traits::iterator::Iterator::try_fold
22185 (0.4%) 145 (0.1%) hashbrown::raw::RawTable<T>::find
22000 (0.4%) 80 (0.0%) hashbrown::raw::RawTable<T>::rehash_in_place
20808 (0.3%) 289 (0.2%) core::alloc::layout::Layout::array
20031 (0.3%) 308 (0.2%) core::option::Option<T>::ok_or_else
19732 (0.3%) 2 (0.0%) html5ever::tree_builder::TreeBuilder<Handle,Sink>::step
18951 (0.3%) 1014 (0.6%) core::ptr::read
17261 (0.3%) 201 (0.1%) core::iter::traits::iterator::Iterator::fold
```
That `visit_enum` for `Key` is not in the first 30 lines, I find it after changing the filtering command to `head -n 60`:
```
11762 (0.2%) 1 (0.0%) <keyboard_types::key::_IMPL_DESERIALIZE_FOR_Key::<impl serde::de::Deserialize for keyboard_types::key::Key>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
```
It has one copy / instantiation instead of four, and contributes exactly four times fewer lines of IR.
The crate total reduced by ~782k lines, a lot more than ~35k for this `visit_enum` function. We also see that `next_element_seed` (also for bincode deserialization) now has 250 copies instead of 1000. So it looks like *everything* bincode-related was reduced by 4×
|
💔 Test failed - status-taskcluster |
highfive
added
S-tests-failed
highfive
added
S-awaiting-merge
|
☀️ Test successful - status-taskcluster |
highfive
removed
the
S-awaiting-merge
|
I'll record my findings here In case anyone comes to this issue curious about why the code size reduced by a factor of 4. With PR #310 the serialization system stopped using the old Rust had to monomorphize each call in that function, resulting in many erroneous copies of The new config system doesn't rely on match arms and instead leverages the type system to instantiate the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
… as measured in lines of unoptimized LLVM IR by
… with https://github.com/dtolnay/cargo-llvm-lines
CC #26713
One of the top functions (in lines contribution) was a
Visitor::visit_enummethod generated byserde_derivefor deserializingkeyboard_types::key::Key, which is an enum with many variants. I had filed serde-rs/serde#1824 to discuss this, and dtolnay proposed pyfisch/keyboard-types#6 manually implementingDeserializefor that enum instead of deriving it.However another point of note is that this function had four copies with monomorphization. I could reproduce this in a program that does nothing but deserialize an enum with bincode: serde-rs/serde#1824 (comment) This suggests that bincode uses instanciates the
Deserializeimpls with four differentDeserializertypes which is three more than necessary.https://dos.cafe/blog/bincode-1.3.html mentions changes to
Deserializertypes in a new version of bincode. And sure enough, this issue is now fixed.Before:
After:
That
visit_enumforKeyis not in the first 30 lines, I find it after changing the filtering command tohead -n 60:It has one copy / instantiation instead of four, and contributes exactly four times fewer lines of IR.
The crate total reduced by ~782k lines, a lot more than ~35k for this
visit_enumfunction. We also see thatnext_element_seed(also for bincode deserialization) now has 250 copies instead of 1000. So it looks like everything bincode-related was reduced by 4×