Skip to content

perf: assemble cookies directly in the C buffer - #2797

Open
andreybolonin wants to merge 1 commit into
php:mainfrom
andreybolonin:perf/build-cookies-in-c-buffer
Open

andreybolonin wants to merge 1 commit into
php:mainfrom
andreybolonin:perf/build-cookies-in-c-buffer

Conversation

@andreybolonin

Copy link
Copy Markdown
Contributor

Reading multiple Cookie fields currently joins them into a Go string, removes NUL bytes into another string when necessary, and copies the result into a C string for PHP.

Compute the filtered length and copy the fields and ; separators directly into the final NUL-terminated C buffer. Preserve empty-field handling, nil versus a non-nil empty result after NUL removal, field order, and the existing joined-length overflow checks. The buffer still uses malloc/free and is released by frankenphp_free_request_context().

Add cookie parsing regression cases for classic and worker mode: absent and empty fields, NUL-only fields, NUL within values, separators around empty fields, duplicates, and long values.

Initial standalone prototype measurements (Go 1.27.3, Apple M2, darwin/arm64, one 150ms sample per case, including freeing the result):

Cookie input Before After Time reduction Go B/op Go allocs/op
Two fields with 256-byte values 143.5 ns 103.7 ns 27.7% 576 → 0 1 → 0
Two short fields 89.08 ns 77.85 ns 12.6% 32 → 0 1 → 0
One short field 69.32 ns 67.98 ns 1.9% 0 → 0 0 → 0
Six short fields 112.4 ns 112.4 ns 0% 32 → 0 1 → 0

These exploratory results describe the prototype before the joined-length overflow guards were restored; the final revision has not been benchmarked. Go benchmem excludes C allocations. Both implementations request the same final C allocation size, and this change removes intermediate Go strings. Whole-request latency and process RSS were not measured.

Validation: gofmt and git diff --check passed. The exact final CGO helper passed edge cases and -race in an isolated harness. Differential fuzzing against the previous Join/ReplaceAll behavior passed 580,435 executions; go vet passed. The added PHP integration cases and full suite require CI: the local environment lacks the required PHP ZTS/shared-library and watcher setup. Benchmarks were not rerun for this PR.

This branch has not been deployed

No deployments
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.

1 participant