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

Change default for CIVICRM_CONTAINER_CACHE to simplify admin/developer experience #12426

Merged
merged 1 commit into from
Jul 6, 2018

Conversation

totten
Copy link
Member

@totten totten commented Jul 5, 2018

Overview

In discussion of cache-related PRs for Civi 5.4, there were a few reports/issues from developers observing ServiceNotFoundException. This is because there's not much awareness about how service-definitions are cached. It shouldn't be a significant issue for production systems running
vanilla code, but for admins and developers (who juggle patches/branches), it can cause confusion/support-issues/false-failures. This PR aims to reduce those.

(This is a follow-up/substitute for #12401.)

Before

  • The default value of CIVICRM_CONTAINER_CACHE is always.

After

  • The default value of CIVICRM_CONTAINER_CACHE is auto.

Technical Details

The Symfony container essentially stores a list of "services". In some Symfony-based apps, building the list of services can be time consuming, so it's common to cache this.

In Civi, this cache physically appears as files/civicrm/templates_c/CachedCiviContainer.*.php. The constant CIVICRM_CONTAINER_CACHE determines how Civi manages the cache. There are three available policies:

  • never: This means we never use the cache. You never have to worry about staleness. But it means we have to fire hook_civicrm_container on every page-request, and we go through any container-compilation tasks. This would have the fewest support-issues for devs and advanced admins.
  • always: This means we always use whatever is in the cache. It never (automatically) rebuilds the cache or checks freshness... if you make change, then you must flush the cache explicitly. This should be the fastest in production.
  • auto: This means we typically use the cache (avoiding hooks/recompilation), but it has to stat() a half-dozen files to check the modification time. (To wit: if the timestamp on Civi/Core/Container.php changes, then we discard the cache.)

Since performance is a consideration, I did some light benchmarking on my laptop (fetching a basic public Civi page 100 times across 3 threads).

https://gist.github.com/totten/ec4bffd723afb7967aec56a3040b9ca3

In these results, the never policy appears to be ~15-20ms slower than auto or always. auto is only ~2ms slower than always.

The other consideration is accuracy -- auto will usually re-compile if you make a change, but there are some edge-cases where you must still flush manually. (In particular -- when you first implement
hook_civicrm_container in a new extension, it might not be aware of the new extension. And extensions need to call $container->addResource().)

However, overall, auto is a pretty good compromise that's almost as fast you can get and works out-of-the-box for many dev/admin scenarios.

@civibot
Copy link

civibot bot commented Jul 5, 2018

(Standard links)

@totten
Copy link
Member Author

totten commented Jul 5, 2018

Since you brought this up - ping @eileenmcnaughton @spalmstr .

@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.4 July 5, 2018 22:22
@eileenmcnaughton
Copy link
Contributor

@totten I switched to the rc but it has extra commits - since this addresses changes in the rc I think it should be merged against it - if you can clean up I'm happy for it to be merged

…r experience

In discussion of cache-related PRs for Civi 5.3, there were a few
reports/issues from developers observing `ServiceNotFoundException`.  This
is because there's not much awareness about how service-definitions are
cached.  It shouldn't be a significant issue for production systems running
vanilla code, but for admins and developers (who juggle patches/branches),
it can cause confusion/support-issues/false-failures.  This PR aims to
reduce those.

(This is a follow-up/substitute for civicrm#12401.)

Before
------
* The default value of `CIVICRM_CONTAINER_CACHE` is `always`.

After
-----
* The default value of `CIVICRM_CONTAINER_CACHE` is `auto`.

Technical Details
-----------------
The Symfony container essentially stores a list of "services".  In some
Symfony-based apps, building the list of services can be time consuming, so
it's common to cache this.

In Civi, this cache physically appears as
`files/civicrm/templates_c/CachedCiviContainer.*.php`.  The constant
`CIVICRM_CONTAINER_CACHE` determines how Civi manages the cache.  There are
three available policies:

* `never`: This means we never use the cache.  You never have to worry about
  staleness.  But it means we have to fire `hook_civicrm_container` on every
  page-request, and we go through any container-compilation tasks.
  This would have the fewest support-issues for devs and advanced admins.

* `always`: This means we always use whatever is in the cache.  It never
  (automatically) rebuilds the cache or checks freshness...  if you make
  change, then you must flush the cache explicitly.  This should be the
  fastest in production.

* `auto`: This means we typically use the cache (avoiding
  hooks/recompilation), but it has to `stat()` a half-dozen files to check
  the modification time.  (To wit: if the timestamp on
  `Civi/Core/Container.php` changes, then we discard the cache.)

Since performance is a consideration, I did some light benchmarking on my
laptop (fetching a basic public Civi page 100 times across 3 threads).

https://gist.github.com/totten/ec4bffd723afb7967aec56a3040b9ca3

In these results, the `never` policy appears to be ~15-20ms slower than
`auto` or `always`. `auto` is only ~2ms slower than `always`.

The other consideration is accuracy -- `auto` will usually re-compile if you
make a change, but there are some edge-cases where you must still flush
manually.  (In particular -- when you first implement
`hook_civicrm_container` in a new extension, it might not be aware of the
new extension.  And extensions need to call `$container->addResource()`.)

However, overall, `auto` is a pretty good compromise that's almost as fast
you can get and works out-of-the-box for many dev/admin scenarios.
@totten totten force-pushed the master-container-cache branch from e64e9b1 to 5497e01 Compare July 5, 2018 22:38
@totten
Copy link
Member Author

totten commented Jul 5, 2018

@eileenmcnaughton - Sure, I've rebased to a slightly older commit (common to 5.4 and master).

@totten totten removed the master label Jul 5, 2018
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001
Copy link
Contributor

Merging as per the tag

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

Successfully merging this pull request may close these issues.

4 participants