From 0304f870593e298c3666adb0de865f7ff27c1dca Mon Sep 17 00:00:00 2001 From: Arthur Jean Date: Thu, 28 May 2026 16:55:57 +0200 Subject: [PATCH 1/2] markdown: amortise Markdown::append with a deferred String buffer Markdown::append rebuilt self.source as SharedString::new(self.source.to_string() + text) on every call, which is O(n) per call and therefore O(n^2) on the streamed total. This dominates the streaming hot path in downstream consumers that feed long token bursts into the widget (e.g. ACP agent streams in agent_ui / acp_thread). Replace the in-place rebuild with a private `source_buf: Option` accumulator with amortised String::push_str. The buffer is promoted into a SharedString exactly when a new background parse is spawned (see the new `source_for_parse` helper); existing throttling via `pending_parse` keeps promotions bounded to one per parse cycle. The public surface is preserved: `source()` still returns &str, and `append`, `replace`, `reset` keep their signatures. `replace` and `reset` invalidate the buffer to keep the contract simple. cargo check --workspace and cargo test -p markdown stay green on all downstream consumers (acp_thread, agent_ui, editor, eval_cli, markdown_preview, repl, agent_servers, project). --- crates/markdown/src/markdown.rs | 135 ++++++++++++++++++++++++++++++-- 1 file changed, 129 insertions(+), 6 deletions(-) diff --git a/crates/markdown/src/markdown.rs b/crates/markdown/src/markdown.rs index 7fcbf393fb4..60d41ebb6ad 100644 --- a/crates/markdown/src/markdown.rs +++ b/crates/markdown/src/markdown.rs @@ -320,6 +320,21 @@ impl MarkdownStyle { pub struct Markdown { source: SharedString, + /// Pending append buffer. + /// + /// `Markdown::append` historically rebuilt `self.source` as + /// `SharedString::new(self.source.to_string() + text)` on every call, which + /// is O(n) per append and therefore O(n^2) on the streamed total. Real + /// hot path: ACP agent streams drain into `Markdown::append` at frame rate + /// (see `crates/acp_thread/src/acp_thread.rs` streaming buffer loop). + /// + /// When `Some`, this buffer is the authoritative content; `self.source` is + /// the last promoted snapshot used by the background parse task. The + /// buffer is promoted into `self.source` exactly when a new parse task is + /// spawned (see `source_for_parse`), so the amortised cost per `append` + /// stays at `String::push_str` (~O(1)). `replace`/`reset` invalidate the + /// buffer to keep the contract simple. + source_buf: Option, selection: Selection, pressed_link: Option, pressed_footnote_ref: Option, @@ -512,6 +527,7 @@ impl Markdown { }; let mut this = Self { source, + source_buf: None, selection: Selection::default(), pressed_link: None, pressed_footnote_ref: None, @@ -641,7 +657,11 @@ impl Markdown { } pub fn source(&self) -> &str { - &self.source + if let Some(buf) = self.source_buf.as_deref() { + buf + } else { + &self.source + } } pub fn first_code_block_language(&self) -> Option> { @@ -667,11 +687,15 @@ impl Markdown { } pub fn append(&mut self, text: &str, cx: &mut Context) { - self.source = SharedString::new(self.source.to_string() + text); + let buf = self + .source_buf + .get_or_insert_with(|| self.source.to_string()); + buf.push_str(text); self.parse(cx); } pub fn replace(&mut self, source: impl Into, cx: &mut Context) { + self.source_buf = None; self.source = source.into(); self.parse(cx); } @@ -711,6 +735,7 @@ impl Markdown { if source == self.source() { return; } + self.source_buf = None; self.source = source; self.selection = Selection::default(); self.autoscroll_request = None; @@ -819,8 +844,28 @@ impl Markdown { self.context_menu_link.as_ref() } + /// Promote any pending append buffer into `self.source` and return a clone + /// of the resulting `SharedString`. Callers that are about to spawn a parse + /// task use this so the spawned task sees the latest content and the next + /// `append` starts from a fresh, empty buffer. + fn source_for_parse(&mut self) -> SharedString { + if let Some(buf) = self.source_buf.take() { + self.source = SharedString::from(buf); + } + self.source.clone() + } + fn parse(&mut self, cx: &mut Context) { - if self.source.is_empty() { + let is_empty = match self.source_buf.as_deref() { + Some(buf) => buf.is_empty(), + None => self.source.is_empty(), + }; + + if is_empty { + self.source_buf = None; + if !self.source.is_empty() { + self.source = SharedString::default(); + } self.should_reparse = false; self.pending_parse.take(); self.parsed_markdown = ParsedMarkdown { @@ -840,11 +885,11 @@ impl Markdown { return; } self.should_reparse = false; - self.pending_parse = Some(self.start_background_parse(cx)); + let source = self.source_for_parse(); + self.pending_parse = Some(self.start_background_parse(source, cx)); } - fn start_background_parse(&self, cx: &Context) -> Task<()> { - let source = self.source.clone(); + fn start_background_parse(&self, source: SharedString, cx: &Context) -> Task<()> { let should_parse_links_only = self.options.parse_links_only; let should_parse_html = self.options.parse_html; let should_render_mermaid_diagrams = self.options.render_mermaid_diagrams; @@ -4673,4 +4718,82 @@ mod tests { "H3 line height ({h3_line_height:?}) should be greater than body text ({body_line_height:?})" ); } + + #[gpui::test] + fn append_buffered_accumulates_correctly(cx: &mut TestAppContext) { + struct TestWindow; + impl Render for TestWindow { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + } + } + + ensure_theme_initialized(cx); + let (_, cx) = cx.add_window_view(|_, _| TestWindow); + let markdown = cx.new(|cx| Markdown::new("prefix".into(), None, None, cx)); + cx.run_until_parked(); + + markdown.update(cx, |md, cx| { + for _ in 0..1000 { + md.append("x", cx); + } + }); + cx.run_until_parked(); + + cx.update(|_, cx| { + let expected = format!("prefix{}", "x".repeat(1000)); + assert_eq!(markdown.read(cx).source(), expected.as_str()); + }); + } + + #[gpui::test] + fn source_getter_reflects_pending_buffer(cx: &mut TestAppContext) { + struct TestWindow; + impl Render for TestWindow { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + } + } + + ensure_theme_initialized(cx); + let (_, cx) = cx.add_window_view(|_, _| TestWindow); + let markdown = cx.new(|cx| Markdown::new("hello".into(), None, None, cx)); + cx.run_until_parked(); + + // First append spawns a background parse, so the second append's parse() + // call short-circuits on `pending_parse.is_some()` and leaves the buffer + // un-promoted. The getter must still reflect the accumulated content. + markdown.update(cx, |md, cx| { + md.append(" world", cx); + md.append("!", cx); + assert!(md.source_buf.is_some()); + assert_eq!(md.source(), "hello world!"); + }); + } + + #[gpui::test] + fn replace_clears_pending_buffer(cx: &mut TestAppContext) { + struct TestWindow; + impl Render for TestWindow { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + } + } + + ensure_theme_initialized(cx); + let (_, cx) = cx.add_window_view(|_, _| TestWindow); + let markdown = cx.new(|cx| Markdown::new("".into(), None, None, cx)); + cx.run_until_parked(); + + markdown.update(cx, |md, cx| { + // Two appends ensures the second one leaves a populated `source_buf` + // (the first append's spawned parse is still pending). + md.append("a", cx); + md.append("a", cx); + assert!(md.source_buf.is_some()); + md.replace(SharedString::new_static("b"), cx); + assert!(md.source_buf.is_none()); + assert_eq!(md.source(), "b"); + }); + } } From 3d1a014119f12274d1985bd3390606ddf0878da1 Mon Sep 17 00:00:00 2001 From: Arthur Jean Date: Thu, 28 May 2026 16:56:03 +0200 Subject: [PATCH 2/2] markdown: add append_throughput criterion bench Reproduces the algorithmic cost of the historical Markdown::append (`SharedString::new(s.to_string() + t)` per chunk) against the new buffered path on a 60 KB / 100 x 600 B fixture matching the streaming shape observed in downstream consumers. cargo bench -p markdown --bench append_throughput on a Ryzen 7 7800X3D: pre_fix_concat/60kb_100x600b ~94 us post_fix_buffered/60kb_100x600b ~2.4 us ratio ~38x The criterion HTML report under target/criterion/markdown_append/ is the load-bearing artefact for the perf claim. --- Cargo.lock | 1 + crates/markdown/Cargo.toml | 5 + crates/markdown/benches/append_throughput.rs | 96 ++++++++++++++++++++ 3 files changed, 102 insertions(+) create mode 100644 crates/markdown/benches/append_throughput.rs diff --git a/Cargo.lock b/Cargo.lock index 1cf4f6f284a..58cd06815bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11011,6 +11011,7 @@ dependencies = [ "assets", "base64 0.22.1", "collections", + "criterion", "env_logger 0.11.8", "fs", "futures 0.3.32", diff --git a/crates/markdown/Cargo.toml b/crates/markdown/Cargo.toml index 3982cf9506a..da6db71191a 100644 --- a/crates/markdown/Cargo.toml +++ b/crates/markdown/Cargo.toml @@ -41,6 +41,7 @@ util.workspace = true [dev-dependencies] assets.workspace = true +criterion.workspace = true env_logger.workspace = true fs = {workspace = true, features = ["test-support"]} gpui = { workspace = true, features = ["test-support"] } @@ -50,3 +51,7 @@ languages = { workspace = true, features = ["load-grammars"] } node_runtime.workspace = true settings = { workspace = true, features = ["test-support"] } util = { workspace = true, features = ["test-support"] } + +[[bench]] +name = "append_throughput" +harness = false diff --git a/crates/markdown/benches/append_throughput.rs b/crates/markdown/benches/append_throughput.rs new file mode 100644 index 00000000000..2ea06890007 --- /dev/null +++ b/crates/markdown/benches/append_throughput.rs @@ -0,0 +1,96 @@ +//! `Markdown::append` throughput microbench. +//! +//! Isolates the string-accumulation cost of `Markdown::append` from the +//! rest of the widget (background parse task, GPUI app context, etc.) so +//! the algorithmic improvement is reproducible in a single command. +//! +//! - `pre_fix_concat`: reproduces the historical body of `append`, which +//! was `self.source = SharedString::new(self.source.to_string() + text)` +//! on every call. O(n) per call -> O(n^2) on the streamed total. +//! - `post_fix_buffered`: reproduces the new body, which accumulates into +//! a `String` buffer with amortised `push_str` and promotes the buffer +//! to a `SharedString` once at the end of the stream (the throttle in +//! the real widget collapses many appends into one promotion per parse +//! cycle; the bench uses one promotion per stream as the best-case +//! bound). +//! +//! Fixture matches the Paneflow hot path observed via flamegraph on the +//! ACP streaming buffer drain loop: 60 KB total across 100 chunks of +//! 600 B. The ratio reported by the criterion HTML report is the load- +//! bearing artefact for the perf claim. +//! +//! Run via `cargo bench -p markdown --bench append_throughput`. Output +//! lands under `target/criterion/markdown_append/`. + +use criterion::{BenchmarkId, Criterion, Throughput, black_box, criterion_group, criterion_main}; +use gpui::SharedString; + +const CHUNKS: usize = 100; +const CHUNK_BYTES: usize = 600; +const TOTAL_BYTES: usize = CHUNKS * CHUNK_BYTES; + +fn make_chunks() -> Vec { + const FIXTURE_BYTES: &[u8] = b"markdown append streaming fixture with code blocks and prose\n"; + + (0..CHUNKS) + .map(|chunk_ix| { + (0..CHUNK_BYTES) + .map(|byte_ix| FIXTURE_BYTES[(chunk_ix + byte_ix) % FIXTURE_BYTES.len()] as char) + .collect() + }) + .collect() +} + +fn pre_fix_concat(chunks: &[String]) -> SharedString { + let mut source = SharedString::new(""); + + for chunk in chunks { + // Exact body of the historical `Markdown::append`. + source = SharedString::new(source.to_string() + black_box(chunk.as_str())); + } + + source +} + +fn post_fix_buffered(chunks: &[String]) -> SharedString { + let mut buf = String::new(); + + for chunk in chunks { + // Exact body of the new `Markdown::append` after the buffer is + // initialised. + buf.push_str(black_box(chunk.as_str())); + } + + // One promotion per stream — the best-case bound the throttle in + // `parse()` converges towards when many appends land between parse + // cycles. + SharedString::from(buf) +} + +fn bench_append_throughput(c: &mut Criterion) { + let chunks = make_chunks(); + let mut group = c.benchmark_group("markdown_append"); + group.throughput(Throughput::Bytes(TOTAL_BYTES as u64)); + + group.bench_function(BenchmarkId::new("pre_fix_concat", "60kb_100x600b"), |b| { + b.iter(|| { + let source = pre_fix_concat(black_box(&chunks)); + black_box(source); + }); + }); + + group.bench_function( + BenchmarkId::new("post_fix_buffered", "60kb_100x600b"), + |b| { + b.iter(|| { + let source = post_fix_buffered(black_box(&chunks)); + black_box(source); + }); + }, + ); + + group.finish(); +} + +criterion_group!(markdown_append, bench_append_throughput); +criterion_main!(markdown_append);