zesterer / ariadne

A fancy diagnostics & error reporting crate
https://crates.io/crates/ariadne
MIT License
1.79k stars 78 forks source link

Reduce string copies in Source #89

Closed goto-bus-stop closed 1 year ago

goto-bus-stop commented 1 year ago

The Source structure stores an owned String for each individual line. This can cause many allocations for large documents. We regularly deal with 65 000 lines or more that only have a handful of diagnostics.

This PR makes two changes:

  1. It separates string storage and line storage, Line{} now only contains indices into the string. Both character and byte indices are calculated in Source::from so the string storage can be sliced efficiently. You now have to go through Source::get_line_text(Line) to get the source text for a line. This commit still copies the text into Source but it does it in one allocation.
  2. It makes Source generic over the string storage. This does add some complexity, as the genericity also requires having an associated string storage type on Cache implementations. But, it lets you use Arc<str> as a storage type to avoid having to copy the whole source text into ariadne, or even &str if you can guarantee the lifetime. The FileCache now no longer copies each file's contents immediately after reading it. The sources() function will use string references if possible. Only users of custom caches should be affected by this change.

With these changes you can use ariadne without copying any strings, the only allocation is for computing line offsets (and for individual diagnostics of course).

goto-bus-stop commented 1 year ago

Yeah I started out with type Storage = String and got yelled at 🥲

Defaulting to String makes sense, mostly eases the upgrade path a bit for people currently writing Source in return types or struct fields.

zesterer commented 1 year ago

Thanks very much for the PR!

goto-bus-stop commented 1 year ago

thanks for reviewing so fast! 🤩

zesterer commented 1 year ago

Were you looking for a release with this in the near term, or are you ok to wait for a while?

goto-bus-stop commented 1 year ago

It's not critical, so please pick as you see fit :)