Fix: Allows disabling root symlinks

[AlliBalliBaba]

Currently this configuration in the Caddyfile will always throw an error if resolve_root_symlink has a second argument

php_server {
    resolve_root_symlink false
}

This pull request fixes the behaviour, so it works in accordance to the docs and allows disabling checking for symlinks.

Not checking for symlinks actually makes a 'Hello World' in frankenphp about 5% faster as you can see in the following flamegraph (path/filepath.EvalSymlinks):

[AlliBalliBaba]

I just realized that in worker mode the value of resolveRootSymlinks is always true since worker mode seems to reference the global FrankenPHPModule and not the one created by the php_server directive.
Is this actually the desired behaviour? Maybe it would make sense to set the default to false here?

frankenphp/caddy/caddy.go

Lines 249 to 252 in 4a85555

	if f.ResolveRootSymlink == nil {
	rrs := true
	f.ResolveRootSymlink = &rrs
	}

[dunglas]

Is this actually the desired behaviour? Maybe it would make sense to set the default to false here?

I think the most plausible reason for this behavior is that I forgot to add the needed config! This isn't intended, we should expose a config for that in the "worker" global block. Would you mind to add it?

I think that we should keep the default to true, because many tools rely on symlinks and because it would be a breaking change to change.
That being said, we should document the performance hit (maybe can we add some sort of cache to limit it by the way) and update our benchmarks to disable this feature!

[dunglas]

After reading the code a second time, I think this is a leftover of some tests. Removing these two lines should be enough to fix the bug: https://github.com/dunglas/frankenphp/blob/main/caddy/caddy.go#L330-L331

(still, we must handle the error case too).

[AlliBalliBaba]

I removed the 2 lines and yeah it works now 👍
It also works in worker mode, I was initially just confused, because I added a php before file_server directive, which caused the second php_server directive to be ignored (since I guess it parses the first directive)

php before file_server
php_server {
    resolve_root_symlink false
}

[AlliBalliBaba]

I do have some other small ideas that I might look into in the future based on these flamegraphs (created from the 'Lock Free Handles' branch):

• The fileserver does work even when no files are being served (php_server is around 12% slower than php)
• The zap.logger does work even when nothing is logged
• frankenphp_register_bulk_variables becomes a bottleneck at some point (maybe registering php server variables just is that slow?)
• runtime.futex sometimes takes up more time when there are more threads created (maybe that's just caused by contention for the request channels?)

[dunglas]

Thanks for your research!

For the fileserver hit, I don't know how we can improve this, but maybe @francislavoie or @mholt will have an idea.

For Zap, wrapping all calls with the Check() method as we do on Mercure is known to improve performance: https://github.com/dunglas/mercure/blob/4e2e253d2c68b78026cccdb2adabfc6b125e14de/handler.go#L103-L105

frankenphp_register_bulk_variables is indeed a heavy operation (most of the work is actually done in this function). I already optimized this path, but there is likely room for improvement.

I've no specific idea for runtime.futex, but IIRC it is quite common and expected in Go (due to how the Go runtime works).

[francislavoie]

The filesystem hit is why we have it off by default in Caddy itself for php_fastcgi. I don't see how it can be avoided when you actually use symlinks.

[dunglas]

@francislavoie thanks for the hint! Having the file server available is convenient and needed for the default setup of Laravel and Symfony, so I propose to keep it by default (especially because it's possible to disable it for maximum performance). However, we should update our benchmarks as well as the TechEmpower benchmark to use the php directive directly.

I'll try to add a "performance" docs entry to compile all these tips.

[dunglas]

Thank you very much for this fix @AlliBalliBaba!