Buffered Reading

[FelixKahle]

Hey there,

I’m writing to you today because I’m super excited about this library I found. It’s an excellent library, and I was really disappointed with the serde-xml library from quick_xml, so I was glad to discover this one.

However, I looked at the source code for the quick-xml implementation and noticed that it doesn’t use Buffered Reading. Is that correct? If so, are there any plans to change that? I’d love to know if there’s any progress on this.

Thanks for your time,
Felix

[lukasfri]

Hey, thanks for taking an interest! I haven't announced xmlity publicly yet (reddit etc) cause I didn't feel it was really ready, but I'm happy to hear that you enjoy it!

The issue with buffering isn't something that can be fixed here but in quick-xml. The issue is that the quick-xml NsReader only supports string slices as inputs, so I'm limited by that, but if quick-xml were to update, I'd love to change it up to allow for BufRead with Seek types.

I've been thinking of doing some work and submitting a PR upstream, but haven't had the time. Will maybe happen some time this fall.

[FelixKahle]

Thank you for the quick response!
I just wanted to point out that quick-xml does support consuming from a BufRead instead of requiring a full string slice:

/// A low level encoding-agnostic XML event reader that performs namespace resolution.
///
/// Consumes a [`BufRead`] and streams XML `Event`s.
#[derive(Debug, Clone)]
pub struct NsReader<R> {
    /// An XML reader
    pub(super) reader: Reader<R>,
    /// A buffer to manage namespaces
    ns_resolver: NamespaceResolver,
    /// We cannot pop data from the namespace stack until returned `Empty` or `End`
    /// event will be processed by the user, so we only mark that we should that
    /// in the next [`Self::read_event_impl()`] call.
    pending_pop: bool,
}

I've cloned the xmlity repository and started implementing buffered reading using BufRead:

/// This reader wraps the `quick_xml::NsReader` and provides a way to peek and read events from the XML stream, as well as observe the depth, which are properties used when deserializing.
#[derive(Debug, Clone)]
struct Reader<R: BufRead> {
    reader: NsReader<R>,
    buffer: Vec<u8>,
    current_depth: i16,
    peeked_event: Option<Event<'static>>,
}

To make this work, I had to give peeked_event a 'static lifetime. While this introduces some trade-offs, I think it's a reasonable compromise for enabling streaming support.

The event reader method now looks like this:

fn read_event(&mut self) -> Result<Option<Event<'static>>, Error> {
        self.buffer.clear();
        match self.reader.read_event_into(&mut self.buffer)? {
            Event::Eof => Ok(None),
            event => Ok(Some(event.into_owned())),
        }
    }

I realize this is still a draft, but I’d appreciate any feedback or thoughts on the general direction.

[lukasfri]

Hey!

I had totally missed that part of the API. 🤦

When I wrote the deserializer, I only found NsReader::read_event and not NsReader::read_event_into but yeah you're totally right.

With it being possible, I'd suggest looking at re-implementing it using a seek-able reader. If you've looked deeper into the xmlity_quick_xml::Deserializer, you know how I've kinda abused the reader by cloning it repeatedly to "checkpoint" the de-serialization, since it needs to rewind if it hits a dead-end. This is fine for &str, but very bad for other readers. In other words, the xmlity_quick_xml::Deserializer should probably be built around a reader that requires BufRead + Seek, and then store positions that it can seek back to instead of cloning the BufRead which might not be possible and probably is very inefficient.

If you want to take this on, I'll happily work with you on it.

Thanks for your effort!
//Lukas

[FelixKahle]

Hey Lukas,

Thanks for the detailed message — I really appreciate the insight! I completely agree that relying on read_event_into and making the deserializer work with a BufRead + Seek interface would be a much cleaner and more efficient approach.

I’d love to work on this with you! I still need a bit of time to familiarize myself more deeply with the current Reader implementation. Once I’m up to speed, I’ll look into how we can refactor it around proper seek-based checkpointing.

Thanks again for the opportunity – really looking forward to contributing here!

Best,
Felix

[FelixKahle]

I made a little Sketch:

#[derive(Debug, Clone)]
struct Resolver<'a, R> {
    reader: &'a NsReader<R>,
}

impl<'a, R> Resolver<'a, R> {
    pub fn new(reader: &'a NsReader<R>) -> Self {
        Self { reader }
    }

    pub fn resolve_qname<'n>(&'n self, qname: QuickName<'n>, attribute: bool) -> ExpandedName<'n> {
        let (resolve_result, _) = self.reader.resolve(qname, attribute);
        let namespace = xml_namespace_from_resolve_result(resolve_result);

        ExpandedName::new(LocalName::from_quick_xml(qname.local_name()), namespace)
    }

    pub fn resolve_bytes_start<'n>(&'n self, bytes_start: &'n BytesStart<'n>) -> ExpandedName<'n> {
        self.resolve_qname(bytes_start.name(), false)
    }

    pub fn resolve_attribute<'n>(&'n self, attribute: &'n Attribute<'n>) -> ExpandedName<'n> {
        self.resolve_qname(attribute.key, true)
    }
}

impl<'a, R> From<&'a NsReader<R>> for Resolver<'a, R> {
    fn from(reader: &'a NsReader<R>) -> Self {
        Self::new(reader)
    }
}

trait Reader {
    type Error;
    type Event<'e> where Self: 'e;
    fn read_event<'e>(&'e mut self) -> Result<Option<Self::Event<'e>>, Self::Error>;
    fn next_event<'e>(&'e mut self) -> Result<Option<Self::Event<'e>>, Self::Error>;
    fn peek_event<'e>(&'e mut self) -> Result<Option<&'e Self::Event<'e>>, Self::Error>;
    fn current_depth(&self) -> i16;
}

• SliceReader<'i>: implements Reader by borrowing from &'i [u8], zero‑copy.
• BufferedReader<R: BufRead + Seek>: implements Reader by owning an internal Vec<u8>, reusing it on each call.

[lukasfri]

Hey!

A few notes:

• In my design, read_event has been an internal implementations detail - it is used by next_event and peek_event depending on if we have a stored event or not.
• I'm not quite sure what you're aiming for with the resolver, but I'm open for anything. One thing of note with the resolver wrapping a reference is that you'll need to reconstruct the Resolver any time you want to resolve since you can't have a reference to an adjacent field. That is why those methods are currently implemented on the xmlity_quick_xml::Reader which wraps the NsReader.

Finally, feel free to open a "Draft PR", I think that's the best way for me to see progress. It's a github feature which basically shows in the PR-list, but can't be merged until marked as ready.

One thing you'll need to tackle is that when rewinding with seek, you'll have to reset the NsReader back to its rewound state, and as such you need to store its state too along with the reader position. This is due to the state of all namespaces (default, bound prefixes) being dependent on position.

Good work!
Lukas

[FelixKahle]

Hi, I tried to pull out the resolving methods as both Reader implantations will need it. I could have thrown them into the trait as well of course with a default implementation, I think that's a design choice. Constructing it repeatedly isn't really a problem as it's just a wrapper around a reference, that should not introduce any notable overhead.
For the rewinding I probably introduce some kind of struct that remembers the position, nothing to fancy.

[FelixKahle]

Hi, I made the first real changes (nothing major yet).
Unfortunately, I can’t push a branch directly — would you prefer I fork the repo and open a Draft PR from there?
Let me know what works best for you.

[lukasfri]

Hi, I tried to pull out the resolving methods as both Reader implantations will need it. I could have thrown them into the trait as well of course with a default implementation, I think that's a design choice. Constructing it repeatedly isn't really a problem as it's just a wrapper around a reference, that should not introduce any notable overhead.

Fair enough, and yeah the overhead should be non-existent.

For the rewinding I probably introduce some kind of struct that remembers the position, nothing to fancy.

The issue is that you'll need to undo changes to the namespace state too. If the reader has passed through namespace declarations and then need to rewind to before them, they need to be removed after being rewound, and previous namespace declarations that might've been overwritten must be restored.

Hi, I made the first real changes (nothing major yet). Unfortunately, I can’t push a branch directly — would you prefer I fork the repo and open a Draft PR from there? Let me know what works best for you.

Yeah exactly, create a fork and then open a Draft PR.

[FelixKahle]

Hi Lukas,
Will do, I am a little stressed out currently with studies and work - I get to it as soon as possible.

Best
Felix

[lukasfri]

Hey, sorry for not getting back to you sooner. Absolutely no problem! I've got summer vacation myself so right now work is a bit slower than usual. Take your time, just let me know if you decide you don't want to continue working on it.