Skip to content
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

docs: try_files performance #1311

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AlliBalliBaba
Copy link
Collaborator

Update to the docs with the most performant file_server setups (disabling file_server won't really gain any performance).

docs/performance.md Outdated Show resolved Hide resolved
Comment on lines 76 to 92
route {
@assets {
path /assets/*
}

# everything behind /assets is handled by the file server
handle @assets {
root /root/to/your/app
file_server
}

# everything that is not in /assets is handled by your index or worker PHP file
rewrite index.php
php {
root /root/to/your/app # explicitly adding the root here allows for better caching
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not tried it, but something like that should work too (and is likely cleaner):

Suggested change
route {
@assets {
path /assets/*
}
# everything behind /assets is handled by the file server
handle @assets {
root /root/to/your/app
file_server
}
# everything that is not in /assets is handled by your index or worker PHP file
rewrite index.php
php {
root /root/to/your/app # explicitly adding the root here allows for better caching
}
}
route {
@assets {
path /assets/*
}
# Everything behind /assets is handled by the file server
file_server @assets {
root /root/to/your/app
}
# Everything that is not in /assets is handled by your index or worker PHP file
php_server {
try_files index.php
file_server off
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

php_server { 
    root /go/src/app/testdata
    try_files index.php
    file_server off
}

seems like 1x file access still from somewhere
flame

php_server {
	root /go/src/app/testdata
	try_files index.php {
		policy first_exist_fallback
	}
	file_server off
}

Even with explicit 'policy first_exist_fallback' this still does a file access
flame

rewrite index.php
php {
	root /go/src/app/testdata
}

With a rewrite and php there is no file access:

flame

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's merge the change as-is then. That would be nice to find out what is causing this extra IO (and if we can prevent it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can wait and see if this can be prevented before merging the docs 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the file access comes from this glob. Tbh first globbing and then checking for explicit file existence just does the same thing twice. This could probably be significantly optimized, but you'd have to be careful about not breaking stuff (especially on windows).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably make a PR to Caddy to optimize this. Skipping glob() on files without wildcards will reduce the number of file operation by 1 in the case any file in try_files matches.

Still, using php instead of php_server removes the overhead of the Matcher, so this PR should be fine to merge as-is.


```caddyfile
php_server {
file_server off
try_files {path} index.php
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, maybe should we explain that in a separate php_server or even try_files section as it's not really related to file_server?

Also, we should update the Laravel page to use this config by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good idea! Probably makes sense to update the default Caddy stub in Octane as well.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes definitely, also API PLatform and Symfony Docker. It's on my todolist.

@AlliBalliBaba AlliBalliBaba changed the title docs: file_server performance docs: try_fiels performance Jan 12, 2025
@AlliBalliBaba AlliBalliBaba changed the title docs: try_fiels performance docs: try_files performance Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants