Optimize route addition #3
Conversation
|
Just wondering what router you are using. If you need speed you may want to look into FastRoute and it's caching support: https://docs.mezzio.dev/mezzio/v3/features/router/fast-route/#fastroute-caching-support |
I am using Aura. Thank you for FastRoute caching link, but i should mention that its caching doesn't bypass |
|
Also, @nightlinus — check the travis failures. |
nightlinus
force-pushed
the
master
branch
2 times, most recently
from
9da302c
to
25c9001
Compare
|
done |
src/RouteCollector.php
Outdated
| public function __construct(RouterInterface $router) | ||
| { | ||
| $this->router = $router; | ||
| $this->duplicateRouteDetector = new DuplicateRouteDetector(); |
There was a problem hiding this comment.
Why don't you move this in RouteCollectorFactory ?
Also, I suggest you implement an interface, make it invokable and also create an alias in ConfigProvider.
This would allow any developer to use it as a service and also override it with a local implementation.
There was a problem hiding this comment.
This is BC break as was suggested by @weierophinney
There was a problem hiding this comment.
I agree, for another reason, about the factory. (I am not sure about the need for an interface for this task, but not against it)
IMO the duplicateRouteDetector instance should also be allowed to be passed in (and to be) null (via factory-(dev|prod)-configuration), thus skipping duplicate checks. Developers should be allowed to control when actually perform it, e.g. in dev-mode only. A default ALWAYS-ON value would not cause any BC break. Kind regards.
There was a problem hiding this comment.
| $this->duplicateRouteDetector = new DuplicateRouteDetector(); | |
| $this->duplicateRouteDetector = $duplicateRouteDetector ?? new DuplicateRouteDetector(); |
There was a problem hiding this comment.
While I agree about the idea of the factory, for BC purposes, it does need to be enabled by default at the constructor level, not at the factory level. If a consumer had decided to create their own factory for the route collector previously, they would not know to pass the DuplicateRouteDetector instance, and would thus end up in a scenario where duplicate detection is disabled after updating the library within the same major version.
What we can do is use a flag here. Since the logic is marked final, we can have a flag that defaults to true, indicating the DuplicateRouteDetector should be used. Users who want to disable it would pass a boolean false for the flag during instantiation of the RouteCollector, which would disable that.
We can then also add a config flag for the factory to consume in order to disable it.
This would address the BC concerns, as well as the concerns that developers have a way to disable it for production.
I'm working on that now, @nightlinus ; no need for you to do more work than you already have on this!
|
Is there something else i should fix before merging @weierophinney ? |
|
@geerteltink could you give reason for closing this PR? or it was an accidentally hit close PR button? |
|
I did not close it. I'm adding automatic releases according the guide. I guess it got automatically closed when removing develop via cli. If I go over the We should revise the workflow and delete the develop and master branch as last, after rebasing all open PRs. /cc @weierophinney |
|
@nightlinus I cannot reopen or rebase your MR. Can you rebase it onto branch /edit: Fix the issue and reopened and rebased onto |
src/RouteCollector.php
Outdated
| * @var DuplicateRouteDetector | ||
| */ | ||
| private $duplicateRouteDetector; | ||
|
|
||
| public function __construct(RouterInterface $router) |
There was a problem hiding this comment.
| public function __construct(RouterInterface $router) | |
| public function __construct(RouterInterface $router, DuplicateRouteDetector $duplicateRouteDetector = null) |
src/RouteCollector.php
Outdated
| public function __construct(RouterInterface $router) | ||
| { | ||
| $this->router = $router; | ||
| $this->duplicateRouteDetector = new DuplicateRouteDetector(); |
There was a problem hiding this comment.
| $this->duplicateRouteDetector = new DuplicateRouteDetector(); | |
| $this->duplicateRouteDetector = $duplicateRouteDetector ?? new DuplicateRouteDetector(); |
|
Fellows, i dont understand what should i do to help this merged or declined. I have done edits as was suggested by @weierophinney, now i get reviews in different directions. Can someone help me to understand what should i do with this PR please? @geerteltink @samsonasik @boesing |
It used to be O(n) to check duplicate, this introduce hashmap structure for route duplicate checks, thus reduce cost of check to O(1) Signed-off-by: nightlinus <m.a.ogarkov@gmail.com>
Duplicate route detection is enabled by default, via a flag in the `RouteCollector` class. If the flag is set, but no duplicate route detector is present in the instance, one is created on the first request to detect duplicate routes. If disabled, duplicate route detection is not performed. The `RouteCollector` factory now also checks for a configuration value, `Mezzio\Router\RouteCollector.detect_duplicates`; when present, the value is passed for the optional second constructor argument of the `RouteCollector`. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Description
This pr optimizes route addition by removing O(n) duplicate check. In case of big monolitic application this starts to bite performance of application initialization. I have moved duplication check to separate class because now it owns hashmap data structure for searching route.
Solves #1