Improve performance of rendering edits in preview mode - #26565
Conversation
Memory usage reportMemory usage unchanged ✅ |
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 94.54%. The percentage of expected errors that received a diagnostic held steady at 90.31%. The number of fully passing files held steady at 96/134. |
|
|
Nice @epage is this something that the upstream annotate snippets will do for us? Is this still worth landing on its own? |
|
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I had a few nits from a quick review, but I'm also curious to hear from Ed.
(I also reran the failing CI jobs, which I think were just temporary failures)
|
I've not gotten to the diff code yet so it is hard to say. |
|
Ah, but I see |
Previously, rendering edits in preview mode involved taking a diff of the entire file (for non-notebooks) for each violation/fix. Instead, we now take a small context window around the edit, and diff that.
af8c47f to
a971860
Compare
|
Agreed. I'm taking on that exploration. |
|
@ntBre I've applied your suggestions and fixed |
|
Thanks! This looks reasonable to me and seems okay to land separately from the Patchdiff --git a/crates/ruff_db/src/diagnostic/render/full.rs b/crates/ruff_db/src/diagnostic/render/full.rs
index 158aa02861..58797990aa 100644
--- a/crates/ruff_db/src/diagnostic/render/full.rs
+++ b/crates/ruff_db/src/diagnostic/render/full.rs
@@ -156,58 +156,55 @@ impl std::fmt::Display for Diff<'_> {
for (cell, offset) in cells {
let range = TextRange::new(last_end, offset);
last_end = offset;
+
+ // For non-notebooks, construct and diff only the source surrounding the edits.
+ let (range, line_offset) = if cell.is_none()
+ && let Some(first) = self.fix.edits().first()
+ && let Some(last) = self.fix.edits().last()
+ {
+ let start_line = source_code
+ .line_index(first.start())
+ .saturating_sub(DIFF_CONTEXT_WINDOW);
+ let last_source_line = source_code.line_index(source_text.text_len());
+ let end_line = source_code
+ .line_index(last.end())
+ .saturating_add(DIFF_CONTEXT_WINDOW)
+ .min(last_source_line);
+
+ (
+ TextRange::new(
+ source_code.line_start(start_line),
+ source_code.line_end(end_line),
+ ),
+ start_line.to_zero_indexed(),
+ )
+ } else {
+ (range, 0)
+ };
+
let input = source_code.slice(range);
let mut output = String::with_capacity(input.len());
let mut last_end = range.start();
- let mut first = None;
+ let mut applied = 0;
for edit in self.fix.edits() {
if range.contains_range(edit.range()) {
- first.get_or_insert(edit);
output.push_str(source_code.slice(TextRange::new(last_end, edit.start())));
output.push_str(edit.content().unwrap_or_default());
last_end = edit.end();
+ applied += 1;
}
}
// No edits were applied, so there's no need to diff.
- if first.is_none() {
+ if applied == 0 {
continue;
}
- // Length of edited output
- let edit_end = output.text_len();
-
- output.push_str(&source_text[usize::from(last_end)..usize::from(range.end())]);
-
- // For non-notebooks, only diff a context window around the edit
- // rather than the entire file. `line_offset` here is required for
- // adjusting the line numbers in the displayed diff
- let (old_slice, new_slice, line_offset) = if cell.is_none() {
- // Unwrap ok here because we applied at least one edit
- let both_start = first.unwrap().start();
-
- // Both slices start from the same point
- let both_line_start = source_code
- .line_index(both_start)
- .saturating_sub(DIFF_CONTEXT_WINDOW);
- let both_start = source_code.line_start(both_line_start);
-
- // Offsets at the end of the context windows
- let old_end = line_end_after(source_text, last_end);
- let new_end = line_end_after(&output, edit_end);
-
- // Slices just around the changes
- let old_slice = source_code.slice(TextRange::new(both_start, old_end));
- let new_slice = &output[TextRange::new(both_start, new_end)];
+ output.push_str(source_code.slice(TextRange::new(last_end, range.end())));
- (old_slice, new_slice, both_line_start.to_zero_indexed())
- } else {
- (input, output.as_str(), 0)
- };
-
- let diff = TextDiff::from_lines(old_slice, new_slice);
+ let diff = TextDiff::from_lines(input, &output);
let mut grouped_ops: Vec<Vec<DiffOp>> = Vec::new();
for group in diff.grouped_ops(FIX_CONTEXT) {
@@ -346,18 +343,6 @@ impl std::fmt::Display for Diff<'_> {
}
}
-fn line_end_after(source: &str, offset: TextSize) -> TextSize {
- let bytes = &source.as_bytes()[usize::from(offset)..];
-
- let line_end = memchr::memchr2_iter(b'\r', b'\n', bytes)
- // Skip `\r` in `\r\n` sequences (only count the `\n`).
- .filter(|&i| bytes[i] == b'\n' || !(bytes[i] == b'\r' && bytes.get(i + 1) == Some(&b'\n')))
- .nth(DIFF_CONTEXT_WINDOW)
- .map(|end| offset.to_u32().saturating_add(end.try_into().unwrap()) as usize)
- .unwrap_or(source.len());
- TextSize::try_from(line_end).expect("offset should be representable as u32")
-}
-
struct Line {
index: Option<OneIndexed>,
width: NonZeroUsize,which does seem a little simpler to me and allows deleting the new helper. It might even be a bit faster because it avoids copying the whole file into |
|
Ah yes, that makes sense, construct the slice first and edit that, instead of making slices around the edited region. I've tested this version and it doesn't affect performance either way, the string building is <1% of the function time, and |
c5908ec to
c51670a
Compare
ntBre
left a comment
There was a problem hiding this comment.
Very nice, thank you!
We can land this without resolving this question, but how did you test the performance of the final PR version? I asked Codex if this PR should close #24548, and it claims that the example there now completes in ~2 seconds with your changes on my laptop, which is awesome!
I'll verify this manually before closing the issue, but if that's true then I think the issue is resolved.
|
I tested this manually with: $ cat > generate_case.py <<'PY'
from pathlib import Path
root = Path("case")
root.mkdir()
chunk = "list[str | int | float | bool | bytes | dict[str, str] | tuple[int, str] | set[int]]"
anno = " | ".join([chunk] * 60)
for file_idx in range(6):
parts = ["from __future__ import annotations\n"]
for i in range(2500):
parts.append("\n\n\n")
parts.append(
f"def func_{file_idx}_{i}(value_{i}: {anno}) -> {anno}:\n"
f" return value_{i}\n"
)
(root / f"mod_{file_idx}.py").write_text("".join(parts))
files = sorted(root.glob("*.py"))
print("files:", len(files))
print("total_bytes:", sum(p.stat().st_size for p in files))
print("largest_bytes:", max(p.stat().st_size for p in files))
PY
$ python generate_case.py
$ timeout 180s uvx ruff@0.15.20 check --isolated --preview --select E303 --no-cache case
$ time ~/astral/worktrees/ruff/alpha/target/release/ruff check --isolated --preview --select E303 --no-cache case
~/astral/worktrees/ruff/alpha/target/release/ruff check --isolated --preview 5.64s user 1.21s system 174% cpu 3.921 totso a little slower than the 2 seconds from Codex, but much faster than 0.15.20, which still times out after 3 minutes. I'll mark this as closing #24548! |
|
Awesome, thanks @ntBre for verifying. |
Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
|
@ntBre I've fixed that comment, so should be good to go? BTW, I hacked our Ruff-fork for Fortran, fortitude, to use |
|
Perfect, thank you! fortitude also looks cool! I might have to throw it at some of the very old fortran I have lying around :) |
Summary
Previously, rendering edits in preview mode involved taking a diff of the entire file (for non-notebooks) for each separate violation/fix. Instead, we now take a small context window around the edit, and diff that instead. This gives a 1.75-3.6x speed up on real world cases.
No AI was used in generating the code or PR text.
Test Plan
Correctness tested with
cargo nextest run.Performance tested using the
profilingprofile on the python code in theruffrepo:main:Taking the min time for each, this is a 1.75x speedup. I tested other (real) projects, and this change was even faster on them.
I think this is a solution for #24548 -- although this PR is not able to complete that extreme example within the timeout of 3 minutes, it does manage to render about 75% of the violations. With a timeout of 10s, current
maingets to aboutfunc_0_32on my machine, whereas this PR gets tofunc_0_415.