From 0edc95fa35e4b6e403e116d4e224ad35329b0159 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:18:31 -0700 Subject: [PATCH] perf(editor): cut per-keystroke work on two rich-markdown paths (#10862) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(editor): cut per-keystroke work on two rich-markdown paths Doc links: both plugins walked every text node and ran matchAll on each — the auto-convert appendTransaction once per keystroke, the preview decorations once per keystroke and again per caret move. A link needs `[[`, so gate on a native substring check first. The two walks had duplicated their guard sequence; they now share one predicate. 3.1x-3.8x over the repo's own markdown. Annotations: resolving a comment's block re-serializes the whole document (every node, plus every adjacent pair), and both the highlight-range and comment-at-position paths did that once per comment — O(comments x document). Build the blocks once and pass them down. On a 12-node fixture with 8 comments that is 184 serializations down to 23. * test(editor): pin the one-build serialization baseline Review feedback, all four points: - The serialize-count assertions compared many-comments against one-comment, so they would have passed if BOTH built blocks twice. Pin the absolute count (23 = 12 nodes + 11 adjacent pairs) derived from the fixture size, so a regression to per-comment building fails instead of comparing equal. Verified by reverting the hoist: 2 tests fail. - Skip an empty benchmark corpus instead of evaluating `index % 0` and dereferencing undefined. - Build fixture paths with path.join. - Condense the benchmark header to purpose plus parity guarantee. Co-authored-by: Orca * test(editor): harden doc-link performance evidence Co-authored-by: Orca --------- Co-authored-by: Orca --- .../rich-markdown-doc-link-scan-benchmark.mjs | 202 ++++++++++++++++++ .../rich-markdown-doc-link-code-context.ts | 16 -- .../editor/rich-markdown-doc-link-scan.ts | 32 +++ .../editor/rich-markdown-doc-link.test.ts | 34 ++- .../editor/rich-markdown-doc-link.ts | 16 +- .../rich-markdown-review-annotations.test.ts | 80 ++++++- .../rich-markdown-review-annotations.ts | 25 ++- 7 files changed, 362 insertions(+), 43 deletions(-) create mode 100644 config/scripts/rich-markdown-doc-link-scan-benchmark.mjs delete mode 100644 src/renderer/src/components/editor/rich-markdown-doc-link-code-context.ts create mode 100644 src/renderer/src/components/editor/rich-markdown-doc-link-scan.ts diff --git a/config/scripts/rich-markdown-doc-link-scan-benchmark.mjs b/config/scripts/rich-markdown-doc-link-scan-benchmark.mjs new file mode 100644 index 000000000..e0f88bd41 --- /dev/null +++ b/config/scripts/rich-markdown-doc-link-scan-benchmark.mjs @@ -0,0 +1,202 @@ +#!/usr/bin/env node +// Measures the complete ProseMirror doc traversal used by the two doc-link plugins. +import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { performance } from 'node:perf_hooks' +import { fileURLToPath } from 'node:url' +import { Schema } from '@tiptap/pm/model' +import { + canHoldDocLink, + DOC_LINK_PATTERN, + isDocLinkLiteralCodeTextNode +} from '../../src/renderer/src/components/editor/rich-markdown-doc-link-scan.ts' + +const REPO_ROOT = fileURLToPath(new URL('../..', import.meta.url)) +const ITERATIONS = Number(process.env.ORCA_DOC_LINK_BENCH_ITERATIONS ?? '41') +const WARMUP_ITERATIONS = Math.min(9, ITERATIONS) + +if (!Number.isSafeInteger(ITERATIONS) || ITERATIONS <= 0) { + throw new Error(`ORCA_DOC_LINK_BENCH_ITERATIONS must be a positive integer, got ${ITERATIONS}`) +} + +const schema = new Schema({ + nodes: { + doc: { content: 'paragraph+' }, + paragraph: { content: 'text*' }, + text: { group: 'inline' } + } +}) + +function walkUngated(doc) { + let matches = 0 + let visited = 0 + doc.descendants((node, _pos, parent) => { + visited += 1 + if (node.type.name !== 'text' || !node.text || isDocLinkLiteralCodeTextNode(node, parent)) { + return + } + for (const _match of node.text.matchAll(DOC_LINK_PATTERN)) { + matches += 1 + } + }) + return { matches, visited } +} + +function walkGated(doc) { + let matches = 0 + let visited = 0 + doc.descendants((node, _pos, parent) => { + visited += 1 + if (!canHoldDocLink(node, parent)) { + return + } + for (const _match of node.text.matchAll(DOC_LINK_PATTERN)) { + matches += 1 + } + }) + return { matches, visited } +} + +function countMatches(source) { + let matches = 0 + for (const _match of source.matchAll(DOC_LINK_PATTERN)) { + matches += 1 + } + return matches +} + +function loadDocs() { + const files = execFileSync('git', ['ls-files', '*.md', 'docs/*.md'], { + cwd: REPO_ROOT, + maxBuffer: 256 * 1024 * 1024 + }) + .toString() + .split('\n') + .filter(Boolean) + const docs = [] + for (const file of files) { + try { + const source = readFileSync(join(REPO_ROOT, file), 'utf8') + const lines = source.split('\n').filter(Boolean) + if (lines.length > 0) { + docs.push({ file, lines, size: source.length, matches: countMatches(source) }) + } + } catch { + // Indexed paths can disappear while the benchmark is running. + } + } + return docs +} + +function createFixture(doc, nonce) { + const paragraphs = doc.lines.map((line, index) => { + const text = index === 0 ? `${line} bench-${nonce}` : line + return schema.node('paragraph', null, text ? schema.text(text) : undefined) + }) + return schema.node('doc', null, paragraphs) +} + +function median(samples) { + const sorted = [...samples].sort((a, b) => a - b) + return sorted[Math.floor(sorted.length / 2)] +} + +function measureCorpus(docs) { + const samples = { ungated: [], gated: [] } + const totals = { + ungated: { matches: 0, visited: 0 }, + gated: { matches: 0, visited: 0 } + } + const seenFixtures = new WeakSet() + let expectedMatches = 0 + let expectedVisited = 0 + const measuredOrder = [] + + for (let round = -WARMUP_ITERATIONS; round < ITERATIONS; round += 1) { + const doc = docs[(round + WARMUP_ITERATIONS) % docs.length] + const measured = round >= 0 + const order = + (round + WARMUP_ITERATIONS) % 2 === 0 ? ['ungated', 'gated'] : ['gated', 'ungated'] + const fixtures = { + ungated: createFixture(doc, String(round)), + gated: createFixture(doc, String(round)) + } + + for (const arm of order) { + const fixture = fixtures[arm] + if (seenFixtures.has(fixture)) { + throw new Error('timed fixture was reused') + } + seenFixtures.add(fixture) + const start = performance.now() + const result = arm === 'ungated' ? walkUngated(fixture) : walkGated(fixture) + const elapsed = performance.now() - start + if (measured) { + samples[arm].push(elapsed) + totals[arm].matches += result.matches + totals[arm].visited += result.visited + measuredOrder.push(arm) + } + } + if (measured) { + expectedMatches += doc.matches + expectedVisited += doc.lines.length * 2 + } + } + + if (totals.ungated.matches !== expectedMatches || totals.gated.matches !== expectedMatches) { + throw new Error( + `gate changed matches: expected ${expectedMatches}, ungated ${totals.ungated.matches}, gated ${totals.gated.matches}` + ) + } + if (totals.ungated.visited !== expectedVisited || totals.gated.visited !== expectedVisited) { + throw new Error( + `full traversal result was not consumed: expected ${expectedVisited}, ungated ${totals.ungated.visited}, gated ${totals.gated.visited}` + ) + } + for (let index = 0; index < measuredOrder.length; index += 2) { + const pair = measuredOrder.slice(index, index + 2).join(',') + const previousPair = index === 0 ? null : measuredOrder.slice(index - 2, index).join(',') + if (!['ungated,gated', 'gated,ungated'].includes(pair) || pair === previousPair) { + throw new Error('benchmark arms were not interleaved in alternating order') + } + } + + return { ungated: median(samples.ungated), gated: median(samples.gated) } +} + +const docs = loadDocs() +if (docs.length === 0) { + throw new Error('no markdown files found in the index') +} +const large = docs.filter((doc) => doc.size > 3000) +const biggest = docs.reduce((a, b) => (b.size > a.size ? b : a)) +const pad = (value, width) => String(value).padStart(width) + +console.log('Doc-link ProseMirror traversal, per editor transaction. Lower is better.') +console.log( + `docs=${docs.length} (>3KB: ${large.length}) iterations=${ITERATIONS} (interleaved median)` +) +console.log( + `${pad('corpus', 26)} ${pad('ungated', 11)} ${pad('gated', 11)} ${pad('delta', 11)} ${pad('speedup', 9)}` +) + +for (const [label, set] of [ + ['all repo markdown', docs], + ['docs over 3 KB', large], + [`biggest (${biggest.file.split('/').pop()})`, [biggest]] +]) { + if (set.length === 0) { + console.log(`${pad(label, 26)} ${pad('no docs in this corpus — skipped', 44)}`) + continue + } + const { ungated, gated } = measureCorpus(set) + const delta = ungated - gated + console.log( + `${pad(label, 26)} ${pad(`${(ungated * 1000).toFixed(1)} us`, 11)} ${pad(`${(gated * 1000).toFixed(1)} us`, 11)} ${pad(`${(delta * 1000).toFixed(1)} us`, 11)} ${pad(`${(ungated / gated).toFixed(2)}x`, 9)}` + ) +} +console.log( + '\nAuto-conversion pays this once per keystroke; preview decorations pay it once\nper keystroke and once per caret move.' +) diff --git a/src/renderer/src/components/editor/rich-markdown-doc-link-code-context.ts b/src/renderer/src/components/editor/rich-markdown-doc-link-code-context.ts deleted file mode 100644 index 703f43bb4..000000000 --- a/src/renderer/src/components/editor/rich-markdown-doc-link-code-context.ts +++ /dev/null @@ -1,16 +0,0 @@ -type DocLinkTextNodeContext = { - marks: readonly { type: { name: string } }[] -} - -type DocLinkParentContext = { - type: { spec: { code?: boolean } } -} - -// Why: inline/fenced code content is literal markdown; auto-converting it would -// corrupt examples and commands that intentionally contain [[...]] text. -export function isDocLinkLiteralCodeTextNode( - node: DocLinkTextNodeContext, - parent: DocLinkParentContext | null -): boolean { - return parent?.type.spec.code === true || node.marks.some((mark) => mark.type.name === 'code') -} diff --git a/src/renderer/src/components/editor/rich-markdown-doc-link-scan.ts b/src/renderer/src/components/editor/rich-markdown-doc-link-scan.ts new file mode 100644 index 000000000..cf7d03f3f --- /dev/null +++ b/src/renderer/src/components/editor/rich-markdown-doc-link-scan.ts @@ -0,0 +1,32 @@ +export const DOC_LINK_PATTERN = /\[\[([^[\]\r\n]+)\]\]/g +const DOC_LINK_OPEN = '[[' + +type DocLinkTextNodeContext = { + type: { name: string } + text?: string + marks: readonly { type: { name: string } }[] +} + +type DocLinkParentContext = { + type: { spec: { code?: boolean } } +} + +export function isDocLinkLiteralCodeTextNode( + node: Pick, + parent: DocLinkParentContext | null +): boolean { + return parent?.type.spec.code === true || node.marks.some((mark) => mark.type.name === 'code') +} + +// A link match requires the exact ASCII opener, including for non-ASCII targets. +export function canHoldDocLink( + node: DocLinkTextNodeContext, + parent: DocLinkParentContext | null +): node is DocLinkTextNodeContext & { text: string } { + return ( + node.type.name === 'text' && + !!node.text && + node.text.includes(DOC_LINK_OPEN) && + !isDocLinkLiteralCodeTextNode(node, parent) + ) +} diff --git a/src/renderer/src/components/editor/rich-markdown-doc-link.test.ts b/src/renderer/src/components/editor/rich-markdown-doc-link.test.ts index 4b95a1f58..acbdc45b2 100644 --- a/src/renderer/src/components/editor/rich-markdown-doc-link.test.ts +++ b/src/renderer/src/components/editor/rich-markdown-doc-link.test.ts @@ -1,8 +1,10 @@ import { describe, expect, it } from 'vitest' -import { isDocLinkLiteralCodeTextNode } from './rich-markdown-doc-link-code-context' +import { canHoldDocLink, DOC_LINK_PATTERN } from './rich-markdown-doc-link-scan' -function textContext(markNames: string[] = []) { +function textContext(text: string, markNames: string[] = []) { return { + type: { name: 'text' }, + text, marks: markNames.map((name) => ({ type: { name } })) } } @@ -15,16 +17,30 @@ function parentContext(code: boolean) { } } -describe('isDocLinkLiteralCodeTextNode', () => { - it('allows doc link conversion in ordinary prose text', () => { - expect(isDocLinkLiteralCodeTextNode(textContext(), parentContext(false))).toBe(false) +function scan(text: string, markNames: string[] = [], parentCode = false): string[] { + const node = textContext(text, markNames) + if (!canHoldDocLink(node, parentContext(parentCode))) { + return [] + } + return [...node.text.matchAll(DOC_LINK_PATTERN)].map((match) => match[1]) +} + +describe('production doc-link scan predicate', () => { + it('finds links in ordinary prose, including non-ASCII targets', () => { + expect(scan('See [[Guide]] and [[文档/😀]].')).toEqual(['Guide', '文档/😀']) }) - it('skips doc link conversion for inline code marks', () => { - expect(isDocLinkLiteralCodeTextNode(textContext(['code']), parentContext(false))).toBe(true) + it('rejects ordinary prose without the exact opener', () => { + expect(scan('See [Guide], plain prose, and closed brackets]].')).toEqual([]) }) - it('skips doc link conversion for fenced code block text', () => { - expect(isDocLinkLiteralCodeTextNode(textContext(), parentContext(true))).toBe(true) + it('skips inline and fenced code', () => { + expect(scan('`[[inline]]`', ['code'])).toEqual([]) + expect(scan('[[fenced]]', [], true)).toEqual([]) + }) + + it('does not match links across CR or LF boundaries', () => { + expect(scan('[[carriage\rreturn]]')).toEqual([]) + expect(scan('[[line\nfeed]]')).toEqual([]) }) }) diff --git a/src/renderer/src/components/editor/rich-markdown-doc-link.ts b/src/renderer/src/components/editor/rich-markdown-doc-link.ts index a35772e4e..bbc1b26b2 100644 --- a/src/renderer/src/components/editor/rich-markdown-doc-link.ts +++ b/src/renderer/src/components/editor/rich-markdown-doc-link.ts @@ -9,16 +9,12 @@ import { parseMarkdownDocLink, resolveMarkdownDocLink } from './markdown-doc-links' -import { isDocLinkLiteralCodeTextNode } from './rich-markdown-doc-link-code-context' import { isReservedRichMarkdownTransportBody, type RichMarkdownSourceTransport } from './rich-markdown-source-transport' import { renderRichMarkdownDocLinkHtml } from './rich-markdown-doc-link-dom' - -// Why: `.matchAll()` at each call site creates a fresh iterator so the shared -// `/g` regex never leaks `lastIndex` state across nested or concurrent scans. -const DOC_LINK_PATTERN = /\[\[([^[\]\r\n]+)\]\]/g +import { canHoldDocLink, DOC_LINK_PATTERN } from './rich-markdown-doc-link-scan' const docLinkDissolveKey = new PluginKey('docLinkDissolve') const docLinkAutoConvertKey = new PluginKey('docLinkAutoConvert') @@ -50,10 +46,7 @@ function buildPreviewDecorations(state: EditorState, storage: DocLinkStorage): D const index = getDocIndex(storage) const cursor = state.selection.from state.doc.descendants((node, pos, parent) => { - if (node.type.name !== 'text' || !node.text) { - return - } - if (isDocLinkLiteralCodeTextNode(node, parent)) { + if (!canHoldDocLink(node, parent)) { return } for (const match of node.text.matchAll(DOC_LINK_PATTERN)) { @@ -279,10 +272,7 @@ export function createMarkdownDocLink(transport: RichMarkdownSourceTransport) { let modified = false newState.doc.descendants((node, pos, parent) => { - if (node.type.name !== 'text' || !node.text) { - return - } - if (isDocLinkLiteralCodeTextNode(node, parent)) { + if (!canHoldDocLink(node, parent)) { return } diff --git a/src/renderer/src/components/editor/rich-markdown-review-annotations.test.ts b/src/renderer/src/components/editor/rich-markdown-review-annotations.test.ts index 36b2d27ca..95c1fa67b 100644 --- a/src/renderer/src/components/editor/rich-markdown-review-annotations.test.ts +++ b/src/renderer/src/components/editor/rich-markdown-review-annotations.test.ts @@ -1,8 +1,12 @@ import { afterEach, describe, expect, it, vi } from 'vitest' +import type { Editor } from '@tiptap/core' +import type { DiffComment } from '../../../../shared/types' import { countRichMarkdownReviewMarkdownLines, getRichMarkdownAnnotationButtonLeft, - getRichMarkdownAnnotationButtonTop + getRichMarkdownAnnotationButtonTop, + getRichMarkdownAnnotationHighlightRanges, + getRichMarkdownCommentAtPos } from './rich-markdown-review-annotations' afterEach(() => { @@ -47,3 +51,77 @@ describe('getRichMarkdownAnnotationButtonLeft', () => { expect(getRichMarkdownAnnotationButtonLeft(72)).toBe(40) }) }) + +// Why count serializes: resolving a comment's block re-serializes the document, +// so doing it per comment made these O(comments x document). A call count is +// deterministic where a wall-clock threshold would be flaky. +describe('rich markdown annotation block reuse', () => { + function makeEditor(nodeCount: number): { editor: Editor; serializeCalls: () => number } { + let serializeCalls = 0 + const content = Array.from({ length: nodeCount }, (_value, index) => ({ + type: 'paragraph', + content: [{ type: 'text', text: `paragraph ${index}` }] + })) + const doc = { + forEach(callback: (node: unknown, offset: number, index: number) => void): void { + content.forEach((node, index) => callback(node, index * 10, index)) + }, + // The text-range search walks the doc to locate the selected text; these + // fixtures never match, so it only needs to be traversable. + nodesBetween(): void {}, + content: { size: nodeCount * 10 } + } + const editor = { + getJSON: () => ({ content }), + state: { doc }, + markdown: { + serialize: (value: { content?: unknown[] }) => { + serializeCalls += 1 + return (value.content ?? []).map((_node, index) => `line ${index}`).join('\n') + } + } + } as unknown as Editor + return { editor, serializeCalls: () => serializeCalls } + } + + function makeComments(count: number): DiffComment[] { + return Array.from( + { length: count }, + (_value, index) => ({ lineNumber: index + 1, selectedText: 'nothing-matches' }) as DiffComment + ) + } + + // One block build over NODE_COUNT nodes: each node serialized alone, plus each + // adjacent pair. Pinned absolutely so "both arms build twice" can't pass as equal. + const NODE_COUNT = 12 + const ONE_BUILD_SERIALIZE_CALLS = NODE_COUNT + (NODE_COUNT - 1) + + it('serializes the document once regardless of comment count', () => { + const single = makeEditor(NODE_COUNT) + getRichMarkdownAnnotationHighlightRanges(single.editor, makeComments(1), 0) + + const many = makeEditor(NODE_COUNT) + getRichMarkdownAnnotationHighlightRanges(many.editor, makeComments(8), 0) + + expect(single.serializeCalls()).toBe(ONE_BUILD_SERIALIZE_CALLS) + expect(many.serializeCalls()).toBe(ONE_BUILD_SERIALIZE_CALLS) + }) + + it('serializes the document once when locating the comment at a position', () => { + const single = makeEditor(NODE_COUNT) + getRichMarkdownCommentAtPos(single.editor, makeComments(1), 0, 5) + + const many = makeEditor(NODE_COUNT) + getRichMarkdownCommentAtPos(many.editor, makeComments(8), 0, 5) + + expect(single.serializeCalls()).toBe(ONE_BUILD_SERIALIZE_CALLS) + expect(many.serializeCalls()).toBe(ONE_BUILD_SERIALIZE_CALLS) + }) + + it('does no work at all with no comments', () => { + const none = makeEditor(12) + expect(getRichMarkdownAnnotationHighlightRanges(none.editor, [], 0)).toEqual([]) + expect(getRichMarkdownCommentAtPos(none.editor, [], 0, 5)).toBeNull() + expect(none.serializeCalls()).toBe(0) + }) +}) diff --git a/src/renderer/src/components/editor/rich-markdown-review-annotations.ts b/src/renderer/src/components/editor/rich-markdown-review-annotations.ts index a45c3f263..a7ef23074 100644 --- a/src/renderer/src/components/editor/rich-markdown-review-annotations.ts +++ b/src/renderer/src/components/editor/rich-markdown-review-annotations.ts @@ -119,17 +119,29 @@ export function getRichMarkdownAnnotationHighlightRanges( comments: readonly DiffComment[], markdownSourceLineOffset: number ): RichMarkdownAnnotationHighlightRange[] { + if (comments.length === 0) { + return [] + } + // Why once: block resolution re-serializes the doc; per comment it was O(n*doc). + const blocks = buildRichMarkdownCommentBlocks(editor) return comments.flatMap((comment) => - getRichMarkdownAnnotationHighlightRangesForComment(editor, comment, markdownSourceLineOffset) + getRichMarkdownAnnotationHighlightRangesForComment( + editor, + comment, + markdownSourceLineOffset, + blocks + ) ) } export function getRichMarkdownAnnotationHighlightRangesForComment( editor: Editor, comment: DiffComment, - markdownSourceLineOffset: number + markdownSourceLineOffset: number, + // Why optional: callers looping over comments pass one shared build. + prebuiltBlocks?: RichMarkdownCommentBlock[] ): RichMarkdownAnnotationHighlightRange[] { - const blocks = buildRichMarkdownCommentBlocks(editor) + const blocks = prebuiltBlocks ?? buildRichMarkdownCommentBlocks(editor) const selectedText = comment.selectedText?.trim() if (!selectedText) { return [] @@ -158,12 +170,17 @@ export function getRichMarkdownCommentAtPos( markdownSourceLineOffset: number, pos: number ): DiffComment | null { + if (comments.length === 0) { + return null + } + const blocks = buildRichMarkdownCommentBlocks(editor) return ( comments.find((comment) => getRichMarkdownAnnotationHighlightRangesForComment( editor, comment, - markdownSourceLineOffset + markdownSourceLineOffset, + blocks ).some((range) => range.from <= pos && pos <= range.to) ) ?? null )