Refactor serialization to avoid reversing or collecting iterators #641
Conversation
|
I have been wanting to do that for a while, so thank you for that |
| pub struct FieldsForSerializeIter<'mem, 'facet, 'shape> { | ||
| stack: Vec<FieldIter<'mem, 'facet, 'shape>>, | ||
| } |
There was a problem hiding this comment.
This type is a hand-written iterator to re-create the old fields_for_serialize() impl Trait method. This was necessary so that facet-serialize could store a nameable iterator type (I also tried Box<dyn Iterator<...>> but hit some weird lifetime bounds issues... plus I didn't feel great about needing an extra box for this
(One minor plus to this implementation is that it's no longer recursive!)
| @@ -473,9 +494,11 @@ where | |||
| let fields = peek_struct.fields_for_serialize().count(); | |||
There was a problem hiding this comment.
Oh, one really subtle thing that I hate!! this is iterating over all the flattened fields twice: once here to count the number of flattened fields, and once while doing the actual serialization
This is needed so we can call serializer.start_object(Some(_)), but I couldn't figure out an efficient way to count the number of flattened fields without just iterating (including allocating a vec)...
If there's a way to impl ExactSizedIterator for FieldsForSerializeIter then we could avoid this
There was a problem hiding this comment.
If there's a way to impl
ExactSizedIteratorforFieldsForSerializeIterthen we could avoid this
Not sure about this, but maybe we can remove the hint...
|
I thought this was merged and I merged some PRs that introduce conflicts :( Feel free to merge it yourself as soon as the conflicts are resolved @kylewlacy |
kylewlacy
force-pushed
the
serialize-without-iter-rev
branch
from
8e39735 to
258d9eb
Compare
This PR makes it so
facet-serializeno longer requiresDoubleEndedIteratorwhen iterating over values. In turn, this also means that serialization can use plain iterator values in more places, without resorting to collecting e.g. into aVec.The "trick" is in
SerialzeTask, which now holds iterators wherever possible. When popped from the stack, it gets the next value from the iterator then pushes it back onto the stack (until the iterator is empty).I was hoping this would lead to perf improvements, but locally, I wasn't seeing a noticeable difference... I still felt like this change had other minor benefits (e.g. less memory use during iteration,
iter_vtableforHashMapno longer needs to collect its keys into a vec, etc.)