Skip to content

perf: avoid copying already rooted request paths - #2794

Merged
dunglas merged 4 commits into
php:mainfrom
andreybolonin:perf/avoid-rooted-path-copy
Nov 2, 2026
Merged

dunglas merged 4 commits into
php:mainfrom
andreybolonin:perf/avoid-rooted-path-copy

Conversation

@andreybolonin

@andreybolonin andreybolonin commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Preparing SCRIPT_FILENAME currently prepends / even when the request path already starts with it. The resulting extra slash makes path.Clean rebuild an otherwise clean path.

Only prepend / for unrooted paths. Keep path cleaning, the Windows locality check, and trailing-slash handling. Add cases for repeated leading slashes and traversal, plus a benchmark for rooted, relative, and traversal paths.

Earlier standalone measurements of this helper (Go 1.27.3, darwin/arm64, median of five 200ms samples):

Path Before After Time reduction Go B/op allocs/op
/index.php 111.8 ns 69.69 ns 37.7% 80 → 32 4 → 1
/products/catalog/index.php 153.9 ns 110.9 ns 27.9% 144 → 48 4 → 1
Long rooted path 499.2 ns 413.3 ns 17.2% 656 → 176 4 → 1

These are helper-level results, not whole-request or RSS measurements. Relative paths had a 2.4% time reduction with unchanged allocations; traversal paths had an 11.0% reduction with Go B/op dropping from 96 to 72. The original benchmark cases were not rerun.

Validation:

  • gofmt and git diff --check passed.
  • The exact helper and its repository path tests passed with -race in an isolated harness without PHP linkage.
  • A differential fuzz run against the original helper passed 657,119 executions.
  • The isolated tests cross-compiled for Windows; Windows runtime tests were not run locally.
  • Full FrankenPHP tests were not run: the local environment lacks the required PHP ZTS/shared-library and watcher setup.

Adversarial benchmark cases use 4096 leading slashes and 512 rooted a/../ segments. Separate exploratory measurements (Go 1.27.3, Apple M2, darwin/arm64; one 300ms sample per case):

Input Before After Go B/op allocs/op
4096 leading slashes 3260 ns 2782 ns 9776 → 4912 4 → 3
512 rooted a/../ segments 3533 ns 3391 ns 5424 → 2736 4 → 3

These exploratory measurements do not establish statistical significance or an improvement on every input. Allocation counts and bytes do not increase in these cases.

The branch also restores the os import needed by os.ErrDeadlineExceeded, which was lost when main was merged and caused the CI compilation failure.

@dunglas
dunglas force-pushed the perf/avoid-rooted-path-copy branch from 6b782fb to 4affa49 Compare November 1, 2026 15:34
@henderkes

Copy link
Copy Markdown
Contributor

You've not measured the adversary case yet, please do that too. Otherwise approved.

@henderkes henderkes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have ensureLeadingSlaah I think. Can't it be used here?

Comment thread frankenphp.go Resolved
Comment thread cgi_test.go Resolved
@dunglas
dunglas force-pushed the perf/avoid-rooted-path-copy branch from e72cb06 to 03c2c5f Compare November 2, 2026 13:16
@dunglas
dunglas force-pushed the perf/avoid-rooted-path-copy branch from 03c2c5f to c509bc5 Compare November 2, 2026 13:27
@dunglas
dunglas merged commit d82fc4f into php:main Nov 2, 2026
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants