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

Automated configuration file cache invalidation #62

Merged
merged 2 commits into from Jan 24, 2019

Conversation

Projects
None yet
4 participants
@antonkril
Copy link
Contributor

antonkril commented Jan 9, 2019

Problem

Every time a configuration file (any xml file) or Service contract is created/edited/removed in Magento, the corresponding configuration cache has to be cleared manually. This degrades developer experience, and makes invalidations more expensive as all configuration types are reloaded when config cache is cleared.

External solutions for automated cache invalidation exist:

Solution

To improve developer experience without reliance on external tools and additional setup, the approach similar to Opcache can be used:

  • When configuration is written to cache, in addition to content, also write the names of files used, and directories checked for the configuration type. Keeping information about directories is requried to invalidate cache when new files are created in modules that did not have them before.
  • Use ini.opcache.validate-timestamps and ini.opcache.revalidate-freq to define when configuration file timestamps should be checked
  • Whenever a file or directory modification time is updated, reload the corresponding configuration type

POC

POC is implemented in https://github.com/magento-architects/magento2/tree/split-framework (see https://github.com/magento-architects/magento2/blob/split-framework/lib/internal/Magento/Framework/Config/Config/Loader.php) for di.xml, webapi.xml, extension_attributes.xml, and Service Contract metadata.

Use https://github.com/magento-architects/magento-project for easy installation of a "hello world" applciation.

@kandy

kandy approved these changes Jan 9, 2019

@kandy

This comment has been minimized.

Copy link
Contributor

kandy commented Jan 9, 2019

Also make sense to implement the same approach for bundled/merged files on frontend (js, and css generated from less)

@Vinai

This comment has been minimized.

Copy link

Vinai commented Jan 9, 2019

Nice to see this in the core!

The following comments are regarding the POC implementation.

I'm wondering, is the apcu extension already required by Magento? I believe not, so that would be a new dependency, right? (iirc it is required by valet+, but not Magento)

Also, I think I remember that apcu records can't be shared between php-fpm|mod_php and php-cli, so on CLI calls (e.g. bin/magento) every cache file will be reloaded because it's timestamp is not stored in apcu and the flow would fall through to line 81, right?
I might be wrong, but given these downsides, maybe it is possible to look for an alternative storage?
Maybe it would be best to use the regular configured default cache storage. Then it's open for new storage backends that don't exist yet, and the implementation will benefit from optimizations on that layer.

EDIT: Found this comment from the original acpu author:
krakjoe/apcu#121 (comment)

@Vinai

This comment has been minimized.

Copy link

Vinai commented Jan 9, 2019

At the moment I'm unable to see how a new file for an existing key would invalidate the existing entries and cause a reload. Is that logic in the linked Loader class, too?

@antonkril

This comment has been minimized.

Copy link
Contributor Author

antonkril commented Jan 9, 2019

The branch has different POCs for different proposals. Apcu is one of them - “Configuration-less cache for simplified config infrastructure”. I plan to publish it separately. Apcu cache was used to try it out. For purposes of this proposal you can just assume any cache adapter.

@antonkril

This comment has been minimized.

Copy link
Contributor Author

antonkril commented Jan 9, 2019

New files would invalidate cache because Filesystem config reader together with used files returns empty checked directories, so their timestamps would also be checked.

@Vinai

This comment has been minimized.

Copy link

Vinai commented Jan 9, 2019

So $loadedFiles is an array of all files and also directories?
Does that also include non-existing directories that potentially include matching configuration XML files, e.g. MyComany/MyModule/etc/frontend/ even if only the etc/ directoy exists at the time in a module?

@antonkril

This comment has been minimized.

Copy link
Contributor Author

antonkril commented Jan 9, 2019

@Vinai, yes. Paths to check (both files and folders) are generated on line 71 of FileResolver. Then their timestamps are collected in Loader::loadAndCache (files that don't exist don't have a timestamp), then in Loader::getCachedContent files or folders that did not have timestamps but exist now trigger reload.

@Vinai

This comment has been minimized.

Copy link

Vinai commented Jan 10, 2019

Thanks for the explanation, @antonkril.
The $loadedFiles variable name tripped me up. Maybe it could be renamed $checkedPaths, too, like in the FileResolver, in the final implementation.

Very cool, looking forward to this being applied to all config XML files in the core!

@buskamuza buskamuza merged commit a7217af into master Jan 24, 2019

@buskamuza buskamuza deleted the automated-config-cache-invalidation branch Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment