Merge 3d1a014119 into 09165c15dc

Cargo.lock

@@ -11009,6 +11009,7 @@ dependencies = [
  "assets",
  "base64 0.22.1",
  "collections",
+ "criterion",
  "env_logger 0.11.8",
  "fs",
  "futures 0.3.32",

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

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<String> {
+    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);

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<String>,
     selection: Selection,
     pressed_link: Option<RenderedLink>,
     pressed_footnote_ref: Option<RenderedFootnoteRef>,
@@ -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<Arc<Language>> {
@@ -667,11 +687,15 @@ impl Markdown {
     }
 
     pub fn append(&mut self, text: &str, cx: &mut Context<Self>) {
-        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<SharedString>, cx: &mut Context<Self>) {
+        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<Self>) {
-        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<Self>) -> Task<()> {
-        let source = self.source.clone();
+    fn start_background_parse(&self, source: SharedString, cx: &Context<Self>) -> 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<Self>) -> 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<Self>) -> 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<Self>) -> 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");
+        });
+    }
 }