oli-obk / priroda

A graphical debugger for Rust MIR
Apache License 2.0
183 stars 13 forks source link

Avoid using `rental` #30

Closed DJMcNab closed 3 years ago

DJMcNab commented 3 years ago

rental is no longer maintained

Currently, we use rental for the highlighting cache. This is because syntect uses references to the source string everywhere. Our only read of the cache is within mark_span, which only calls styled_line_to_highlighted_html. We could work around this efficiently if styled_line_to_highlighted_html instead accepted an IntoIterator<Item=(Style, &str)>> (since that is the only capacity of the slice it uses).

We can easily store the (Style, Range<usize>)s from RangedHighlightIterator, but we would need to create a Vec<(Style, &str)> to use styled_line_to_highlighted_html. This might be an acceptable solution.

cc @trishume

DJMcNab commented 3 years ago

Rental also crashes when run by rust-analyzer's procedural macro support, because https://github.com/jpernst/rental/blob/213671ab3aab3452efd7e2290c6bb714ee327014/rental-impl/src/lib.rs#L23-L27 validates for a literal #[allow(unused)], but rust-analyzer passes it as # [allow (unused)]. To be clear, this is rental's fault, not rust-analyzer's

trishume commented 3 years ago

I haven't checked how you're cache works and how simple it is, but that at least wasn't the way I intended highlighting to be cached. There's ParseState and HighlightState structs which can be cached for any line and used to efficiently re-create highlighting starting from that line, and those structs contain no references, and the docs have some info about using them for caching. It's plausible this is much more difficult than the way you do it but I'm not sure.

I'd suggest just instantiating the Vec, especially since if you want to save on allocation you can re-use a single Vec over and over by clearing it and then using extend. Another option is copy-pasting the implementation from html.rs in syntect to your code and modifying it, it only uses public APIs and isn't that long. I might accept a PR to switch the API to iterator, but I'm not that keen on making the API super generic and thus hurting incremental compiles, for a minor performance win in a use case where generating the HTML strings probably takes way more time than storing things in a Vec (a benchmark showing it does matter would cause me to change my mind and accept a PR without hesitation).

bjorn3 commented 3 years ago

mark_span can split the highlighted file at arbitrary points to insert an html element markimg the location of the currently executing code. Using *State would require re-parsing from the start of the line every time I think. The highlighting is rather performance critical,as the code runs every time you step forward or backward in the code. This can be done multiple times a second if you click fast enough.

trishume commented 3 years ago

Syntect highlights at about 10k lines per second, which does unfortunately mean that re-highlighting a whole 100 line viewport using start-of-line caching would eat into a 16ms frame budget a bit. But that might be fine for an HTML rendering server, and re-highlighting a single line by caching the HTML and only re-rendering the HTML for the one line that needs to be re-split should be dwarfed by other costs for UI use cases.