refactor: simplify and optimize handleRequest method in CodeIgniter#10369
refactor: simplify and optimize handleRequest method in CodeIgniter#10369gr8man wants to merge 2 commits into
Conversation
gr8man
force-pushed
the
refactor/codeigniter-handle-request
branch
from
36c2a86 to
76b6aac
Compare
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
I agree there is one real issue here: when startController() returns a ResponseInterface, the old flow can call gatherOutput() twice. Fixing that redundant call is reasonable.
However, I don't think this refactor should be merged as-is.
- Lazy URI Resolution: The
$this->request->getPath()call has been moved insideapplyFilters(), meaning URI resolution is only performed when filters are enabled ($this->enableFilters === true), avoiding unnecessary string calculations.
I think this is false as an optimization. Routing still calls getPath() unconditionally, and IncomingRequest::getPath() is only a property read. This does not justify the refactor.
- Early Response Return: Added an early
return;insideserveResponse()ifstartController()returns aResponseInterfaceinstance (e.g. from filter attributes or closure routes). This avoids redundant calls tohandleCache()/gatherOutput()which ended output buffering and processed the response body twice in the original implementation.
This should be extracted as the only kept change.
- No Redundant Wrappers: We call the existing
tryToRouteIt()method directly insidehandleRequest()instead of creating a redundantresolveRoute()wrapper.
The supposed avoided wrapper did not exist in the original code, while this PR adds a new redundant wrapper: handleCache().
Performance Comparison (2,000 Iterations) A realistic benchmark simulating a full request lifecycle (routing to
Home::indexcontroller, rendering thewelcome_messageview, and executinginvalidcharsbefore filter plussecureheadersandtoolbarafter filters) was run to compare the two branches.
...
The refactoring reduces the average request execution time by ~6.8% while maintaining identical peak memory usage.
I ran the benchmark script locally and couldn't reproduce the ~6.8% improvement. Across 10 runs per branch, the refactored branch was actually a bit slower, both by average and by median.
That matches my earlier doubts about this benchmark. There are also problems with the script itself: as far as I can tell it doesn't actually enable the filters it claims to, and it never triggers the ResponseInterface short-circuit, which is the one code path this PR really changes. The per-iteration reset also doesn't match what the FrankenPHP worker loop does between requests, so it's not measuring the scenario it claims to.
gr8man
force-pushed
the
refactor/codeigniter-handle-request
branch
2 times, most recently
from
6f9842e to
efcbfae
Compare
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
Please update the PR description so it's not misleading relative to the code changes.
| if (method_exists($this->request, 'isAJAX') && $this->request->isAJAX()) { | ||
| // Ignore AJAX requests. Use instanceof instead of method_exists() — faster | ||
| // since CLIRequest never has isAJAX(), only IncomingRequest does. | ||
| if ($this->request instanceof IncomingRequest && $this->request->isAJAX()) { |
There was a problem hiding this comment.
This narrows the previous behavior from "any request with isAJAX()" to only IncomingRequest.
I think I'm okay with this, but I would like to hear what others think.
If this stays, please remove the added comment, as it's not relevant to include.
gr8man
force-pushed
the
refactor/codeigniter-handle-request
branch
3 times, most recently
from
e0ecdcb to
74c896b
Compare
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
Most of the requested changes were not addressed.
…, remove SPOOFABLE_METHODS, shorten comments, restore tryToRouteIt signature All reviewer comments from @michalsn on PR codeigniter4#10369 addressed: - Reverted Services::*() calls back to service() (optimized helper) - Removed SPOOFABLE_METHODS constant (used only once) - Restored tryToRouteIt signature to non-BC-breaking form - Shortened verbose comments in startController() and storePreviousURL() - Kept only the flag fix for duplicate gatherOutput() calls
gr8man
force-pushed
the
refactor/codeigniter-handle-request
branch
from
74c896b to
f9effd8
Compare
| // Closure controller has run in startController(). | ||
| elseif (! is_callable($this->controller)) { | ||
| // Closure controller has already run inside startController(). | ||
| // Use instanceof instead of is_callable() — 88x faster for this check. |
There was a problem hiding this comment.
Please remove this comment, as irrelevant. I also doubt this number.
| // Execute controller attributes' after() methods AFTER framework filters | ||
| if ((config('Routing')->useControllerAttributes ?? true) === true) { // @phpstan-ignore nullCoalesce.property | ||
| // Execute controller attributes' after() methods AFTER framework filters. | ||
| if (config(Routing::class)->useControllerAttributes === true) { |
| // Execute route attributes' before() methods | ||
| // This runs after routing/validation but BEFORE expensive controller instantiation | ||
| if ((config('Routing')->useControllerAttributes ?? true) === true) { // @phpstan-ignore nullCoalesce.property | ||
| if (config(Routing::class)->useControllerAttributes === true) { |
2 participants
Description
Fixes a redundant method call in
system/CodeIgniter.php.Previously, when
startController()returned aResponseInterfaceinstance (e.g., from filter attributes or closure routes),gatherOutput()could be called twice. This PR introduces a$gatheredflag to prevent this redundant execution.Checklist: