perf(terminal): bound the PR-link carry scan to the trailing window (#10741)

config/scripts/terminal-pr-link-carry-benchmark.mjs

@@ -0,0 +1,221 @@
+#!/usr/bin/env node
+// Benchmark: per-chunk cost of the GitHub PR-link carry scan on the PTY output path.
+//
+// createTerminalGitHubPRLinkDetector() runs on every PTY chunk (renderer
+// pty-connection + parked-terminal-byte-watcher). Before the fix,
+// getPotentialGitHubPRCarry() ran `lastIndexOf` for BOTH http scheme prefixes
+// across the entire combined chunk — even on the early-out path where the chunk
+// provably has no `/pull/`. The carry it returns is always a suffix of at most
+// MAX_CARRY_LENGTH (512) bytes, so every byte scanned before
+// `length - 512` was guaranteed-wasted work.
+//
+// The fix bounds the scan to that trailing window. This script measures the
+// scan itself across chunk sizes so the saved work is quantified.
+//
+// carryBefore/carryAfter are mirrors: node cannot import the .ts source, which is
+// why the sibling benchmarks in this directory inline their subject too. The
+// constants below are re-read from the real module at startup so a drifted cap or
+// scheme list fails loudly here instead of quietly benchmarking dead code.
+import { readFileSync } from 'node:fs'
+import { performance } from 'node:perf_hooks'
+import { fileURLToPath } from 'node:url'
+
+const DETECTOR_SOURCE = readFileSync(
+  fileURLToPath(new URL('../../src/shared/terminal-github-pr-link-detector.ts', import.meta.url)),
+  'utf8'
+)
+
+function readMirroredConstants(source) {
+  const cap = source.match(/const MAX_CARRY_LENGTH = (\d+)/)
+  const prefixes = source.match(/const HTTP_SCHEME_PREFIXES = \[([^\]]+)\]/)
+  if (!cap || !prefixes) {
+    throw new Error(
+      'terminal-github-pr-link-detector.ts no longer exposes MAX_CARRY_LENGTH / HTTP_SCHEME_PREFIXES in the expected shape; re-sync this benchmark with the implementation.'
+    )
+  }
+  return {
+    maxCarryLength: Number(cap[1]),
+    httpSchemePrefixes: prefixes[1]
+      .split(',')
+      .map((entry) => entry.trim().replace(/^['"]|['"]$/g, ''))
+      .filter(Boolean)
+  }
+}
+
+const { maxCarryLength: MAX_CARRY_LENGTH, httpSchemePrefixes: HTTP_SCHEME_PREFIXES } =
+  readMirroredConstants(DETECTOR_SOURCE)
+const ITERATIONS = Number.parseInt(process.env.ORCA_PR_CARRY_BENCH_ITERATIONS ?? '2000', 10)
+const WARMUP = Number.parseInt(process.env.ORCA_PR_CARRY_BENCH_WARMUP ?? '200', 10)
+
+for (const [name, value] of [
+  ['ORCA_PR_CARRY_BENCH_ITERATIONS', ITERATIONS],
+  ['ORCA_PR_CARRY_BENCH_WARMUP', WARMUP]
+]) {
+  if (!Number.isInteger(value) || value <= 0) {
+    throw new Error(`${name} must be a positive integer, received ${value}`)
+  }
+}
+
+function hasTerminalUrlWhitespace(value, start, end) {
+  for (let index = start; index < end; index += 1) {
+    if (/\s/.test(value.charAt(index))) {
+      return true
+    }
+  }
+  return false
+}
+
+function endsWithHttpSchemePrefixFragment(value) {
+  for (const prefix of HTTP_SCHEME_PREFIXES) {
+    for (let length = Math.min(prefix.length - 1, value.length); length > 0; length--) {
+      if (value.endsWith(prefix.slice(0, length))) {
+        return value.slice(value.length - length)
+      }
+    }
+  }
+  return ''
+}
+
+// Pre-fix implementation, kept verbatim for comparison.
+function carryBefore(value) {
+  const schemeIndex = Math.max(...HTTP_SCHEME_PREFIXES.map((prefix) => value.lastIndexOf(prefix)))
+  if (schemeIndex !== -1) {
+    const tailLength = value.length - schemeIndex
+    if (tailLength > MAX_CARRY_LENGTH) {
+      return ''
+    }
+    return hasTerminalUrlWhitespace(value, schemeIndex, value.length)
+      ? ''
+      : value.slice(schemeIndex)
+  }
+  return endsWithHttpSchemePrefixFragment(value)
+}
+
+// Post-fix implementation, mirroring src/shared/terminal-github-pr-link-detector.ts.
+function lastIndexOfHttpScheme(value, fromIndex) {
+  let lastIndex = -1
+  for (const prefix of HTTP_SCHEME_PREFIXES) {
+    const candidate =
+      fromIndex === undefined ? value.lastIndexOf(prefix) : value.lastIndexOf(prefix, fromIndex)
+    if (candidate > lastIndex) {
+      lastIndex = candidate
+    }
+  }
+  return lastIndex
+}
+
+function carryAfter(value) {
+  const windowStart = value.length > MAX_CARRY_LENGTH ? value.length - MAX_CARRY_LENGTH : 0
+  const window = windowStart === 0 ? value : value.slice(windowStart)
+  const schemeIndexInWindow = lastIndexOfHttpScheme(window)
+  if (schemeIndexInWindow !== -1) {
+    const schemeIndex = windowStart + schemeIndexInWindow
+    return hasTerminalUrlWhitespace(value, schemeIndex, value.length)
+      ? ''
+      : value.slice(schemeIndex)
+  }
+  const fragment = endsWithHttpSchemePrefixFragment(window)
+  if (fragment === '' || windowStart === 0) {
+    return fragment
+  }
+  return lastIndexOfHttpScheme(value, windowStart - 1) === -1 ? fragment : ''
+}
+
+const GITHUB_PR_PATH_MARKER = '/pull/'
+
+// Agent TUI output: no scheme anywhere, which is the overwhelmingly common case
+// and the one where the old code scanned the full chunk to return ''. `tail`
+// forces the chunk to end mid-scheme so the fallback branch is measured too.
+function makeChunk(bytes, tail = '') {
+  const line = 'build output line with some text and punctuation, id=12345\n'
+  const filled = line.repeat(Math.ceil(bytes / line.length)).slice(0, bytes)
+  return tail ? filled.slice(0, bytes - tail.length) + tail : filled
+}
+
+// Why measure this too: the detector runs includes() over the whole chunk before
+// the carry scan and the fix does not touch that cost, so timing the carry alone
+// reports a win the hot path cannot actually realize. These fixtures never hold
+// the marker, so this mirrors the early-out branch ordinary output takes.
+function detectorEarlyOut(carry, value) {
+  if (value.includes(GITHUB_PR_PATH_MARKER)) {
+    throw new Error('benchmark fixture unexpectedly contains the PR marker')
+  }
+  return carry(value)
+}
+
+function measure(fn, chunk) {
+  for (let index = 0; index < WARMUP; index += 1) {
+    fn(chunk)
+  }
+  const samples = []
+  for (let round = 0; round < 5; round += 1) {
+    const start = performance.now()
+    for (let index = 0; index < ITERATIONS; index += 1) {
+      fn(chunk)
+    }
+    samples.push((performance.now() - start) / ITERATIONS)
+  }
+  samples.sort((a, b) => a - b)
+  return samples[2]
+}
+
+// Why non-empty fixtures: a chunk of ordinary text yields '' from both versions,
+// so an equality check over it would pass even for a carry that always returns ''.
+const EQUIVALENCE_FIXTURES = [
+  `noise ${'x'.repeat(400)}https://github.com/acme/orca/pull/7`,
+  `https://github.com/acme/orca/pull/1${'x'.repeat(600)}`,
+  `https://github.com/acme/orca/pull/1${'x'.repeat(600)}https`,
+  `${'x'.repeat(1000)}https`,
+  `${'x'.repeat(1000)}http`,
+  'https://github.com/acme/orca/pull/7 trailing words',
+  `${'y'.repeat(600)}`,
+  '',
+  'https://github.com/acme/orca/pull/7'
+]
+for (const fixture of EQUIVALENCE_FIXTURES) {
+  if (carryBefore(fixture) !== carryAfter(fixture)) {
+    throw new Error(
+      `carry mismatch on fixture (len ${fixture.length}): ${JSON.stringify(carryBefore(fixture))} vs ${JSON.stringify(carryAfter(fixture))}`
+    )
+  }
+}
+
+const SIZES = [4 * 1024, 16 * 1024, 64 * 1024, 256 * 1024, 1024 * 1024]
+const rows = []
+for (const bytes of SIZES) {
+  const chunk = makeChunk(bytes)
+  // 'with' ends in 'h', so the chunk terminates on a partial scheme fragment and
+  // the new code pays the extra bounded probe behind the window.
+  const fragmentChunk = makeChunk(bytes, 'with')
+  for (const sample of [chunk, fragmentChunk]) {
+    if (carryBefore(sample) !== carryAfter(sample)) {
+      throw new Error(`carry mismatch at ${bytes} bytes`)
+    }
+  }
+  rows.push({
+    chunk: `${(bytes / 1024).toFixed(0)} KiB`,
+    carry: measure(carryBefore, chunk) / measure(carryAfter, chunk),
+    path:
+      measure((value) => detectorEarlyOut(carryBefore, value), chunk) /
+      measure((value) => detectorEarlyOut(carryAfter, value), chunk),
+    fragment: measure(carryBefore, fragmentChunk) / measure(carryAfter, fragmentChunk)
+  })
+}
+
+const pad = (value, width) => String(value).padStart(width)
+console.log('PR-link carry scan, per PTY chunk. Speedup = before / after (>1 is faster).')
+console.log(`iterations=${ITERATIONS} warmup=${WARMUP} (median of 5 rounds)`)
+console.log(
+  `${pad('chunk', 9)} ${pad('carry only', 12)} ${pad('detector path', 15)} ${pad('fragment tail', 15)}`
+)
+for (const row of rows) {
+  console.log(
+    `${pad(row.chunk, 9)} ${pad(`${row.carry.toFixed(1)}x`, 12)} ${pad(`${row.path.toFixed(1)}x`, 15)} ${pad(`${row.fragment.toFixed(2)}x`, 15)}`
+  )
+}
+console.log(
+  '\ncarry only    = the scan this change bounds, in isolation.\n' +
+    'detector path = includes() + carry, i.e. what the PTY hot path actually saves.\n' +
+    'fragment tail = chunk ending mid-scheme, where the new code pays an extra\n' +
+    '                bounded probe. ~1x means the fallback costs nothing material.'
+)

src/shared/terminal-github-pr-link-detector.test.ts

@@ -184,4 +184,111 @@ describe('createTerminalGitHubPRLinkDetector', () => {
     expect(observe(`https://github.com/acme/orca/pull/${'4'.repeat(10_000)}`)).toEqual([])
     expect(observe('2\r\n')).toEqual([])
   })
+
+  // Why: the carry scan only looks at the trailing MAX_CARRY_LENGTH (512) bytes,
+  // so these pin both sides of that cap and the drop arm behind it.
+  it('still joins a PR URL split across chunks after a large scheme-free chunk', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    expect(observe(`${'filler output\n'.repeat(5_000)}https://github.com/acme/orca/pull/`)).toEqual(
+      []
+    )
+    expect(observe('7\r\n')).toEqual([
+      {
+        url: 'https://github.com/acme/orca/pull/7',
+        slug: { owner: 'acme', repo: 'orca', host: 'github.com' },
+        number: 7
+      }
+    ])
+  })
+
+  it('drops carry when the scheme sits further back than the carry cap', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    // The URL opened >512 bytes before the chunk end, so it already overran the
+    // cap and must not resurrect on the next chunk.
+    expect(observe(`https://github.com/acme/orca/pull/${'4'.repeat(600)}`)).toEqual([])
+    expect(observe('2\r\n')).toEqual([])
+  })
+
+  it('keeps carry when the scheme-to-end tail is exactly at the cap', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    // Scheme-to-end tail is exactly MAX_CARRY_LENGTH, so the carry survives and
+    // the URL joins on the next chunk. Shrinking the window by one byte drops it.
+    // Padding goes in the repo segment so the trailing PR number stays finite.
+    const stem = `https://github.com/acme/${'r'.repeat(481)}/pull/7`
+    expect(stem).toHaveLength(512)
+    expect(observe(`noise\n${stem}`)).toEqual([])
+    expect(observe('\n')).toEqual([
+      {
+        url: stem,
+        slug: { owner: 'acme', repo: 'r'.repeat(481), host: 'github.com' },
+        number: 7
+      }
+    ])
+  })
+
+  it('drops carry one byte past the cap', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    // One byte longer than the cap, so the carry is abandoned and nothing joins.
+    const stem = `https://github.com/acme/${'r'.repeat(482)}/pull/7`
+    expect(stem).toHaveLength(513)
+    expect(observe(`noise\n${stem}`)).toEqual([])
+    expect(observe('\n')).toEqual([])
+  })
+
+  it('drops a trailing scheme fragment when a scheme sits behind the window', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    // The earlier scheme already overran the cap, so the trailing 'https'
+    // fragment must not restart a carry and revive the abandoned URL.
+    expect(observe(`https://github.com/acme/orca/pull/1${'x'.repeat(600)}https`)).toEqual([])
+    expect(observe('://github.com/acme/orca/pull/12\n')).toEqual([])
+  })
+
+  it('does not fabricate a link by splicing a stale fragment onto later output', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    // Keeping the 'h' would splice it onto the next chunk and emit a PR link for
+    // a repo that never appeared in the stream.
+    expect(observe(`https://github.com/acme/orca/pull/1${'x'.repeat(600)}h`)).toEqual([])
+    expect(observe('ttps://github.com/zz/yy/pull/9\n')).toEqual([])
+  })
+
+  // Why both schemes: the carry scan checks every entry of HTTP_SCHEME_PREFIXES,
+  // so http:// needs its own carry coverage or half the loop goes unpinned.
+  it('joins a plain http:// PR URL split across chunks', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    expect(observe('http://github.internal/MyOrg/my_repo/pull/39')).toEqual([])
+    expect(observe('5\r\n')).toEqual([
+      {
+        url: 'http://github.internal/MyOrg/my_repo/pull/395',
+        slug: { owner: 'MyOrg', repo: 'my_repo', host: 'github.internal' },
+        number: 395
+      }
+    ])
+  })
+
+  it('drops a trailing fragment when a plain http:// scheme sits behind the window', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    expect(observe(`http://github.internal/o/r/pull/1${'x'.repeat(600)}https`)).toEqual([])
+    expect(observe('://github.com/a/b/pull/12\n')).toEqual([])
+  })
+
+  it('keeps a trailing scheme fragment when no scheme sits behind the window', () => {
+    const observe = createTerminalGitHubPRLinkDetector()
+
+    expect(observe(`${'x'.repeat(1_000)}https`)).toEqual([])
+    expect(observe('://github.com/acme/orca/pull/12\n')).toEqual([
+      {
+        url: 'https://github.com/acme/orca/pull/12',
+        slug: { owner: 'acme', repo: 'orca', host: 'github.com' },
+        number: 12
+      }
+    ])
+  })
 })

src/shared/terminal-github-pr-link-detector.ts

@@ -53,19 +53,38 @@ function endsWithHttpSchemePrefixFragment(value: string): string {
   return ''
 }
 
-function getPotentialGitHubPRCarry(value: string): string {
-  const schemeIndex = Math.max(...HTTP_SCHEME_PREFIXES.map((prefix) => value.lastIndexOf(prefix)))
-  if (schemeIndex !== -1) {
-    const tailLength = value.length - schemeIndex
-    if (tailLength > MAX_CARRY_LENGTH) {
-      return ''
+function lastIndexOfHttpScheme(value: string, fromIndex?: number): number {
+  let lastIndex = -1
+  for (const prefix of HTTP_SCHEME_PREFIXES) {
+    const candidate =
+      fromIndex === undefined ? value.lastIndexOf(prefix) : value.lastIndexOf(prefix, fromIndex)
+    if (candidate > lastIndex) {
+      lastIndex = candidate
     }
+  }
+  return lastIndex
+}
+
+function getPotentialGitHubPRCarry(value: string): string {
+  // Why bounded: carry is always a suffix of at most MAX_CARRY_LENGTH, so a scheme
+  // further back can only ever be dropped — scanning to it is O(chunk) per PTY write.
+  const windowStart = value.length > MAX_CARRY_LENGTH ? value.length - MAX_CARRY_LENGTH : 0
+  const tailWindow = windowStart === 0 ? value : value.slice(windowStart)
+  const schemeIndexInWindow = lastIndexOfHttpScheme(tailWindow)
+  if (schemeIndexInWindow !== -1) {
+    const schemeIndex = windowStart + schemeIndexInWindow
     return hasTerminalUrlWhitespace(value, schemeIndex, value.length)
       ? ''
       : value.slice(schemeIndex)
   }
 
-  return endsWithHttpSchemePrefixFragment(value)
+  const fragment = endsWithHttpSchemePrefixFragment(tailWindow)
+  if (fragment === '' || windowStart === 0) {
+    return fragment
+  }
+  // Why look behind: an older scheme means the URL already overran the cap, so the
+  // carry is abandoned rather than restarted from this fragment.
+  return lastIndexOfHttpScheme(value, windowStart - 1) === -1 ? fragment : ''
 }
 
 function hasTerminalUrlWhitespace(value: string, start: number, end: number): boolean {