mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-12 20:15:56 +00:00
* perf(search_text): precompute line offsets for O(log n) line lookup
search_for_pattern resolved each match's line number by rescanning the file from the beginning (TextUtils.get_line_from_index walks a TextStepper from index 0), making coordinate resolution O(n) per match and O(n*m) for m matches. search_text now precomputes line start offsets once per file (O(n)) and resolves each match via binary search (O(log n) per match). The precomputed table mirrors TextStepper line semantics exactly: \n, \r\n and bare \r are line separators, and an index pointing at the \n of a \r\n pair resolves to (next line, 0) — the same rule TextUtils.get_line_col_from_index implements, now pinned by regression tests (including the mixed line-ending edge cases from the insert_text_at_position fix). On a synthetic 12k-line file (494 KB) with 723 matches, match-coordinate resolution drops from ~1764 ms to ~2 ms (~1000x); the one-off precompute costs ~1.1 ms. Small files regress nothing (100 lines, 5 matches: 70 µs -> 11 µs). Results are byte-for-byte identical to the previous implementation. Note: compatible with PR #1899 (interruptible agent regexes): that PR touches the re.compile call site in search_text and the ContentReplacer helpers, this PR touches the post-match line-coordinate resolution; the two changes are complementary and rebase cleanly onto each other. * perf(search_text): add reproducible benchmark script scripts/profile_search_text.py regenerates the synthetic benchmark content with a fixed seed (no real-world code), asserts both resolution paths produce identical results, and prints the before/after timings quoted in the PR description. Default: 12k lines / ~1830 matches; optional CLI args scale the file down for quick runs. * refactor(ls): encapsulate cached text coordinates in TextCoordinates abstraction Address review feedback on the search_text line-lookup PR: the previously added module-level helpers duplicated TextStepper's line-ending logic and exposed raw offsets as a low-level data structure shared between two functions, violating the project's software design principles (each concern in exactly one home; dataclasses instead of tuples for simple data storage). Introduce in solidlsp/ls_utils.py: - LineCol: frozen, kw-only dataclass for a 0-based line/column position - TextCoordinates: encapsulates the cached table of line start offsets, built via TextStepper (step_line + line_start_idx), with a single public method line_col_at_index() resolving an index via binary search, including the CRLF rule (an index pointing at the '\n' of a '\r\n' pair denotes the beginning of the next line, column 0) and InvalidTextLocationError for out-of-range indices search_text now instantiates TextCoordinates once per file and resolves each match's coordinates through the public method; the private helpers (_compute_line_starts, _line_col_at_index) are removed. Equivalence with TextUtils.get_line_col_from_index verified across mixed line endings, empty content and boundary indices (0 mismatches). Tests are behaviour-anchored: the implementation-coupled parametrized test importing the private helpers is replaced by public-contract tests of TextCoordinates (test/solidlsp/test_ls_utils.py) and absolute line assertions through the public search_text API. The benchmark script uses the public API as well. Benchmark (synthetic 12k lines, ~723 matches, best of 5): coordinate resolution ~1764 ms -> ~2 ms; precompute ~1.1 ms; small files unchanged. Semantics are unchanged; results are identical to the previously proposed implementation and to the upstream reference implementation. * fix(test): rename test_ls_utils to test_text_coordinates to avoid basename collision The new test file collided with the pre-existing upstream test/solidlsp/util/test_ls_utils.py: pytest (prepend import mode, no __init__.py in the test dirs) requires unique basenames, and collecting both modules produced an import-file-mismatch error that aborted collection with exit code 2 across every CI matrix job. Rename to test_text_coordinates.py (unique, and matches the class under test). Full-suite collection verified locally (2252 tests, 0 errors).
This commit is contained in:
1 parent
704e8c3d19
commit
fa79a2260f
6 files changed
+329
-4
No files matched your search
@@ -15,6 +15,9 @@ Status of the `main` branch. Changes prior to the next official version change w
|
||||
- Fix: MCP `initialize` now reports Serena's version instead of the installed mcp SDK version (#1889)
|
||||
- Fix: Parallel agents auto-registering projects could overwrite each other's changes to the global
|
||||
project list in `serena_config.yml`
|
||||
- Perf: `search_for_pattern` resolved each match's line number by rescanning the file from the
|
||||
beginning (O(n) per match, O(n*m) total for m matches); coordinates are now resolved via the new
|
||||
`TextCoordinates` abstraction (cached line starts + binary search)
|
||||
- Fix: `TextUtils.insert_text_at_position` returned a wrong position when the inserted text merged
|
||||
with an adjacent character into a single newline sequence (e.g. a `\n` inserted directly after an
|
||||
existing `\r`); the position is now determined from the resulting text
|
||||
|
||||
@@ -0,0 +1,110 @@
|
||||
# SPDX-License-Identifier: GPL-3.0-or-later
|
||||
|
||||
"""Benchmark for search_text line-coordinate resolution (see PR "perf(search_text): precompute line offsets").
|
||||
|
||||
Reproduces the before/after numbers quoted in the PR description:
|
||||
|
||||
- "before" resolves each match's line number via ``TextUtils.get_line_from_index``,
|
||||
which walks a TextStepper from index 0 (O(n) per match, O(n*m) for m matches)
|
||||
- "after" precomputes line start offsets once (O(n)) and resolves each match via
|
||||
binary search (O(log n) per match)
|
||||
|
||||
The content is fully synthetic and generated with a fixed seed: it contains no
|
||||
code from any real project. Both paths must produce identical results; the script
|
||||
asserts that before timing anything.
|
||||
|
||||
Usage: uv run python scripts/profile_search_text.py [num_lines] [num_matches]
|
||||
"""
|
||||
|
||||
import random
|
||||
import re
|
||||
import sys
|
||||
import time
|
||||
|
||||
import solidlsp # noqa: F401 # imported first: solidlsp must resolve before serena.util.text_utils (known circular-import window)
|
||||
from solidlsp.ls_utils import TextCoordinates, TextUtils
|
||||
|
||||
|
||||
def generate_synthetic_content(num_lines: int, match_identifier: str, match_frequency: float = 0.15) -> str:
|
||||
"""
|
||||
Generates synthetic pseudo-code content with a fixed seed.
|
||||
|
||||
:param num_lines: number of lines to generate
|
||||
:param match_identifier: the identifier the benchmark pattern will search for
|
||||
:param match_frequency: fraction of lines that reference the match identifier
|
||||
:return: the synthetic content as a single string
|
||||
"""
|
||||
rng = random.Random(1234) # fixed seed for reproducibility
|
||||
filler_identifiers = ["var_global_contador", "otra_funcion_aleatoria", "gestion_operaciones", "ajuste_niveles"]
|
||||
lines = []
|
||||
for i in range(num_lines):
|
||||
kind = rng.random()
|
||||
if kind < match_frequency:
|
||||
lines.append(f" double resultado_{i} = {match_identifier}({rng.randint(1, 999)});")
|
||||
elif kind < match_frequency + 0.15:
|
||||
lines.append(f" if ({filler_identifiers[rng.randrange(len(filler_identifiers))]} > {rng.randint(0, 100)}) {{")
|
||||
elif kind < match_frequency + 0.25:
|
||||
lines.append(" }")
|
||||
elif kind < match_frequency + 0.40:
|
||||
lines.append(f"// comment line {i} with filler text {rng.randint(1000, 9999)}")
|
||||
else:
|
||||
lines.append(f" int local_variable_{i} = {rng.randint(0, 5000)};")
|
||||
return "\n".join(lines) + "\n"
|
||||
|
||||
|
||||
def resolve_before(content: str, compiled_pattern: re.Pattern) -> list[tuple[int, int]]:
|
||||
"""Resolves line coordinates the way upstream currently does (TextStepper from index 0 per match)."""
|
||||
results = []
|
||||
for match in compiled_pattern.finditer(content):
|
||||
start_pos, end_pos = match.start(), match.end()
|
||||
start_line_num = TextUtils.get_line_from_index(content, start_pos)
|
||||
end_line_num = TextUtils.get_line_from_index(content, end_pos)
|
||||
if end_line_num > start_line_num and TextUtils.get_line_col_from_index(content, end_pos)[1] == 0:
|
||||
end_line_num -= 1
|
||||
results.append((start_line_num, end_line_num))
|
||||
return results
|
||||
|
||||
|
||||
def resolve_after(content: str, compiled_pattern: re.Pattern) -> list[tuple[int, int]]:
|
||||
"""Resolves line coordinates the way this PR proposes (cached line starts + binary search)."""
|
||||
coordinates = TextCoordinates(content)
|
||||
results = []
|
||||
for match in compiled_pattern.finditer(content):
|
||||
start_pos, end_pos = match.start(), match.end()
|
||||
s = coordinates.line_col_at_index(start_pos).line
|
||||
end_loc = coordinates.line_col_at_index(end_pos)
|
||||
e = end_loc.line
|
||||
if e > s and end_loc.col == 0:
|
||||
e -= 1
|
||||
results.append((s, e))
|
||||
return results
|
||||
|
||||
|
||||
def main() -> None:
|
||||
num_lines = int(sys.argv[1]) if len(sys.argv) > 1 else 12000
|
||||
match_identifier = "funcion_calcula_parametro"
|
||||
content = generate_synthetic_content(num_lines, match_identifier)
|
||||
compiled_pattern = re.compile(match_identifier)
|
||||
|
||||
num_matches = len(list(compiled_pattern.finditer(content)))
|
||||
print(f"synthetic file: {content.count(chr(10)):,} lines, {len(content):,} chars, {num_matches} matches")
|
||||
|
||||
before = resolve_before(content, compiled_pattern)
|
||||
after = resolve_after(content, compiled_pattern)
|
||||
assert before == after, "implementations disagree; this script only measures identical work"
|
||||
print(f"results identical: OK ({len(before)} matches)")
|
||||
|
||||
for name, fn in [
|
||||
("before (TextStepper, O(n) per match)", lambda: resolve_before(content, compiled_pattern)),
|
||||
("after (bisect, O(log n) per match)", lambda: resolve_after(content, compiled_pattern)),
|
||||
]:
|
||||
times = []
|
||||
for _ in range(5):
|
||||
t0 = time.perf_counter()
|
||||
fn()
|
||||
times.append(time.perf_counter() - t0)
|
||||
print(f"{name}: {min(times) * 1000:.2f} ms (best of 5)")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
@@ -13,7 +13,7 @@ from joblib import Parallel, delayed
|
||||
from sensai.util.string import ToStringMixin
|
||||
|
||||
from serena.util.file_proxy import FileCollection, FileProxy
|
||||
from solidlsp.ls_utils import TextUtils
|
||||
from solidlsp.ls_utils import TextCoordinates, TextUtils
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
|
||||
@@ -150,6 +150,10 @@ def search_text(
|
||||
lines = TextUtils.split_lines(content)
|
||||
total_lines = len(lines)
|
||||
|
||||
# precompute line start offsets once so that each match's coordinates can be resolved via binary search
|
||||
# instead of re-scanning the text from the beginning for every match
|
||||
coordinates = TextCoordinates(content)
|
||||
|
||||
# For multiline matches, optionally use DOTALL so '.' matches newlines
|
||||
flags = (re.MULTILINE | re.DOTALL) if multiline else 0
|
||||
compiled_pattern = re.compile(pattern, flags)
|
||||
@@ -159,9 +163,10 @@ def search_text(
|
||||
end_pos = match.end()
|
||||
|
||||
# Find the line numbers for the start and end positions
|
||||
start_line_num = TextUtils.get_line_from_index(content, start_pos)
|
||||
end_line_num = TextUtils.get_line_from_index(content, end_pos)
|
||||
if end_line_num > start_line_num and TextUtils.get_line_col_from_index(content, end_pos)[1] == 0:
|
||||
start_loc = coordinates.line_col_at_index(start_pos)
|
||||
end_loc = coordinates.line_col_at_index(end_pos)
|
||||
start_line_num, end_line_num = start_loc.line, end_loc.line
|
||||
if end_line_num > start_line_num and end_loc.col == 0:
|
||||
# `end_pos` is exclusive, so if it is at the start of a line, the match ends with the
|
||||
# preceding line's newline and does not extend into the line that `end_pos` points to
|
||||
end_line_num -= 1
|
||||
|
||||
@@ -3,6 +3,7 @@ This file contains various utility functions like I/O operations, handling paths
|
||||
"""
|
||||
# SPDX-License-Identifier: MIT
|
||||
|
||||
import bisect
|
||||
import gzip
|
||||
import hashlib
|
||||
import logging
|
||||
@@ -15,6 +16,7 @@ import tempfile
|
||||
import uuid
|
||||
import zipfile
|
||||
from collections.abc import Sequence
|
||||
from dataclasses import dataclass
|
||||
from enum import Enum
|
||||
from pathlib import Path, PurePath
|
||||
from typing import Literal, cast
|
||||
@@ -179,6 +181,75 @@ class TextStepper:
|
||||
return lines
|
||||
|
||||
|
||||
@dataclass(frozen=True, kw_only=True)
|
||||
class LineCol:
|
||||
"""
|
||||
Represents a position in a text as a pair of 0-based line and column numbers.
|
||||
"""
|
||||
|
||||
line: int
|
||||
"""the 0-based line number"""
|
||||
|
||||
col: int
|
||||
"""the 0-based column number"""
|
||||
|
||||
|
||||
class TextCoordinates:
|
||||
r"""
|
||||
Provides line/column coordinates for character indices in a text, backed by a privately cached table of line start
|
||||
offsets.
|
||||
|
||||
The table is built once upon construction and mirrors the line semantics of :class:`TextStepper`: "\n", "\r\n"
|
||||
and a bare "\r" are all treated as line separators (as defined by the Language Server Protocol). Resolving a
|
||||
coordinate then requires only a binary search, independent of the text length.
|
||||
"""
|
||||
|
||||
def __init__(self, text: str):
|
||||
"""
|
||||
:param text: the text in which character indices are to be located
|
||||
"""
|
||||
self._text = text
|
||||
self._line_starts = self._compute_line_starts()
|
||||
|
||||
def line_col_at_index(self, index: int) -> LineCol:
|
||||
r"""
|
||||
Returns the line/column coordinates corresponding to the given character index.
|
||||
|
||||
An index pointing at the "\n" of a "\r\n" sequence denotes the beginning of the following line
|
||||
(column 0), in the same way as a cursor insertion position between "\r" and "\n" does.
|
||||
|
||||
:param index: the 0-based index in the text; must not exceed the text length
|
||||
:return: the coordinates corresponding to the index
|
||||
:raises InvalidTextLocationError: if the index is negative or greater than the text length
|
||||
"""
|
||||
# determine the line containing the index, which is the last line whose start does not exceed the index
|
||||
if index < 0 or index > len(self._text):
|
||||
raise InvalidTextLocationError(f"{index=}")
|
||||
line_num = bisect.bisect_right(self._line_starts, index) - 1
|
||||
line_start = self._line_starts[line_num]
|
||||
|
||||
# an index pointing at the "\n" of a "\r\n" pair maps to the beginning of the following line
|
||||
if index > 0 and self._text[index - 1] == "\r" and self._text[index : index + 1] == "\n":
|
||||
return LineCol(line=line_num + 1, col=0)
|
||||
|
||||
return LineCol(line=line_num, col=index - line_start)
|
||||
|
||||
def _compute_line_starts(self) -> list[int]:
|
||||
"""
|
||||
Computes the character offsets at which the lines of the text begin, using a TextStepper to process
|
||||
the text line by line.
|
||||
|
||||
:return: a list where entry i is the 0-based character offset at which line i starts;
|
||||
entry 0 is always 0
|
||||
"""
|
||||
line_starts = [0]
|
||||
text_stepper = TextStepper(self._text)
|
||||
while text_stepper.step_line():
|
||||
if text_stepper.is_newline:
|
||||
line_starts.append(text_stepper.line_start_idx)
|
||||
return line_starts
|
||||
|
||||
|
||||
class TextUtils:
|
||||
"""
|
||||
Utilities for text operations.
|
||||
|
||||
@@ -190,6 +190,41 @@ class TestSearchText:
|
||||
|
||||
assert len(matches) == 0
|
||||
|
||||
def test_search_text_crlf_line_numbers(self):
|
||||
"""Matches in CRLF content report the same line numbers as with LF endings."""
|
||||
crlf = "alpha\r\nbeta\r\ngamma\r\n"
|
||||
lf = "alpha\nbeta\ngamma\n"
|
||||
crlf_matches = search_text("beta", content=crlf)
|
||||
lf_matches = search_text("beta", content=lf)
|
||||
assert len(crlf_matches) == 1
|
||||
assert len(lf_matches) == 1
|
||||
assert crlf_matches[0].start_line == lf_matches[0].start_line == 1
|
||||
assert crlf_matches[0].end_line == lf_matches[0].end_line == 1
|
||||
|
||||
def test_search_text_bare_cr_line_numbers(self):
|
||||
r"""Bare \r line endings produce separate lines, matching TextStepper semantics."""
|
||||
content = "alpha\rbeta\r"
|
||||
matches = search_text("beta", content=content)
|
||||
assert len(matches) == 1
|
||||
assert matches[0].start_line == 1
|
||||
assert matches[0].end_line == 1
|
||||
|
||||
def test_search_text_match_at_boundaries(self):
|
||||
"""Matches at the very start and very end of the content resolve to sane line numbers."""
|
||||
content = "first\nmiddle\nlast"
|
||||
first = search_text("first", content=content)
|
||||
assert first[0].start_line == 0
|
||||
last = search_text("last", content=content)
|
||||
assert last[0].start_line == 2
|
||||
|
||||
def test_search_text_multiline_match_line_range(self):
|
||||
"""A multiline match spanning several lines reports the full matched range."""
|
||||
content = "a\nTARGET_START\nb\nc\nTARGET_END\nd\n"
|
||||
matches = search_text("TARGET_START[\\s\\S]*?TARGET_END", content=content)
|
||||
assert len(matches) == 1
|
||||
assert matches[0].start_line == 1
|
||||
assert matches[0].end_line == 4
|
||||
|
||||
|
||||
# Mock file reader that always returns matching content
|
||||
def mock_reader_always_match(file_path: str) -> str:
|
||||
|
||||
@@ -0,0 +1,101 @@
|
||||
import pytest
|
||||
|
||||
from solidlsp.ls_utils import InvalidTextLocationError, LineCol, TextCoordinates
|
||||
|
||||
|
||||
class TestTextCoordinates:
|
||||
"""Tests for the public contract of TextCoordinates and LineCol."""
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"content",
|
||||
[
|
||||
"hello\nworld\n",
|
||||
"hello\r\nworld\r\n",
|
||||
"hello\rworld\r",
|
||||
"a\rb\nc\r\nd\r\n",
|
||||
"",
|
||||
"no newline at end",
|
||||
"\n",
|
||||
"\r",
|
||||
"a\r\n\r\nb",
|
||||
],
|
||||
)
|
||||
def test_line_col_at_index_end_to_end(self, content):
|
||||
"""Every index within the text resolves to a position that is consistent with the line layout."""
|
||||
coordinates = TextCoordinates(content)
|
||||
for index in range(len(content) + 1):
|
||||
loc = coordinates.line_col_at_index(index)
|
||||
assert loc.line >= 0
|
||||
assert loc.col >= 0
|
||||
# a col greater than 0 implies that none of the col characters before the index is a line separator
|
||||
if loc.col > 0:
|
||||
preceding = content[index - loc.col : index]
|
||||
assert "\n" not in preceding and "\r" not in preceding
|
||||
|
||||
def test_line_col_at_index_basic_lf(self):
|
||||
"""Coordinates for LF-only content match the manually derived line layout."""
|
||||
coordinates = TextCoordinates("alpha\nbeta\ngamma\n")
|
||||
assert coordinates.line_col_at_index(0) == LineCol(line=0, col=0)
|
||||
assert coordinates.line_col_at_index(3) == LineCol(line=0, col=3)
|
||||
assert coordinates.line_col_at_index(5) == LineCol(line=0, col=5)
|
||||
assert coordinates.line_col_at_index(6) == LineCol(line=1, col=0)
|
||||
assert coordinates.line_col_at_index(8) == LineCol(line=1, col=2)
|
||||
assert coordinates.line_col_at_index(11) == LineCol(line=2, col=0)
|
||||
assert coordinates.line_col_at_index(12) == LineCol(line=2, col=1)
|
||||
assert coordinates.line_col_at_index(16) == LineCol(line=2, col=5)
|
||||
# an index at the end of the text (after the trailing newline) denotes the start of a new line
|
||||
assert coordinates.line_col_at_index(17) == LineCol(line=3, col=0)
|
||||
|
||||
def test_line_col_at_index_crlf(self):
|
||||
r"""CRLF sequences count as a single line ending. An index pointing at the "\n" of a pair denotes
|
||||
the beginning of the following line (column 0), whereas the "\r" itself still belongs to the
|
||||
preceding line.
|
||||
"""
|
||||
coordinates = TextCoordinates("alpha\r\nbeta\r\n")
|
||||
assert coordinates.line_col_at_index(0) == LineCol(line=0, col=0)
|
||||
assert coordinates.line_col_at_index(5) == LineCol(line=0, col=5)
|
||||
assert coordinates.line_col_at_index(6) == LineCol(line=1, col=0)
|
||||
assert coordinates.line_col_at_index(7) == LineCol(line=1, col=0)
|
||||
assert coordinates.line_col_at_index(8) == LineCol(line=1, col=1)
|
||||
assert coordinates.line_col_at_index(11) == LineCol(line=1, col=4)
|
||||
assert coordinates.line_col_at_index(12) == LineCol(line=2, col=0)
|
||||
assert coordinates.line_col_at_index(13) == LineCol(line=2, col=0)
|
||||
|
||||
def test_line_col_at_index_bare_cr(self):
|
||||
r"""Bare "\r" characters act as line separators."""
|
||||
coordinates = TextCoordinates("alpha\rbeta\r")
|
||||
assert coordinates.line_col_at_index(5) == LineCol(line=0, col=5)
|
||||
assert coordinates.line_col_at_index(6) == LineCol(line=1, col=0)
|
||||
assert coordinates.line_col_at_index(10) == LineCol(line=1, col=4)
|
||||
assert coordinates.line_col_at_index(11) == LineCol(line=2, col=0)
|
||||
|
||||
def test_line_col_at_index_mixed(self):
|
||||
"""Mixed line endings resolve consistently in a single text."""
|
||||
coordinates = TextCoordinates("a\r\nb\rc\nd\r\n")
|
||||
# "\r\n" ends line 0; "\r" ends line 1; "\n" ends line 2; "\r\n" ends line 3
|
||||
assert coordinates.line_col_at_index(0) == LineCol(line=0, col=0)
|
||||
assert coordinates.line_col_at_index(1) == LineCol(line=0, col=1)
|
||||
assert coordinates.line_col_at_index(2) == LineCol(line=1, col=0)
|
||||
assert coordinates.line_col_at_index(3) == LineCol(line=1, col=0)
|
||||
assert coordinates.line_col_at_index(4) == LineCol(line=1, col=1)
|
||||
assert coordinates.line_col_at_index(5) == LineCol(line=2, col=0)
|
||||
assert coordinates.line_col_at_index(6) == LineCol(line=2, col=1)
|
||||
assert coordinates.line_col_at_index(7) == LineCol(line=3, col=0)
|
||||
assert coordinates.line_col_at_index(8) == LineCol(line=3, col=1)
|
||||
assert coordinates.line_col_at_index(9) == LineCol(line=4, col=0)
|
||||
assert coordinates.line_col_at_index(10) == LineCol(line=4, col=0)
|
||||
|
||||
def test_line_col_at_index_empty_content(self):
|
||||
"""The only valid index in empty content resolves to the origin."""
|
||||
coordinates = TextCoordinates("")
|
||||
assert coordinates.line_col_at_index(0) == LineCol(line=0, col=0)
|
||||
with pytest.raises(InvalidTextLocationError):
|
||||
coordinates.line_col_at_index(1)
|
||||
|
||||
def test_line_col_at_index_out_of_range(self):
|
||||
"""Indices beyond the text length are rejected."""
|
||||
coordinates = TextCoordinates("abc")
|
||||
with pytest.raises(InvalidTextLocationError):
|
||||
coordinates.line_col_at_index(4)
|
||||
with pytest.raises(InvalidTextLocationError):
|
||||
coordinates.line_col_at_index(-1)
|
||||
Reference in new issue
Block a user