[7.x] Refactor route caching #31188

driesvints · 2020-01-21T15:41:07Z

The past couple of weeks I've been trying to update the route caching system by using the new Symfony CompiledUrlMatcherDumper and CompiledUrlMatcher in order to get faster route resolving. Unfortunately, the benchmarks that I did so far haven't been very promising. The speed of route resolving vs the old system has almost no difference. By opening a PR here I'm hoping to see if anyone's able to chip in and help out to see if there's anything I'm missing or doing wrong. I've left some comments on the PR itself on some of the parts I'm doubting or where I believe we could do better.

Would love to get some feedback here 🙏

Big thanks to @frankdejonge for the initial inspiration with his work on this! https://github.com/frankdejonge/framework/pull/1

src/Illuminate/Routing/RouteCollection.php

Miguel-Serejo · 2020-01-21T15:46:52Z

Define "almost no difference" and what % you're looking for, and in what circumstances.

Do you have any benchmark code for people to test this out and try to improve?

src/Illuminate/Routing/RouteCollection.php

driesvints · 2020-01-21T16:35:34Z

@36864 definitely. I used the ab benchmark tool to do my benchmarks.

Here's a benchmark for 800 dynamic routes:

routes.php:

<?php

Route::get('/', 'Controller@index');

for ($i = 0; $i < 800; $i++) {
    Route::get("/foo/{bar}/$i", 'Controller@index')->name("non-static-$i");
}

Benchmark for current cached routing returns 269 requests per second:

View benchmark

$ ab -t 10 -c 10 http://test-routes.test/foo/wrwer/799
This is ApacheBench, Version 2.3 <$Revision: 1843412 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking test-routes.test (be patient)
Finished 2691 requests


Server Software:        nginx/1.17.7
Server Hostname:        test-routes.test
Server Port:            80

Document Path:          /foo/wrwer/799
Document Length:        2426 bytes

Concurrency Level:      10
Time taken for tests:   10.001 seconds
Complete requests:      2691
Failed requests:        0
Total transferred:      9127040 bytes
HTML transferred:       6528366 bytes
Requests per second:    269.08 [#/sec] (mean)
Time per request:       37.164 [ms] (mean)
Time per request:       3.716 [ms] (mean, across all concurrent requests)
Transfer rate:          891.23 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    19   37  34.9     33    1582
Waiting:       19   37  34.9     33    1581
Total:         19   37  34.9     33    1582

Percentage of the requests served within a certain time (ms)
  50%     33
  66%     38
  75%     41
  80%     43
  90%     50
  95%     55
  98%     65
  99%     72
 100%   1582 (longest request)

Benchmark for new cached routing returns 221 requests per second:

View benchmark

$ ab -t 10 -c 10 http://test-routes-caching.test/foo/ewrrre/799                ~/Sites/test-routes-caching
This is ApacheBench, Version 2.3 <$Revision: 1843412 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking test-routes-caching.test (be patient)
Finished 2215 requests


Server Software:        nginx/1.17.7
Server Hostname:        test-routes-caching.test
Server Port:            80

Document Path:          /foo/ewrrre/799
Document Length:        2426 bytes

Concurrency Level:      10
Time taken for tests:   10.003 seconds
Complete requests:      2215
Failed requests:        0
Total transferred:      7512576 bytes
HTML transferred:       5373590 bytes
Requests per second:    221.44 [#/sec] (mean)
Time per request:       45.160 [ms] (mean)
Time per request:       4.516 [ms] (mean, across all concurrent requests)
Transfer rate:          733.44 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    12   45  99.1     32    2512
Waiting:       12   45  99.1     32    2512
Total:         12   45  99.1     32    2513

Percentage of the requests served within a certain time (ms)
  50%     32
  66%     35
  75%     38
  80%     39
  90%     50
  95%     85
  98%    244
  99%    280
 100%   2513 (longest request)
  90%     50
  95%     55
  98%     65
  99%     72
 100%   1582 (longest request)

What must be noted is that I get widely different results with ab from time to time for some reason. I have no idea why that is.

Miguel-Serejo · 2020-01-21T16:46:45Z

Thanks. I'll see if I can dig into this tonight.

Looks like the new system is slower on average, but has a slightly faster best-case.

It might be worth benchmarking cases with different number of routes to determine if the changes made have an impact on every app or only those with many routes (although I guess if an app only has 5 routes no amount of fiddling with the caching mechanism would have a significant impact)

As for the random variance you're getting, try reducing the concurrency level for ab.

danilopinotti · 2020-01-21T18:01:31Z

Did you try to use json_encode/json_decode instead of serialize/unserialize?

src/Illuminate/Foundation/Console/RouteCacheCommand.php

donnysim · 2020-01-21T18:21:08Z

Don't know, for me the new version is faster (ran multiple times):

Windows 10, PHP 7.4.1, Apache
ab -t 10 -c 10

Laravel 6

This is ApacheBench, Version 2.3 <$Revision: 1826891 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking laravel6.test (be patient)
Finished 2529 requests


Server Software:        Apache/2.4.35
Server Hostname:        laravel6.test
Server Port:            80

Document Path:          /foo/wrwer/799
Document Length:        2 bytes

Concurrency Level:      10
Time taken for tests:   10.005 seconds
Complete requests:      2529
Failed requests:        0
Total transferred:      2452280 bytes
HTML transferred:       5058 bytes
Requests per second:    252.77 [#/sec] (mean)
Time per request:       39.562 [ms] (mean)
Time per request:       3.956 [ms] (mean, across all concurrent requests)
Transfer rate:          239.36 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.5      0       1
Processing:    15   37  97.8     23     934
Waiting:       15   37  97.8     22     933
Total:         15   38  97.8     23     934

Percentage of the requests served within a certain time (ms)
  50%     23
  66%     24
  75%     24
  80%     25
  90%     26
  95%     28
  98%    518
  99%    682
 100%    934 (longest request)

Laravel 7 refactor-route-caching

This is ApacheBench, Version 2.3 <$Revision: 1826891 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking laravel.test (be patient)
Finished 3189 requests


Server Software:        Apache/2.4.35
Server Hostname:        laravel.test
Server Port:            80

Document Path:          /foo/wrwer/799
Document Length:        2 bytes

Concurrency Level:      10
Time taken for tests:   10.010 seconds
Complete requests:      3189
Failed requests:        0
Total transferred:      3182876 bytes
HTML transferred:       6380 bytes
Requests per second:    318.57 [#/sec] (mean)
Time per request:       31.390 [ms] (mean)
Time per request:       3.139 [ms] (mean, across all concurrent requests)
Transfer rate:          310.51 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.5      0       1
Processing:    15   30  73.0     21     816
Waiting:       13   30  73.0     21     816
Total:         15   31  73.0     21     817

Percentage of the requests served within a certain time (ms)
  50%     21
  66%     22
  75%     23
  80%     23
  90%     24
  95%     25
  98%     28
  99%    548
 100%    817 (longest request)

driesvints · 2020-01-21T20:18:59Z

Did you try to use json_encode/json_decode instead of serialize/unserialize?

No. I don't think that'll be any faster though?

Don't know, for me the new version is faster:

Thanks for testing! Did you try running the benchmarks a couple of times to see if you get similar results?

donnysim · 2020-01-21T20:22:21Z

Yeah, around 10 times each, with opcache and without, new version was always faster by 30+ requests/s.

danilopinotti · 2020-01-21T20:39:48Z

No. I don't think that'll be any faster though?

In very simple tests, json_decode is faster, but, with complex and larger objects, json_decode is more slow than unserialize.

Really, will not bring improvements.

nicolas-grekas · 2020-01-22T08:59:31Z

Sorry for the noob question, but could anyone provide me with a small app that would run this PR and allow me to run the ab benchs too?

gocanto · 2020-01-22T09:19:28Z

Sorry for the noob question, but could anyone provide me with a small app that would run this PR and allow me to run the ab benchs too?

@nicolas-grekas

you will have to create a dummy app and then pull this version thru composer when requiring the framework

or you could clone this repo and do the same ^

nicolas-grekas · 2020-01-22T09:22:29Z

@gocanto thanks, that's the parts I figured out, but then, I don't know what to do, which files to patch, how to compile the routes, etc :)

Miguel-Serejo · 2020-01-22T09:25:08Z

I don't know what to do, which files to patch, how to compile the routes, etc :)

Once you have this branch pulled, you'll need to empty the contents of routes/api.php, add a method named index to the App\Http\Controllers\Controller class, and then add the test routes to routes/web.php.

Once you've done that, run php artisan route:cache to cache the routes.

gocanto · 2020-01-22T09:27:23Z

@nicolas-grekas I see.

If you want to compile the routes, you will have to go into your dummy app console line and type php artisan route:cache.

After this is done, you should be able to see a compiled routes file within your boostrap/cache directory.

Miguel-Serejo · 2020-01-22T09:30:52Z

After typing out all the steps I realized it would have been easier to just upload my test project. Here you go: https://github.com/36864/laravel-route-cache-test

Just clone that, run composer install, and then php artisan route:cache.

nicolas-grekas · 2020-01-22T10:01:26Z

Thanks @36864 that helped a lot!

So here are my quick findings:

The biggest perf hog is the call to app('router')->setRoutes() - actually it's the unserialization+decoding of its argument - a huge payload.

With the current state of the PR, I'm at 74 req/s.

If I comment the call to setRoutes() in the routes cache, I get 180 req/s.

Then, if I switch to using var_export() instead of base64+serialize, I top at 250 req/s.

My reco would be to use var_export() for the call to setCompiledRoutes(): it's been optimized for leveraging opcache static arrays.

For setRoutes(), using b64+serialize looks fine, but I would recommend finding a way to NOT do the call - it's so costly to create this RouteCollection. At least, it should be done lazily, if possible.

danilopinotti · 2020-01-22T13:02:59Z

Another problem that I found in current route cache implementation is the high RAM consume in a request when system has a lot of routes (1000+). It increments about 10MB of RAM by each request. Currently, I needed to disable route cache when production because this problem.

Will this problem be solved? Maybe split cache files by first character may be resolve the problem.

driesvints · 2020-01-22T17:12:17Z

@nicolas-grekas thank you for helping out here! Some thoughts on your feedback:

If I comment the call to setRoutes() in the routes cache, I get 180 req/s.

Hmm, technically it's not possible that the current PR will work without the call. If I comment it out myself I get a 404 when I visit any route but I do get improved results from the ab testing tool and it's not showing any failures (which is bizar because the requests return 404's).

Then, if I switch to using var_export() instead of base64+serialize, I top at 250 req/s.

If I use var_export for the regular routes I get the following error:

Call to undefined method Illuminate\Routing\RouteCollection::__set_state()

I guess you implemented this method on the RouteCollection?

My reco would be to use var_export() for the call to setCompiledRoutes(): it's been optimized for leveraging opcache static arrays.

I've just pushed a commit with switching to var_export for setCompiledRoutes. Already seeing some better results indeed!

For setRoutes(), using b64+serialize looks fine, but I would recommend finding a way to NOT do the call - it's so costly to create this RouteCollection. At least, it should be done lazily, if possible.

I'll try to look into this tomorrow if that's possible.

One more question: what exact benchmark command are you using? Thanks again for helping out!

driesvints · 2020-01-22T17:52:27Z

@taylorotwell let me know that with the latest version he isn't seeing improvements himself.

nicolas-grekas · 2020-01-22T17:56:04Z

Call to undefined method Illuminate\Routing\RouteCollection::__set_state()

Actually I didn't, I just commented out the call :)

But that's definitely the next thing to optimize. Could be by implementing this method, or by creating a new CompiledRouteCollection that would take a simple array as argument (the class would create individual route objects on demand.)

what exact benchmark command are you using?

It was ab -n500 with php artisan serve :)

GrahamCampbell · 2020-01-22T21:59:16Z

Was opcache enabled?

GrahamCampbell · 2020-01-22T22:00:27Z

I'm assuming opcache would execute the base64 decode ahead of time, if enabled, which could explain why you and Dries were seeing some differences.

nicolas-grekas · 2020-01-22T22:03:01Z

It was enabled for me, but it would be totally new knowledge to me if opcache were to resolve b64 ahead of time. What do you base this idea on?

GrahamCampbell · 2020-01-23T08:20:21Z

Opcache already does constant folding and some ahead of time evaluations, like is_string on a value that is known to be a string. I wouldn't be surprised if it did the base64 decode a head of time, but I've not checked the source. If it doesn't... maybe it should. ;)

nicolas-grekas · 2020-01-23T08:42:44Z

For reference, here is what I know on the topic, with links to the source: PHP-CS-Fixer/PHP-CS-Fixer#3048
TL;DR, only a few carefully selected functions benefit from compile-time optimizations and adding more won't happen AFAIK because it would slow down the engine.

driesvints · 2020-01-23T12:37:39Z

I also have opcache enabled.

Could be by implementing this method

Hmm, since CompiledRoute doesn't implements this, this isn't really an option I think 🤔

or by creating a new CompiledRouteCollection that would take a simple array as argument

Hmm this was the path I took first but it overcomplicated the router (because it needs to have the $routes property set). I'm a bit hesitant on trying that again.

I really wonder why there's so much difference in the test results between yours and mine. With your ab test from above I get 85.74 rps on the current route cache and 93.85 rps on the new implementation (both with opcache enabled). I'm a bit at a loss and don't really know what to do any further.

driesvints · 2020-01-23T12:57:56Z

I think I finally managed to crack it. I ditched the setRoutes call like you suggested @nicolas-grekas and I'm now finally seeing much better results. I still need to find a way to lazy-load the routes when getRoutes is called upon the collection but I think we're finally getting somewhere.

I'll pick this up on monday.

driesvints · 2020-02-13T16:04:34Z

Verified that the fallback issue isn't present in this pr. Think except for tests that we're pretty much there then.

@nicolas-grekas I'm wondering if it makes a difference if we just returned an array from the compiled routes file and do the setCompiledRoutes call in the service provider instead. Not sure if that makes a difference for opcache?

https://github.com/laravel/framework/pull/31188/files#diff-285965dba4ad1f52b822bbae17e94ca6R14

nicolas-grekas · 2020-02-13T16:13:10Z

Not preferences in terms of opcache: both ways would provide equal opcache-ability. As fits best for you.

driesvints · 2020-02-14T21:08:49Z

Thank you ALL for helping getting this merged in! Especially @nicolas-grekas and @frankdejonge 👏👏

* upstream/master: (78 commits) [7.x] Implement anonymous components (laravel#31363) Apply fixes from StyleCI (laravel#31480) Apply fixes from StyleCI (laravel#31479) revert broken table feature move files add test for event payload of type object (laravel#31477) [7.x] Refactor route caching (laravel#31188) [6.x] Fixes appendRow on console table (laravel#31469) Apply fixes from StyleCI (laravel#31474) formatting Throw exception on empty collection (laravel#31471) Add tests for Query Builder when array value given (laravel#31464) Remove addHidden method (laravel#31463) allow afterResponse chain add getRawOriginal Remove unused use-statement Update comment [6.x] Change MySql nullable modifier to allow generated columns to be not null (laravel#31452) [6.x] Test for pushed events (laravel#31451) Fixed phpdoc ...

yaquawa · 2020-08-03T00:50:25Z

Hi @driesvints
Thanks for the efforts of this!

But, now you can't set your own validators....
Before this PR, you can replace the validators so we can control the validation rules of routes.

Route::$validators = [
    new MethodValidator,
    new SchemeValidator,
    new HostValidator,
    new UriValidator,
];

This PR completely breaks my code if I cached the routes.

Can we have a change to choose one of them?

driesvints · 2020-08-03T07:51:54Z

@yaquawa this was never documented or publicly supported sorry. You can technically still use that (although I don't recommend to) if you don't use route caching.

yaquawa · 2020-08-03T08:02:02Z

Hi @driesvints, but I don't think it's a good idea to change the behavior of how router works just because caching is applied or not. Why not just use one of them? Two implementations work together feels so weird.

driesvints · 2020-08-03T08:02:49Z

@yaquawa sorry we have no plans to change this.

yaquawa · 2020-08-03T23:34:45Z

@driesvints
I think the only way to workaround is to use a fallback route now, then I found this piece of code, which I don't know what's the point of this.

https://github.com/laravel/framework/blob/7.x/src/Illuminate/Routing/CompiledRouteCollection.php#L132-L142

The try block always thrown because $this->routes always is an empty collection of routes right? If that's true, why will you need such piece of code?

Could you give me some information about this? Thanks.

yaquawa · 2020-08-03T23:37:58Z

@driesvints By the way, do you know any other ways to use my own "match strategy" against the request after this PR?

driesvints · 2020-08-04T08:22:46Z

then I found this piece of code, which I don't know what's the point of this.

To handle dynamic route arguments properly. We have tests to back that.

why will you need such piece of code?

The framework supports dynamically added routes at runtime. This collection holds those routes.

By the way, do you know any other ways to use my own "match strategy" against the request after this PR?

Not at the moment no. You can still use it as long as you don't use route caching (afaik).

yaquawa · 2020-08-04T12:04:13Z

@driesvints

The framework supports dynamically added routes at runtime. This collection holds those routes.

But I think the "dynamic" ones will never get checked through the CompiledUrlMatcher, which means it never get matched, anything I've missed?

driesvints · 2020-08-04T12:08:34Z

We have tests that verify exactly that.

driesvints commented Jan 21, 2020

View reviewed changes

src/Illuminate/Routing/RouteCollection.php Outdated Show resolved Hide resolved

driesvints force-pushed the refactor-route-caching branch from 7e8c990 to 9cb7657 Compare January 21, 2020 16:28

driesvints commented Jan 21, 2020

View reviewed changes

src/Illuminate/Routing/RouteCollection.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Jan 21, 2020

View reviewed changes

src/Illuminate/Foundation/Console/RouteCacheCommand.php Outdated Show resolved Hide resolved

driesvints and others added 6 commits February 13, 2020 16:22

Cleanup RouteCacheCommand

fa717ab

Delay route resolving

d704614

Refactor to CompiledRouteCollection

2af05d4

Types

f9962b6

Revert some changes

a5bb67b

formatting

475635e

driesvints force-pushed the refactor-route-caching branch from 4280851 to 2673a27 Compare February 14, 2020 14:29

Move fallback routes to the end of symfony routes collection

e7ac4f5

driesvints force-pushed the refactor-route-caching branch from 2673a27 to e7ac4f5 Compare February 14, 2020 14:33

Add test for CompiledRoutesCollection

fe0047d

driesvints marked this pull request as ready for review February 14, 2020 15:34

taylorotwell and others added 2 commits February 14, 2020 10:47

formatting

5390a81

Rename cached routes file

c71461c

taylorotwell merged commit 8b134ce into master Feb 14, 2020

driesvints deleted the refactor-route-caching branch February 14, 2020 20:59

dunhamjared mentioned this pull request Apr 28, 2020

Implement route caching speed improvements laravel/lumen-framework#1074

Closed

derekmd mentioned this pull request Jul 9, 2021

[8.x] Fix the cached routes issue #37968

Closed

[7.x] Refactor route caching #31188

[7.x] Refactor route caching #31188

Conversation

driesvints commented Jan 21, 2020 • edited

Miguel-Serejo commented Jan 21, 2020

driesvints commented Jan 21, 2020

Miguel-Serejo commented Jan 21, 2020

danilopinotti commented Jan 21, 2020

donnysim commented Jan 21, 2020 • edited

driesvints commented Jan 21, 2020

donnysim commented Jan 21, 2020

danilopinotti commented Jan 21, 2020

nicolas-grekas commented Jan 22, 2020 • edited

gocanto commented Jan 22, 2020

nicolas-grekas commented Jan 22, 2020

Miguel-Serejo commented Jan 22, 2020

gocanto commented Jan 22, 2020

Miguel-Serejo commented Jan 22, 2020

nicolas-grekas commented Jan 22, 2020 • edited

danilopinotti commented Jan 22, 2020 • edited

driesvints commented Jan 22, 2020

driesvints commented Jan 22, 2020

nicolas-grekas commented Jan 22, 2020

GrahamCampbell commented Jan 22, 2020

GrahamCampbell commented Jan 22, 2020

nicolas-grekas commented Jan 22, 2020

GrahamCampbell commented Jan 23, 2020

nicolas-grekas commented Jan 23, 2020

driesvints commented Jan 23, 2020 • edited

driesvints commented Jan 23, 2020 • edited

driesvints commented Feb 13, 2020 • edited

nicolas-grekas commented Feb 13, 2020 • edited

driesvints commented Feb 14, 2020

yaquawa commented Aug 3, 2020

driesvints commented Aug 3, 2020

yaquawa commented Aug 3, 2020 • edited

driesvints commented Aug 3, 2020

yaquawa commented Aug 3, 2020

yaquawa commented Aug 3, 2020 • edited

driesvints commented Aug 4, 2020 • edited

yaquawa commented Aug 4, 2020

driesvints commented Aug 4, 2020

driesvints commented Jan 21, 2020 •

edited

donnysim commented Jan 21, 2020 •

edited

nicolas-grekas commented Jan 22, 2020 •

edited

nicolas-grekas commented Jan 22, 2020 •

edited

danilopinotti commented Jan 22, 2020 •

edited

driesvints commented Jan 23, 2020 •

edited

driesvints commented Jan 23, 2020 •

edited

driesvints commented Feb 13, 2020 •

edited

nicolas-grekas commented Feb 13, 2020 •

edited

yaquawa commented Aug 3, 2020 •

edited

yaquawa commented Aug 3, 2020 •

edited

driesvints commented Aug 4, 2020 •

edited