ptx: performance improvement - avoid per-word filename clones and lin…#13478
Closed
Aisha630 wants to merge 1 commit into
Closed
ptx: performance improvement - avoid per-word filename clones and lin…#13478Aisha630 wants to merge 1 commit into
Aisha630 wants to merge 1 commit into
Conversation
…e rescans Store a `file_idx` into `FileMap` on each `WordRef` instead of cloning the filename per word, and index the map directly instead of scanning it by name and line range on every output line. Also cache the keyword's character offset (`char_position`) computed during `create_word_set`, so `prepare_line_chunks` no longer rescans the line prefix on each call.
Contributor
|
note: long comments (probably written by llm) aren't helpful what we care about it benchmarks in tree (using divan) that demonstrate the improvement of your work |
sylvestre
reviewed
Jul 20, 2026
Comment on lines
+195
to
+204
| // Character offset of `position` within the line. Fully derived from | ||
| // `(line, position)`, which precede it in the derived `Ord`/`Eq`, so it can | ||
| // never change the comparison result: whenever every preceding field ties, | ||
| // this one ties too. Cached here purely to avoid rescanning the line prefix | ||
| // on every output call (see `prepare_line_chunks`). | ||
| char_position: usize, | ||
| // Index into `FileMap` of the file this word came from. Stored instead of a | ||
| // cloned `OsString` to avoid duplicating the filename once per word; the name | ||
| // is looked up by index when needed. Acts as a deterministic `Ord` tiebreaker, | ||
| // though `global_line_nr` already uniquely identifies the line beforehand. |
Contributor
There was a problem hiding this comment.
why such long comments ?
sylvestre
reviewed
Jul 20, 2026
| Some(x) => (x.start(), x.end()), | ||
| None => (0, 0), | ||
| }; | ||
| // Running cursor for byte -> character offset conversion. `find_iter` |
Contributor
There was a problem hiding this comment.
same, long comments aren't super helpful
Author
noted. will make a new PR addressing these |
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ptx: fix super-linear slowdown on long lines, and cut peak memory usage
Summary
While evaluating
uutilscoreutils as part of a research project, I ran a fuzzing campaign and noticed thatptxwould appear to hang on large inputs. By "hang," I mean it kept running for several minutes and, on bigger files, effectively never finished, even within an hour. After reproducing it and reading the source, I found it was not an infinite loop but an algorithmic problem.ptxwas repeatedly rescanning the same line prefixes.This PR fixes that. On a pathological 2 MB single-line input,
ptxgoes from ~10 s to ~0.34 s (~29× faster), and it also uses ~19% less peak memory than the original.How I found it
My corpus was valid UTF-8 with very few or no newlines. That is the worst case for
ptx. With no newlines, the whole file is one giant "line."Doubling the line length doubled the runtime while the file stayed 2 MB, the signature of
Θ(n·L)work, which degenerates toΘ(n²)for a single line.Root cause
Word positions are found as byte offsets, but output indexes into a
Vec<char>, soptxconverts byte → character offsets inprepare_line_chunks:line[..word_ref.position].chars().count()walks the line from the start every time. Since words later in a line sit at larger offsets, the conversion cost grows.ptxdeliberately builds aVec<char>for O(1) indexing, but this prefix rescan defeated that intent.Fix, part 1 - eliminate the rescans (big speedup)
create_word_setalready walks each line's matches in increasing byte order viafind_iter, so I convert offsets with a running cursor (only counting the characters between the previous keyword and the current one) and cache the result asWordRef::char_position. ASCII lines get a fast path where byte == char, so the conversion is a no-op.prepare_line_chunksthen just reads the cached value.This collapses the curve from quadratic to flat:
xychart-beta title "Runtime vs line length, fixed 2 MB input (lower is better)" x-axis ["6KB", "25KB", "100KB", "400KB", "800KB", "1.6MB", "2MB(1 line)"] y-axis "Seconds" 0 --> 11 line [0.39, 0.47, 0.82, 2.20, 3.76, 6.72, 10.02] line [0.33, 0.32, 0.32, 0.33, 0.35, 0.32, 0.34]Upper curve = baseline (blows up with line length); flat lower curve = after the fix.
But this honestly made things worse in one dimension: caching a
usizeper word occurrence pushed average peak memory up ~16% (≈66.6 → ≈77.3 MiB). I wasn't happy shipping a memory regression, so I kept going.Fix, part 2 - pay back the memory
I noticed that each
WordRefalso stored a clonedOsStringfilename, one owned copy per word occurrence. I replaced it with afile_idx: usizeindex into the existingFileMap, looking the name up only when emitting output. As a bonus, this removed anO(files)filename scan that ran on every output line inwrite_traditional_output;file_idxalso disambiguates duplicate file arguments (ptx file file) directly, which the name-based scan had to handle in more complex way.That more than pays for the cached offset. Final memory lands below the original baseline:
xychart-beta title "Average peak memory, 2 MB inputs (lower is better)" x-axis ["Baseline", "Rescan fix only", "Rescan fix + filename dedup"] y-axis "MiB" 0 --> 90 bar [66.6, 77.3, 54.0]Combined result
xychart-beta title "Worst case: single 2 MB line (lower is better)" x-axis ["Baseline", "Rescan fix", "Rescan fix + dedup"] y-axis "Seconds" 0 --> 11 bar [10.02, 0.39, 0.34]Complexity goes from
Θ(n·L)(→Θ(n²)for one long line) to effectivelyΘ(n).Correctness
This is a pure performance change with no behavioral difference:
cargo test -p coreutils --test tests -- test_ptx- 44 passed, 0 failed.tests/ptx/ptx.pl(coreutils 9.11) run against this build - all cases pass, apart from the pre-exisitng regex failure.Shipped as a single, self-contained commit touching only
src/uu/ptx/src/ptx.rs.