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

Ensure each plugin has a config.php and tracker.php file #14430

Merged
merged 3 commits into from May 28, 2019
Merged

Conversation

katebutler
Copy link

Fixes #14420

…t have them; add to whitelist of files that are included in new plugins generated by generate:plugin
@katebutler katebutler added the Needs Review PRs that need a code review label May 8, 2019
@katebutler katebutler added this to the 3.11.0 milestone May 8, 2019
@tsteur
Copy link
Member

tsteur commented May 9, 2019

@mattab @toredash

Did a test on a production server... creating a container the regular way where those config files don't exist, took about 1s for 10K containers. So about 0.1ms per container.

With those config files it took about 1.4s for 10K containers, about 0.14ms per container. Opcache is enabled. It may be slower cause while the config file existence might be cached now, it has to do extra work for each config file. It now needs to load each config file, it has to merge it with the regular config etc. It now needs to basically execute the logic in this method for each config file: https://github.com/matomo-org/matomo/blob/3.10.0-b1/core/Container/ContainerFactory.php#L93

For low level traffic, we don't see any improvements. But, when sudden peaks (16req/s to 1000req/s) occurs within 30s, we no longer have issues with increased latency on our Tracker API requests.

Overall we could still merge this assuming it puts less load on to the filesystem when there is heaps of traffic and it becomes more efficient in the end. For low traffic sites it wouldn't matter if it takes 0.1ms or 0.14ms

Any thoughts?

@toredash
Copy link
Contributor

toredash commented May 9, 2019

@tsteur could you post your full opcache config and PHP version used? I assume NFS is not used

@tsteur
Copy link
Member

tsteur commented May 9, 2019

Tested on a regular filesystem and a cached EFS (NFS). If you are using NFS for the whole application, I highly recommend you move matomo application directory on a regular disk and not EFS. EFS/NFS can slow it quite a bit.

If you only have the config files in NFS, then it should be fine. In general you might want to look at https://linux.die.net/man/8/cachefilesd

@toredash
Copy link
Contributor

@tsteur Is opcache configured with opcache.enable_file_override ? If set to true, calls in php, file_exists(), is_file() and is_readable(), should be cached in opcache. We have this enabled, related to #14420

@tsteur
Copy link
Member

tsteur commented May 22, 2019

enable_file_override is off. When it is enabled, shouldn't that make things faster too even without this PR?
Of course you may reduce time on file system re stat calls, on the other side you spend more time in PHP processing these files etc. That's maybe fine though in general as it maybe avoids disk being a bottleneck and you can always scale by adding more servers if needed etc.

@tsteur
Copy link
Member

tsteur commented May 22, 2019

@katebutler could we maybe also create the PHP "plugin" files listed below when you have some time?

38 /var/www/plugins/ExampleSettingsPlugin/ExampleSettingsPlugin.php", F_OK) = -1 ENOENT (No such file or directory)
39 /var/www/plugins/ExampleCommand/ExampleCommand.php", F_OK) = -1 ENOENT (No such file or directory)
41 /var/www/plugins/ExampleUI/ExampleUI.php", F_OK) = -1 ENOENT (No such file or directory)
74 /var/www/plugins/CoreConsole/CoreConsole.php", F_OK) = -1 ENOENT (No such file or directory)
77 /var/www/plugins/Morpheus/Morpheus.php", F_OK) = -1 ENOENT (No such file or directory)

They would only need to extend Piwik\Plugin and have no other code in the class.

@katebutler
Copy link
Author

@tsteur Have added these files

@mattab mattab merged commit 75561ad into 3.x-dev May 28, 2019
@mattab mattab deleted the 14420 branch May 28, 2019 01:31
@mattab mattab modified the milestones: 3.11.0, 3.10.0 May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance by avoiding access() calls to non-existing files
4 participants