perf: custom zero-overhead attribute extraction

[alexander-beedie]

Optimisation

Continuing to iterate through flamegraphs/profiling to tackle hotspots 👍

This PR replaces the quick_xml Attributes iterator with a custom zero-overhead RawAttrIter that operates on the raw element attribute bytes and returns name/value byte-slice pairs , integrating it via trait extension and a get_attrs! macro. It's surprisingly straightforward, and noticeably faster.

Avoids all unnecessary per-item overhead from Result wrapping, Cow and QName newtypes, etc, finds all attributes in a single pass, and quick-exits the iterator as soon as all requested attributes are identified.

Code cleanup

A pleasant side-effect of the integration is how clean the calling code becomes; despite attrs.rs (where the new code lives) being ~125 lines, this PR actually reduces the total amount of project code by nearly ~100 lines.

For example,

let mut pos_attr = None;
let mut style_attr = None;
let mut type_attr = None;
for a in c_element.attributes() {
    let a = a.map_err(XlsxError::XmlAttr)?;
    let Cow::Borrowed(val) = a.value else {
        continue;
    };
    match a.key {
        QName(b"r") => pos_attr = Some(val),
        QName(b"s") => style_attr = Some(val),
        QName(b"t") => type_attr = Some(val),
        _ => {}
    }
}

becomes...

let (pos_attr, style_attr, type_attr) =
    get_attrs!(c_element, b"r" => r, b"s" => s, b"t" => t);

...and

e.attributes()
    .filter_map(|a| a.ok())
    .find(|a| a.key == QName(b"numFmtId")).map_or(...)

is now just

e.raw_attr(b"numFmtId").map_or(...)

Also, spotted a few places that were missing entity unescaping on user-visible values like displayName, tableColumn names, and pivot field names; ensured they now go through decode_attr.

Performance

• A very consistent ~16% speedup¹ in total reading speed for xlsx workbooks, across a variety of sizes.
• Tested from 10K cells all the way up to 100M cells.

  ┌──────────────────┬──────────────┬─────────────┬─────────────┬─────────┐
  │ file             │        cells │ master (ms) │ branch (ms) │ speedup │
  ├──────────────────┼──────────────┼─────────────┼─────────────┼─────────┤
  │ test_1000000_100 │  100,000,000 │      22,456 │      18,834 │   16.1% │
  ├──────────────────┼──────────────┼─────────────┼─────────────┼─────────┤
  │ test_100000_100  │   10,000,000 │       2,217 │       1,856 │   16.3% │
  ├──────────────────┼──────────────┼─────────────┼─────────────┼─────────┤
  │ test_10000_100   │    1,000,000 │         219 │         184 │   16.2% │
  ├──────────────────┼──────────────┼─────────────┼─────────────┼─────────┤
  │ test_10000_10    │      100,000 │        22.9 │        19.2 │   16.2% │
  ├──────────────────┼──────────────┼─────────────┼─────────────┼─────────┤
  │ test_1000_10     │       10,000 │        2.35 │        1.96 │   16.5% │
  └──────────────────┴──────────────┴─────────────┴─────────────┴─────────┘

Footnotes

1. Benchmarked on: Apple Silicon M3 Max ↩

[jmcnamara]

This does give an impressive performance improvement and also nicely simplifies the code for getting attributes.

My concern would be that we are turning off the safefy guards of quick_xml and relying on well formed or consistent XML. For example I think a key/value attribute pair like <row r = "50" spans = "1:4"> with whitespace around the = wouldn't be parsed correctly even though it is valid. I think namespaces like xmlns:foo="bar" may not be handled in this change either (see also #632). Also, the matching of quoted values doesn't report errors and silently? fails.

        if quote != b'"' && quote != b'\'' {
            return None;
        }

These are use some initial, non-exhaustive issues, which could probably be fixed, and/or may exist in the current implementation anyway. But I'm not sure if I can make a judgement call on whether the (impressive 16%) performance increase is worth the additional risk.

If you are in contact with tafia you could run it by him. I could also ask the current maintainer of quick_xml for a review. He has helped out here before.

It is probably worth deciding on whether this is a worthwhile change first before trying to harden the current implementation.

[jmcnamara]

I'd suggest adding some lib tests here like:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_basic_attrs() {
        let bytes = b"key1=\"val1\" key2='val2'";
        let mut iter = RawAttrIter::new(bytes);
        assert_eq!(iter.next(), Some((&b"key1"[..], &b"val1"[..])));
        assert_eq!(iter.next(), Some((&b"key2"[..], &b"val2"[..])));
        assert_eq!(iter.next(), None);
    }

    #[test]
    fn test_whitespace_around_equals() {
        let bytes = b"key = \"value\"";
        let mut iter = RawAttrIter::new(bytes);
        assert_eq!(iter.next(), Some((&b"key"[..], &b"value"[..])));
        assert_eq!(iter.next(), None);
    }

    #[test]
    fn test_empty_value() {
        let bytes = b"key=\"\"";
        let mut iter = RawAttrIter::new(bytes);
        assert_eq!(iter.next(), Some((&b"key"[..], &b""[..])));
    }

    #[test]
    fn test_no_trailing_space() {
        let bytes = b"key=\"value\"";
        let mut iter = RawAttrIter::new(bytes);
        assert_eq!(iter.next(), Some((&b"key"[..], &b"value"[..])));
        assert_eq!(iter.next(), None);
    }
}

[alexander-beedie]

Definitely a good idea - will get back to this shortly 👌