-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
perf: use slice for proven ordered bounds #7977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Sheraff
wants to merge
5
commits into
main
Choose a base branch
from
codex/shorter-string-operations
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
780a4ac
perf: use shorter equivalent string operations
Sheraff 1cee449
test(router-core): cover reversible extraction bounds
Sheraff 7e0b0f5
perf: retain indexOf membership probes
Sheraff ae8b039
test(router-core): benchmark ordered string slicing
Sheraff 95bacef
Merge main into codex/shorter-string-operations
Sheraff File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # Shorter equivalent string operations | ||
|
|
||
| ## Principle | ||
|
|
||
| Use `slice` instead of `substring` only when both bounds are proven non-negative and ordered. This change applies that rule to 24 calls. It does not change public options, return shapes, component props, serialized data, or user-visible behavior. | ||
|
|
||
| ## Isolated attribution | ||
|
|
||
| Representative scenario: `react-router.minimal`, measured against `main` at `697ebb6ddbd433d052b6b4707938a5c595865d58`. | ||
|
|
||
| | Hunk | Raw | Initial gzip | Gzip | Brotli | | ||
| | ----------------------------------- | ----: | -----------: | ----: | -----: | | ||
| | Proven-range `substring` to `slice` | -88 B | -10 B | -12 B | -110 B | | ||
|
|
||
| ## Full bundle matrix | ||
|
|
||
| The candidate and control were built independently from the same exact base and dependency graph. The final slice-only candidate is recorded in `/tmp/native-slice-only-full.json`; gzip improves in all 17 scenarios. | ||
|
|
||
| | Scenario | Raw | Initial gzip | Gzip | Brotli | | ||
| | -------------------------------- | ----: | -----------: | ----: | -----: | | ||
| | react-router.minimal | -88 B | -10 B | -12 B | -110 B | | ||
| | react-router.full | -88 B | -11 B | -7 B | -163 B | | ||
| | solid-router.minimal | -88 B | -11 B | -11 B | +21 B | | ||
| | solid-router.full | -88 B | -2 B | -2 B | -10 B | | ||
| | vue-router.minimal | -88 B | -7 B | -8 B | -16 B | | ||
| | vue-router.full | -88 B | -9 B | -8 B | -15 B | | ||
| | react-start.minimal | -88 B | -13 B | -14 B | -1 B | | ||
| | react-start.deferred-hydration | -88 B | -15 B | -14 B | +102 B | | ||
| | react-start.full | -86 B | +3 B | -2 B | +61 B | | ||
| | react-start.rsbuild.minimal | -86 B | -14 B | -14 B | -77 B | | ||
| | react-start.rsbuild.minimal-iife | -86 B | -12 B | -11 B | -27 B | | ||
| | react-start.rsbuild.full | -84 B | -5 B | -5 B | +8 B | | ||
| | solid-start.minimal | -88 B | -13 B | -12 B | +58 B | | ||
| | solid-start.deferred-hydration | -88 B | -15 B | -10 B | -17 B | | ||
| | solid-start.full | -88 B | -17 B | -17 B | -106 B | | ||
| | vue-start.minimal | -88 B | -24 B | -23 B | +2 B | | ||
| | vue-start.full | -88 B | -8 B | -8 B | +4 B | | ||
|
|
||
| Ranges: | ||
|
|
||
| - raw: -88 B to -84 B in all scenarios | ||
| - initial gzip: -24 B to +3 B; 16 improve and one regresses | ||
| - gzip: -23 B to -2 B in all scenarios | ||
| - Brotli: improves in ten scenarios and regresses by 2–102 B in seven | ||
|
|
||
| ## Semantic constraints | ||
|
|
||
| Every changed `slice` bound comes from a fixed non-negative offset or parser offsets that are constructed in ascending order. The five parameter-extraction calls whose bounds can reverse remain `substring`: prefix/suffix overlaps can make value bounds cross, and Unicode lowercasing can expand an affix before name extraction. Focused tests preserve named, optional, and wildcard overlap behavior plus the Unicode length-expansion edge case. | ||
|
|
||
| `String.prototype.slice` is already used throughout the affected shipped packages, so this does not raise the browser-support floor. | ||
|
|
||
| ## Validation | ||
|
|
||
| The focused router-core path suite passes with 361 tests and no type errors. Package-level unit validation also passes: | ||
|
|
||
| - history: 25 tests | ||
| - router-core: 1,528 passed and three expected failures | ||
| - React Router: 989 passed and one skipped | ||
| - Solid Router: 838 passed and one skipped | ||
|
|
||
| The initial combined unit command was stopped after its output stalled twice under the repository's execution guardrail; the package-level runs above completed against the exact candidate. Type tests pass for all four affected packages across TypeScript 5.6 through 7.0. ESLint reports no errors; remaining warnings are pre-existing. | ||
|
|
||
| The focused `path-string-operations.bench.ts` benchmark first compares the changed native operation directly and verifies that `slice` and `substring` return identical values for representative ordered bounds. Across four final runs, `slice` averaged 34,385.01 Hz versus 31,162.09 Hz for `substring`, a 10.34% improvement. | ||
|
|
||
| The same benchmark exercises href parsing, interpolation, route-tree construction, and search-prefix handling through their real APIs. Four bracketed exact-base/candidate pairs produced these average throughputs: | ||
|
|
||
| | Operation | Exact base | Candidate | Change | | ||
| | --------------------------------- | -----------: | -----------: | -----: | | ||
| | Parse 400 hrefs | 11,379.09 Hz | 11,560.88 Hz | +1.60% | | ||
| | Interpolate 300 path templates | 3,691.13 Hz | 3,697.17 Hz | +0.16% | | ||
| | Construct a 30-route dynamic tree | 74,722.08 Hz | 75,369.98 Hz | +0.87% | | ||
| | Parse 400 search prefixes | 7,100.11 Hz | 7,062.15 Hz | -0.53% | | ||
|
|
||
| The search-prefix result has no stable direction: the four paired deltas alternate between -4.54%, +2.05%, -1.75%, and +2.37%, while the directly changed operation is consistently faster. The other workflows are flat to positive. The repository's existing framework link benchmark could not provide a usable control: the React run exhausted its 4 GB heap after existing `act(...)` warnings, and the Solid run loaded a client-only API in server mode. The focused benchmark was added so performance validation would not depend on those unrelated failures. | ||
|
|
||
| ## Rejected nearby variant | ||
|
|
||
| Five boolean `indexOf` probes were also tested as `includes`. That group saved 10 gzip bytes in `react-router.minimal`, but three isolated performance pairs ranged from -1.60% to +0.64% and averaged -0.23%. The result was not confidently neutral, so the group was dropped from the final candidate. | ||
|
|
||
| Five independent publication reviews cover semantic equivalence, adversarial bounds, browser support, public API behavior, tree-shaking, performance, tests, and measurement integrity. Their final disposition is recorded before publication. |
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
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
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
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
130 changes: 130 additions & 0 deletions
130
packages/router-core/tests/path-string-operations.bench.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| import { parseHref } from '@tanstack/history' | ||
| import { bench, describe, expect } from 'vitest' | ||
| import { processRouteTree } from '../src/new-process-route-tree' | ||
| import { interpolatePath } from '../src/path' | ||
| import { parseSearchWith } from '../src/searchParams' | ||
|
|
||
| const hrefs = [ | ||
| '/posts/123?sort=newest#comments', | ||
| '/files/report.pdf#download', | ||
| '/search?q=router&page=2', | ||
| '/plain/path', | ||
| ] | ||
| const interpolationCases = [ | ||
| { | ||
| path: '/teams/$team/projects/$project', | ||
| params: { team: 'router', project: 'benchmarks' }, | ||
| }, | ||
| { | ||
| path: '/files/prefix{$file}.json/{-$revision}', | ||
| params: { file: 'results', revision: 'latest' }, | ||
| }, | ||
| { | ||
| path: '/docs/$', | ||
| params: { _splat: 'guides/data-loading' }, | ||
| }, | ||
| ] | ||
| const routeTree = { | ||
| id: '__root__', | ||
| isRoot: true, | ||
| fullPath: '/', | ||
| path: '/', | ||
| children: Array.from({ length: 30 }, (_, index) => { | ||
| const path = `/team-${index}/prefix{$project}.json/{-$revision}` | ||
| return { id: path, fullPath: path, path } | ||
| }), | ||
| } | ||
| const parseSearch = parseSearchWith((search) => search) | ||
| const extractionCases: ReadonlyArray< | ||
| readonly [value: string, start: number, end?: number] | ||
| > = [ | ||
| ['/posts/123?sort=newest#comments', 0, 10], | ||
| ['/files/prefix{$file}.json', 7, 19], | ||
| ['prefix{$project}.json', 7, 15], | ||
| ['0.123456789abcdefghijklmnopqrstuvwxyz', 7], | ||
| ] | ||
| let benchmarkSink = 0 | ||
|
|
||
| expect(parseHref(hrefs[0]!, undefined)).toMatchObject({ | ||
| pathname: '/posts/123', | ||
| search: '?sort=newest', | ||
| hash: '#comments', | ||
| }) | ||
| expect( | ||
| interpolatePath({ | ||
| ...interpolationCases[1]!, | ||
| server: false, | ||
| }).interpolatedPath, | ||
| ).toBe('/files/prefixresults.json/latest') | ||
| expect(processRouteTree(routeTree).routesByPath).toHaveProperty( | ||
| '/team-29/prefix{$project}.json/{-$revision}', | ||
| ) | ||
| expect(parseSearch('?sort=newest')).toEqual({ sort: 'newest' }) | ||
| expect( | ||
| extractionCases.map(([value, start, end]) => value.slice(start, end)), | ||
| ).toEqual( | ||
| extractionCases.map(([value, start, end]) => value.substring(start, end)), | ||
| ) | ||
|
|
||
| describe('equivalent native string operations', () => { | ||
| bench('substring on proven ordered bounds', () => { | ||
| let size = 0 | ||
| for (let index = 0; index < 1_000; index++) { | ||
| for (const [value, start, end] of extractionCases) { | ||
| size += value.substring(start, end).length | ||
| } | ||
| } | ||
| benchmarkSink = size | ||
| }) | ||
|
|
||
| bench('slice on proven ordered bounds', () => { | ||
| let size = 0 | ||
| for (let index = 0; index < 1_000; index++) { | ||
| for (const [value, start, end] of extractionCases) { | ||
| size += value.slice(start, end).length | ||
| } | ||
| } | ||
| benchmarkSink = size | ||
| }) | ||
| }) | ||
|
|
||
| describe('path string operations', () => { | ||
| bench('parse 400 hrefs', () => { | ||
| let size = 0 | ||
| for (let index = 0; index < 100; index++) { | ||
| for (const href of hrefs) { | ||
| const result = parseHref(href, undefined) | ||
| size += | ||
| result.pathname.length + result.search.length + result.hash.length | ||
| } | ||
| } | ||
| benchmarkSink = size | ||
| }) | ||
|
|
||
| bench('interpolate 300 path templates', () => { | ||
| let size = 0 | ||
| for (let index = 0; index < 100; index++) { | ||
| for (const input of interpolationCases) { | ||
| size += interpolatePath({ | ||
| ...input, | ||
| server: false, | ||
| }).interpolatedPath.length | ||
| } | ||
| } | ||
| benchmarkSink = size | ||
| }) | ||
|
|
||
| bench('construct a 30-route dynamic tree', () => { | ||
| benchmarkSink = Object.keys(processRouteTree(routeTree).routesByPath).length | ||
| }) | ||
|
|
||
| bench('parse 400 search prefixes', () => { | ||
| let size = 0 | ||
| for (let index = 0; index < 400; index++) { | ||
| size += Object.keys(parseSearch('?sort=newest&page=2')).length | ||
| } | ||
| benchmarkSink = size | ||
| }) | ||
| }) | ||
|
|
||
| void benchmarkSink |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add braces to the optional-parameter guard.
Line 385 uses a one-line
ifbody. Add braces to meet the TypeScript control-statement rule.Proposed fix
As per coding guidelines, “Always use curly braces for
if,else, loops, and similar control statements.”📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines