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.
| $csp = service('csp'); | ||
| $csp = Services::csp(); |
There was a problem hiding this comment.
Please revert everywhere. We prefer the use of service() as it's optimized.
| protected function tryToRouteIt(?RouteCollectionInterface $routes = null) | ||
| protected function tryToRouteIt(?RouteCollectionInterface $routes = null, ?string $uri = null) |
There was a problem hiding this comment.
This is a BC break, without real gain. This is just one getPath() call. Please revert.
| // Is it routed to a Closure? | ||
| if (is_object($this->controller) && ($this->controller::class === 'Closure')) { | ||
| // Is it routed to a Closure? Use instanceof — faster than is_object() + ::class string check. | ||
| if ($this->controller instanceof Closure) { |
There was a problem hiding this comment.
It's likely true, but please remove the added part of the comment. I see no reason why we should have this in the code.
| 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.
| if (in_array($method, [Method::PUT, Method::PATCH, Method::DELETE], true)) { | ||
| if (in_array($method, self::SPOOFABLE_METHODS, true)) { |
There was a problem hiding this comment.
I'm not sure if this gives us any benefits here. This constant will be used only once.
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
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: