-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance: adds application snapshot and minimizes App::make #980
base: 2.x
Are you sure you want to change the base?
Performance: adds application snapshot and minimizes App::make #980
Conversation
# Conflicts: # src/Concerns/ProvidesDefaultConfigurationOptions.php # src/Listeners/GiveNewApplicationInstanceToRouter.php # src/OctaneServiceProvider.php
# Conflicts: # src/Worker.php
Does this PR increase the risk of state bleeding between requests that we don't want to? It feels like re-using the same app instance fundamentally changes how Octane manages state between requests. |
This PR should reduce the risk of state bleeding between requests since we cannot have a stale app instance with stale references. All references that the application holds are stored in public/protected variables and these are reset between requests. That basically makes the app container a clone of the original app instance without changing the reference. The only thing I'm currently a bit unhappy about is the BC break. It should be possible to keep the |
Alright, I rewrote the implementation a bit. It now keeps the Event Listeners, but they will use a custom Event Dispatcher. This allows Listeners to keep the references to initial instances themselves. Additionally, the listeners can again be registered in the config one by one (if someone dislikes the defaults). This has similar performance gains to the previous implementation (the benchmarks I'm using can be found here) On a sidenote, tests for 8.1 are currently generally failing for Openswoole |
This PR extends on #888 and might be better intended for a 3.x version. I'm merging to this branch anyways since 3.x doesn't exist yet and I want to share my findings.
Octane really improves upon Laravel's performance by keeping the Application instance in memory, but there are still a lot of areas for possible improvement. After creating some Flamegraphs with
xhprof
and a Laravel Octane 'Hello World', I noticed that a lot of request time is spent onApplication::make()
. CallingApplication::make()
seems to be ~250 times slower (JIT enabled) than accessing an object from an array even if it's just an instance registered with the container.During a simple api request
Application::make()
is called many times, most notably once for each Event Listener and twice for each Middleware. It accounts for roughly 40% of time spent:Flamegraph 2.x
This PR re-introduces the Application 'snapshot' approach from #888, removes the default event listeners and accesses all initially registered Service Providers directly:
Flamegraph this PR
As you can see, most of the slowdown from Application::make is removed. It now only accounts for ~20% of the request time spent. The speed improvement is also notable in a quick local benchmark (almost 50% more RPS):
wrk -t2 -c 120 http://localhost/hello-world
20 CPU coresdunglas/frankenphp
Docker on WSL UbuntuCan this be even faster?
I think with Octane's unique request flow it should in principle be possible to completely remove most of the overhead coming from
Application::make()
. Laravel's container implementation could be tweaked for faster access in long running processes by making writing to the container more expensive while reading from the container more efficient. What such an implementation might look like, I don't know yet though.BC break
This PR will not break applications that use Octane in it's default state. The following lines will only break if users have attempted to unpack these event listeners manually (since the underlying Event Listeners do not exist anymore)
octane/config/octane.php
Lines 73 to 77 in ee88fe3
If someone did indeed unpack these listeners, it was likely for performance reasons anyways.
Registering the application via a snapshot will also reduce the likelihood of memory leaks since the app instance does not change in-between requests (as explained in #888). The only BC break here could occur if users created a custom Application that extends the Laravel base application. Custom private fields on that application instance would not reset anymore between requests (not sure if this is something people even do).