-
Notifications
You must be signed in to change notification settings - Fork 424
feat: optimized worker directives (php_worker) #1552
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
Conversation
|
I feel like it's not very intuitive that defining one of those would let all php requests be served by that file. Is there any use case for this outside of situations where the whole app is served through a single entry point? Or is this just a convenience wrapper to skip the php_server block? That would probably be a good thing, especially with the file_server checks. Would it match the performance of an @assets matcher + php directive? |
|
Yeah this essentially does 3 things:
If you think this can be made more intuitive somehow, let me know. It is pretty much equivalent to the following configuration (quick local bench 20 CPU cores ~100.000 RPS) {
frankenphp {
worker {
num 7
file public/worker.php
}
}
}
:8000 {
php_server {
root public
try_files {path} worker.php
}
}Instead with {
frankenphp
}
:8000 {
php_worker {
num 7
file /anywhere/worker.php
root public
}
} |
# Conflicts: # caddy/caddy.go
|
Hmm, on the one hand I like this because it's short and elegant, on the other it's unintuitive (needs good documentation) and I'm not sure if it solves an actual problem or only offers a shortcut - at the cost of an additional directive to maintain. |
|
As mentioned, the main benefit is being able to put the worker outside of the public directory. It's specifically designed for Frameworks like Symfony runtime, Laravel Octane, Yii, etc (probably a majority of worker mode users) Having the option to put the worker file directly into The performance benefit is due to reduced file operations since it's specifically designed for the 'single-entrypoint' use case. I'm also fine with somehow integrating this into existing directives. |
I agree, but I strongly doubt we will see that happening any time soon. I expect at least 10-15 years to pass until the major web servers support this - and until they do, frameworks can't change the index location.
I'm not sure. It would be better in a way because users could choose What has always irked me about the php_worker {
file /path/to/worker.php
root /path/to/project/public
env APP_ENV prod
assets {
path /assets/**
path /bundles/**
}
}could work around that nicely, without a performance penalty. In the end it's a question if the benefit is worth the maintenance burden. That's not up to me to say. |
Frameworks not being able to change the index location is kind of the point. Laravel for instance will copy a file to the public directory at runtime as a workaround.
The overhead of a route directive is not really significant, it's more about reducing unnecessary file operations. It would also be possible to instead add a special case for What would you think about a worker {
file /anywhere/worker php
match index php
} |
| @@ -0,0 +1,250 @@ | |||
| package caddy | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.go?
frankenphp is redundant
| "github.com/dunglas/frankenphp/internal/fastabs" | ||
| ) | ||
|
|
||
| /* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we use double-slash comments, and maybe can we just double quotes or backticks around frankenphp
| "github.com/dunglas/frankenphp/internal/fastabs" | ||
| ) | ||
|
|
||
| /* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (double-slashes and double quotes)
| @@ -0,0 +1,749 @@ | |||
| package caddy | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module.go?
I like this! It would be less maintenance than a new directive and looks like it would carry all the benefit, albeit being slightly more to write. Or, pulling the uno_reverse: example.com {
php {
match assets/* file_server /anywhere/public
match * worker /path/to/worker.php
}
}equivalent to: example.com {
php {
root /anywhere/public/
match assets/* file_server
match * worker /path/to/worker.php
}
}or (now, with the directory limitation) example.com {
root /anywhere/public
route {
@assets {
path /assets/*
}
file_server @assets
rewrite worker.php
php {
root /anywhere/public/
worker worker.php
}
}
} |
|
Hmm I'll probably split this PR up into a refactor section and a section that adds some kind of 'match' feature. Not sure yet how to implement the performance improvements in that case, but it's probably possible somehow |
|
I'd wait what Kevin and Rob think about the match feature first. The refactoring is a great idea either way, though. |
|
I'll probably re-open a refactor PR once #1601 is merged |
#1509 introduced a new way to assign workers to requests without requiring path matching. This inspired me to create a worker configuration that would allow the worker file to sit outside of the public path and to generally optimize file access in the 'modern' framework case.
This PR achieves this with a 'php_worker' directive
In a previous implementation the configuration looked like this. Let me know if you think this or something like this is better
Additional performance benefits are:
file_serveronly makes a single 'file-exists' check, all PHP routes fall back to the workerThis PR also does some refactoring to split up FrankenPHPApp and FrankenPHPModule