Remove extra ast pass for Pattern#names #39914
Conversation
Before this commit #ast and #names were each doing a separate pass over the journey ast. Since both of these methods are called for every route definition, we can do that work up front in #initialize, consolidating into a single pass through the ast. I think we can also get rid of the `spec` alias, but I am planning to do that in a separate PR to limit the scope of this one. This is similar to the work done in rails#38901 and rails#39903, and the benchmark results are similar: ``` Warming up -------------------------------------- before 7.791k i/100ms after 15.843k i/100ms Calculating ------------------------------------- before 78.081k (± 1.4%) i/s - 397.341k in 5.089797s after 159.093k (± 1.8%) i/s - 807.993k in 5.080367s Comparison: after: 159093.1 i/s before: 78081.2 i/s - 2.04x (± 0.00) slower MEMORY Calculating ------------------------------------- before 240.000 memsize ( 40.000 retained) 4.000 objects ( 1.000 retained) 0.000 strings ( 0.000 retained) after 120.000 memsize ( 40.000 retained) 2.000 objects ( 1.000 retained) 0.000 strings ( 0.000 retained) Comparison: after: 120 allocated before: 240 allocated - 2.00x more ```
Along the same lines as rails#38901, rails#39903, and rails#39914, this commit removes one pass through the journey ast in an attempt to improve application boot time. Before this commit, `Mapper#path` would iterate through the ast to alter the regex for custom routes. With this commit we move the regex alterations to initialize, where we were already iterating through the ast. The Benchmark for this change is similar to what we have been seeing in the other PRs. ``` Warming up -------------------------------------- after 13.121k i/100ms before 7.416k i/100ms Calculating ------------------------------------- after 128.469k (± 3.1%) i/s - 642.929k in 5.009391s before 76.561k (± 1.8%) i/s - 385.632k in 5.038677s Comparison: after: 128469.4 i/s before: 76560.8 i/s - 1.68x (± 0.00) slower Calculating ------------------------------------- after 160.000 memsize ( 0.000 retained) 3.000 objects ( 0.000 retained) 0.000 strings ( 0.000 retained) before 360.000 memsize ( 0.000 retained) 6.000 objects ( 0.000 retained) 0.000 strings ( 0.000 retained) Comparison: after: 160 allocated before: 360 allocated - 2.25x more ```
This isn't quite true: If I remove these deprecations (which will be gone before the next release), the application I work on which has 3897 routes sees 3892 calls to I haven't benchmarked it yet, but I wonder if the cost of eagerly building |
|
Eep! Good call. Yeah that changes things. I'm going to close this PR for now, since I mainly opened it on the way to what I am working on in https://github.com/composerinteralia/rails/tree/journey-performance-improvements, where I try to combine some of the passes in path/pattern with those in routing/mapper (it's still a work in progress). |
|
Nice, that looks promising! Thanks for working on this, by the way; as someone whose app has nearly four thousand routes I'm always willing to review PRs that improve their performance 😁 |
|
Yeah, that is a lot of routes. Removing these extra ast passes won't be a huge win, but I'm hoping to chip away at it little by little. You may also have thoughts about #39916, which is rather similar to this one. |
Before this commit #ast and #names were each doing a separate pass over
the journey ast. Since both of these methods are called for every route
definition, we can do that work up front in #initialize, consolidating
into a single pass through the ast.
I think we can also get rid of the
specalias, but I am planning to dothat in a separate PR to limit the scope of this one.
This is similar to the work done in #38901 and #39903, and the benchmark
results are similar: