Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

epaint: Memoize individual lines during text layout #5411

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

afishhh
Copy link

@afishhh afishhh commented Nov 28, 2024

This is an almost complete implementation of the approach described by emilk in this comment, excluding CoW semantics for LayoutJob (but including them for Row).
It supersedes the previous unsuccessful attempt here: #4000.

Draft because:

  • Currently individual rows will have ends_with_newline always set to false.
    This breaks selection with Ctrl+A (and probably many other things)
  • The whole block for doing the splitting and merging should probably become a function (I'll do that later).
  • I haven't run the check script, the tests, and haven't made sure all of the examples build (although I assume they probably don't rely on Galley internals).
  • Layout is sometimes incorrect (missing empty lines, wrapping sometimes makes text overlap).
  • A lot of text-related code had to be changed so this needs to be properly tested to ensure no layout issues were introduced, especially relating to the now row-relative coordinate system of Rows. Also this requires that we're fine making these very breaking changes.

It does significantly improve the performance of rendering large blocks of text (if they have many newlines), this is the test program I used to test it (adapted from #3086):

code
use eframe::egui::{self, CentralPanel, TextEdit};
use std::fmt::Write;

fn main() -> Result<(), eframe::Error> {
    let options = eframe::NativeOptions {
        ..Default::default()
    };

    eframe::run_native(
        "editor big file test",
        options,
        Box::new(|_cc| Ok(Box::<MyApp>::new(MyApp::new()))),
    )
}

struct MyApp {
    text: String,
}

impl MyApp {
    fn new() -> Self {
        let mut string = String::new();
        for line_bytes in (0..50000).map(|_| (0u8..50)) {
            for byte in line_bytes {
                write!(string, " {byte:02x}").unwrap();
            }
            write!(string, "\n").unwrap();
        }
        println!("total bytes: {}", string.len());
        MyApp { text: string }
    }
}

impl eframe::App for MyApp {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        CentralPanel::default().show(ctx, |ui| {
            let start = std::time::Instant::now();
            egui::ScrollArea::vertical().show(ui, |ui| {
                let code_editor = TextEdit::multiline(&mut self.text)
                    .code_editor()
                    .desired_width(f32::INFINITY)
                    .desired_rows(40);
                let response = code_editor.show(ui).response;
                if response.changed() {
                    println!("total bytes now: {}", self.text.len());
                }
            });
            let end = std::time::Instant::now();
            let time_to_update = end - start;
            if time_to_update.as_secs_f32() > 0.5 {
                println!("Long update took {:.3}s", time_to_update.as_secs_f32())
            }
        });
    }
}

I think the way to proceed would be to make a new type, something like PositionedRow, that would wrap an Arc<Row> but have a separate pos and ends_with_newline (that would mean Row only holds a size instead of a rect). This type would of course have getters that would allow you to easily get a Rect from it and probably a Deref to the underlying Row.
I haven't done this yet because I wanted to get some opinions whether this would be an acceptable API first. This is now implemented, but of course I'm still open to discussion about this approach and whether it's what we want to do.

Breaking changes (currently):

  • The Galley::rows field has a different type.
  • There is now a PlacedRow wrapper for Row.
  • Row now uses a coordinate system relative to itself instead of the Galley.

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5411-cache_galley_lines
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@afishhh
Copy link
Author

afishhh commented Nov 29, 2024

Okay, so I did the thing and implemented the Row wrapper. This turned out to be more difficult than expected, and requires changing the semantics of Rows themselves. glyphs and the mesh(_bounds) must be relative to the Row itself, instead of the whole Galley, as otherwise repositioning them wouldn't work (unless we did something hacky like store two different offsets). Currently I also implemented Deref<Target = Row> for the PlacedRow wrapper (bike-shedding appreciated) which may not be the best idea as it makes it easy to confuse the different coordinate systems of the offset and non-offset Row.

Currently this "somewhat" works, I've managed to fix the selection issues I was seeing, but there is some layout issues you can actually see in the live demo above where the text is overlapping sometimes when wrapped (since this was present before the whole PlacedRow mess, I think this may be related to galley-merging).

@afishhh
Copy link
Author

afishhh commented Nov 30, 2024

So I've managed to make this look pretty correct, these are the remaining issues:

  • Rows now have very weird semantics because an empty Row that is in-between two non-empty lines will actually have a size of zero. This could be solved by actually incorporating the line-terminating newline into the cached job, but there is one issue with that approach because then each of the line galleys will now contain a trailing Row that would have to be treated specially by the merging code.
  • I've definitely introduced changes in rounding which is probably what makes the snapshot tests fail (apart from the fact that they sometimes segfault on my machine).
    Issues now solved (except the rendering_tests are still utterly broken on my machine)

@afishhh
Copy link
Author

afishhh commented Nov 30, 2024

I think I've made some good progress:

  • Made Row hold ends_with_newline again and just dropped excess rows during galley-merging, this means Row once again has pretty sensible semantics.
  • Properly respecting LayoutJob::round_output_size_to_nearest_ui_point fixed a lot of the snapshot test failures.

The remaining test failures are:

  • All rendering tests seem to have slight differences but in separators not in text which suggests that sizing might be ever so slightly wrong. Turns out this also happens on master for me, so it probably is unrelated to my changes.
    It looks like I can make more tests pass by using software rendering (WGPU_ADAPTER_NAME=llvmpipe) but still not all of them (dpi_1.25 and dpi_1.75 fail), so I'm going to pass this off as the graphics driver's fault.
  • The "Misc Demos" snapshot test also seems to fail, possibly due to a slightly incorrect alignment after the button widget? I've managed to remove the most hacky line of this patch and fix this in one fell swoop!.

I'm wondering whether it is worth fixing fixing these tests, should I just regenerate the snapshots instead?
Also I think stats don't take into account the fact multiple Galleys can point to the same Row.

@afishhh afishhh marked this pull request as ready for review December 1, 2024 23:57
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I haven't reviewed all of GalleyCache yet, because it is a very dense and complex function.

I wonder how we best test and/or benchmark this though 🤔

I defiantly feel we should have a benchmark to justify this code, i.e. something that shows a big speedup before/after this PR.

Comment on lines 598 to 599
// Note we **don't** ignore the leading/trailing whitespace here!
row.size.x = target_max_x - target_min_x;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? What does this affect?

Copy link
Author

@afishhh afishhh Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now that I think about it changing this comment was a mistake, leading and trailing whitespace is actually still ignored (I don't know why I thought otherwise). I'll change it back.
However ignoring whitespace here does seem kind of strange since the current docs for Row::rect say that it includes whitespace. Wouldn't this be wrong after it passes through this function? Maybe at least that documentation could be adjusted to clarify this is only true for left-aligned non-justified text.

I also noticed a more important issue though I think, this function actually makes Rows have glyphs that are outside their PlacedRow::pos + Row::size rectangle (because it just repositions the glyphs and leaves the position unchanged). This currently breaks selecting such text. I'll try to fix this soon.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the issue found above and the comment. Although I still want to know what you think about changing the Row::size comment, in general halign is surprising with how it suddenly totally changes how the coordinates work even though it makes perfect sense in hindsight.

crates/epaint/src/tessellator.rs Outdated Show resolved Hide resolved
galley: galley.clone(),
});
galley
if job.break_on_newline {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also only do this if the text is long enough and/or actually contains newline characters?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can check for a '\n', initially I was skeptical but the actual layout is way more expensive than this string find will cost us so it's probably fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the newline part, I think adding a length check might not be worth it because it'd definitely make testing harder if an issue "only showed up if the text is longer than 1000 characters".

crates/epaint/src/text/fonts.rs Show resolved Hide resolved
galley: galley.clone(),
});
galley
if job.break_on_newline {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment, let me know if you feel this is sufficient or if you had something else in mind.

Comment on lines +740 to +746
while current != job.text.len() {
let end = job.text[current..]
.find('\n')
.map_or(job.text.len(), |i| i + current + 1);
let start = current;

let mut line_job = LayoutJob {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functions is very long.

I would love if we could break this up into parts, for instance:

let mut galleys = vec![];
for line_job in job.split_on_newlines() {
    …
}
Galley::concat(galleys)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should such LayoutJob::split_on_newlines and Galley::concat functions be public API or do we want to keep them private?

Copy link
Author

@afishhh afishhh Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into splitting out these functions but it's actually non trivial, for example the splitting actually has to do the layout while it is doing the splitting because it has to update LayoutJob::max_rows as more rows are being laid out. As for Galley::concat, it needs access to pixels_per_point for correct rounding so it'd be at the very least awkward as a public API (I guess you'd need to pass in Fonts?) and as a private one it could accept FontsImpl, it also needs access to the full LayoutJob so it can store it inside the Galley and also to check round_output_size_to_nearest_ui_point.

I think we could do something like a Galley::concat(fonts: &mut FontsImpl, job: LayoutJob, galleys: impl Iterator<Item = Galley>) -> Galley and keep splitting in GalleyCache::layout_multiline or split it out into a method of GalleyCache but it has to have access to GalleyCache so it can't be on LayoutJob. Also at that point I don't know whether scattering the implementation detail of FontsImpl into a Galley method is any more clean than keeping it in GalleyCache.

Maybe another solution would be to actually disable this line caching if LayoutJob::max_rows is finite, because in that scenario there's going to be more ways to invalidate the cached lines anyways and the performance benefits will be much more situational (any edit that will cause a line-count change will cause a re-layout of all subsequent lines). This could probably allow splitting out a LayoutJob::split_on_newlines method.

@afishhh
Copy link
Author

afishhh commented Dec 4, 2024

I wonder how we best test and/or benchmark this though 🤔

I defiantly feel we should have a benchmark to justify this code, i.e. something that shows a big speedup before/after this PR.

I agree having a benchmark would be nice, currently the only thing I have is "it looks quicker" when you have a lot of text (deletion of small segments works instantly for example).
At first I wanted to suggest a kittest benchmark but then I looked in crates/egui_demo_lib/benches/benchmark.rs and noticed there already seem to be text layout benchmarks, so it'd probably be relatively easy to do something like:

  • Have a big (few megabytes) block of text
  • Lay it out
  • In the benchmark loop:
    • Randomly remove an N-byte segment from it (into a copy)
    • Lay it out

As for tests, I've encountered issues with selection drawing multiple times while working on this at this point, so maybe it would be beneficial if this was actually checked by the test suite, it looks like mouse events are supported in kittest so maybe that's a good start. Especially since I also think selection visuals might be another good place for optimization because currently selecting the whole text will force all Rows to be cloned again.

It also looks like the rendering_test tests still seem to be failing on CI. The diffs seem to contain three lines near the middle that look like the bottom of the text background, maybe the background is slightly too small/large? I have no idea why that might be though.

@afishhh afishhh force-pushed the cache_galley_lines branch from f406701 to 40f237d Compare December 5, 2024 17:45
@afishhh
Copy link
Author

afishhh commented Dec 11, 2024

I added a benchmark similar to the scenario I described above.
On master it takes about 100ms per iteration on average, on cache_galley_lines it takes 825µs which is an improvement of around 123x. I believe this may be enough to justify the code.

@afishhh afishhh requested a review from emilk December 11, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimizing re-layout of 1MB+ pieces of text in a TextEdit
2 participants