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

Afform - improve loading performance #27783

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Oct 11, 2023

Overview

This PR makes a few big changes and several small changes to the loading of Afforms in-particular, and Angular Modules in-general, with the goal of improving efficiency and consistency.

Technical Details

  1. Caching
    Before: One SqlGroup cache is used to store Afform dependencies, but everything else about loading Angular modules (calling hook_civicrm_angularModules, scanning all extensions and directories for Afforms, dispatching 'civi.afform.get') happens at least once per page load (and is cached in a static array).
    After: One SqlGroup cache is used to store all Angular modules (including Afform dependencies). So none of the above has to happen on every page load.
  2. Permission Checks
    Before: API.Afform.get would delegate permission checks to another function... which in turn would call API.Afform.get! Amazingly this somehow avoided an infinite loop but was still pretty weird. API.Afform.checkAccess wasn't working at all.
    After: Afform permission checks are still centralized, but without the weird loop. API.Afform.checkAccess works and has a test.
  3. Dependencies:
    Before: AngularDependencyMapper relies on file scanning, so Afforms provided via hook ('civi.afform.get') are left out.
    After: All Afforms are scanned for dependencies regardless of how they're provided. The AngularDependencyMapper class is greatly simplified because the bulk of it was dedicated to file scanning and cache management. Since caching is now handled at the level of Angular modules, it's not needed in this class.
  4. Misc Cleanup
  • modified_date returned by API.Afform.get (reading from filemtime() was a by-product of the old caching mechanism in AngularDependencyMapper which I've ripped out but thought I'd keep that bit of useful info and output it from the API)

Comments

I keep being surprised every time I notice that we don't actually cache angular module info. I always assume that we do and I think the aspiration was to cache it but it just never got implemented. Lots of code acts as though a cache exists, for example, this function didn't have to change at all...

function _afform_clear() {
$container = \Civi::container();
$container->get('afform_scanner')->clear();
$container->get('angular')->clear();
}

... that was being called to clear the (nonexistent) Angular module cache every time an Afform is saved. Well, now it actually does something!

@civibot
Copy link

civibot bot commented Oct 11, 2023

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Oct 11, 2023
@colemanw colemanw changed the title Afform performance Afform - improve loading performance Oct 11, 2023
@colemanw colemanw force-pushed the afformPerformance branch 3 times, most recently from 6b7e1d0 to b600d09 Compare October 11, 2023 18:27
@totten
Copy link
Member

totten commented Oct 12, 2023

@colemanw Questions:

  1. How much does performance improve?
  2. What happens if you change a document from another tool (eg PhpStorm or git checkout)? Does it serve stale data?

I keep being surprised every time I notice that we don't actually cache angular module info. I always assume that we do and I think the aspiration was to cache it but it just never got implemented.

My recollection is that it originally aimed to cache indexes (scans/metadata) while the main data would use clean reads. In principle (aka in the theory at the time), a typical page-load should be a limited set of afforms (say, 5 afforms) -- and 5 calls to file_get_contents('foo.hml') shouldn't be very different from 5 calls to SELECT html WHERE name=foo.

But I haven't read this in a while, and obviously things change, so it needs a fresh read.

$cacheValue = ChangeSet::applyResourceFilters($this->getChangeSets(), 'partials', $this->getRawPartials($name));
$this->cache->set($cacheKey, $cacheValue);
if (!isset($this->partials[$name])) {
$this->partials[$name] = ChangeSet::applyResourceFilters($this->getChangeSets(), 'partials', $this->getRawPartials($name));
Copy link
Member

Choose a reason for hiding this comment

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

OK, cool. So it looks like it would still be reading live HTML files. It's just using $this->partials(array) instead of CRM_Utils_Cache_ArrayCache.

@colemanw
Copy link
Member Author

colemanw commented Oct 12, 2023

What happens if you change a document from another tool (eg PhpStorm or git checkout)? Does it serve stale data?

Well this only caches module metadata, not the html partials themselves if that's what you mean. But I suppose if you are a developer and decide to change angular metadata via your filesystem and then you decide not to clear caches and expect freshness... I dunno what to tell you.

How much does performance improve?

I'm not sure but I can tell you how performance improves. The bottom line is that we'll be hitting the sql cache the same amount before & after, but now that cache includes all angular metadata instead of just the afform dependency graph. This means we no longer have to call hook_civicrm_angularModules or dispatch 'civi.afform.get' (both of which involves a lot of file scanning and database hits as it generates afforms from custom groups and ECK entities).

Before: Angular modules were not cached, afform loading was slow
After: Cache previously used for afform dependencies now used for all Angular modules
@totten
Copy link
Member

totten commented Nov 6, 2023

I did some light r-run and confirmed that the extra caching is focused on metadata. The layout files are only cached to the same extent as before. 👍 (i.e. in debug mode, you effectively get the latest version the layout file; without debug mode, you get the cached bundles).

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.

2 participants