Repository navigation
Conversation
…ntime The `overlapped-lists` feature is unified across the dependency tree, so a crate that never asked for it can still be forced to buffer and rescan events, which can turn a sub-second parse of a large document into hours. Add a per-deserializer switch that makes the sequence access stop at the first unrelated element instead of skipping and buffering it, restoring the behaviour of a build without the feature. closes tafia#885
Mingun
left a comment
There was a problem hiding this comment.
I think it will be dangerous to change the mode during parse. When mode is disabled, you still need to reach checkpoint and drain already buffered events, otherwise you will get wrong results.
| #[cfg(feature = "overlapped-lists")] | ||
| pub fn overlapped_lists(&mut self, enable: bool) -> &mut Self { | ||
| self.skip_unknown = enable; | ||
| self | ||
| } |
There was a problem hiding this comment.
It wouldn't work for your use-case if would be gated. If you does not enable overlapped-lists feature, you will not able to call this method.
| #[cfg(feature = "overlapped-lists")] | |
| pub fn overlapped_lists(&mut self, enable: bool) -> &mut Self { | |
| self.skip_unknown = enable; | |
| self | |
| } | |
| pub fn overlapped_lists(&mut self, enable: bool) -> &mut Self { | |
| #[cfg(feature = "overlapped-lists")] | |
| self.skip_unknown = enable; | |
| self | |
| } |
Also, what if you change it in-flight? It seems that it should be configurable only at creation time.
| if self.map.de.can_skip() { | ||
| self.map.de.skip()?; | ||
| continue; | ||
| } | ||
| Ok(None) |
There was a problem hiding this comment.
It would beter to have
| if self.map.de.can_skip() { | |
| self.map.de.skip()?; | |
| continue; | |
| } | |
| Ok(None) | |
| #[cfg(feature = "overlapped-lists")] | |
| if self.map.de.skip()? { | |
| continue; | |
| } | |
| Ok(None) |
| #[cfg(feature = "overlapped-lists")] | ||
| #[inline] | ||
| pub(crate) const fn can_skip(&self) -> bool { | ||
| self.skip_unknown | ||
| } | ||
|
|
There was a problem hiding this comment.
Inline this check into skip()
| #[cfg(feature = "overlapped-lists")] | |
| #[inline] | |
| pub(crate) const fn can_skip(&self) -> bool { | |
| self.skip_unknown | |
| } |
…nd inline the check into skip() The method is now always present so callers can use it unconditionally; without the overlapped-lists feature it is a no-op. skip() itself returns whether events were buffered and the seq access stops iteration when it reports false, which removes the separate can_skip() accessor. Docs say to set the mode before deserialization starts.
|
Thanks for the review, pushed
|
Closes #885
What
The
overlapped-listsfeature is unified across the dependency tree, so a crate that never enabled it can still end up buffering and rescanning events on every sequence field. As reported in #885 this turns a sub-second parse of a tens-of-MB document into hours.This adds the option suggested in the issue thread: a per-deserializer switch
Deserializer::overlapped_lists(bool)(only present with the feature, defaulttrue). When set tofalse,MapValueSeqAccess::next_element_seedreturnsNoneat the first element that does not belong to the sequence instead of callingskip(), i.e. it behaves exactly like a build without the feature. Nothing is buffered, soevent_buffer_sizebecomes irrelevant for that deserializer.Changes
src/de/mod.rs: newskip_unknownfield, publicoverlapped_lists()setter with a doctest, crate-privatecan_skip(); unit testde::tests::skip::disabledasserting the duplicate-field error and an empty skip buffer.src/de/map.rs: the twoskip()call sites checkcan_skip().Cargo.toml: feature docs mention the new switch.Changelog.md: entry under Unreleased / New Features.Verification
RED→GREEN: with the
map.rschange reverted,de::tests::skip::disabledfails withExpected `Err(Custom("duplicate field `item`"))`, but got `Ok(List { item: [(), (), ()] })`; with it, it passes.