backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Enable Static Caching on system.date.json #5800

Closed quicksketch closed 11 months ago

quicksketch commented 2 years ago

Description of the bug

While performance testing date output in #5788, I found that the "system.date.json" file is parsed and read from disk twice every time a date field is displayed. On a page with 1500 dates, this means reading system.date.json from disk 3000 times.

Steps To Reproduce

To reproduce the behavior:

  1. Add a date field to any content type.
  2. Create several sample pieces of content.
  3. Make a view that lists the date fields.
  4. Either using a debugger or with a debug statement like dpm() in ConfigFileStorage::read() (line 1305 of config.inc):
    if ($name === 'system.date') {
    dpm('Read system.date.json');
    }
  5. Check how many times system.date.json is read.

Actual behavior

File is read from disk twice per date field.

Expected behavior

File should only be read once per page load.

Additional information

Add any other information that could help, such as:

quicksketch commented 2 years ago

Below is XHProf profiling of the same page, before and after enabling the static memory cache on the system.date.json file:

image

Basically, reading config from disk takes up more than 10% of the total request time on this page. This may be an exaggerated use-case, but there's no need at all to read the config multiple times.

I filed a PR at https://github.com/backdrop/backdrop/pull/4214 that enables static caching.

Long-term, I think we should set some better defaults for config files. This "_config_static" property is not well documented and pretty much invisible to new developers.

indigoxela commented 2 years ago

This "_config_static" property is not well documented and pretty much invisible to new developers.

Not only new ones. :wink: Do we have an issue for that documentation, yet? Documentation for that property is not only "not well", but simply nonexistent.

indigoxela commented 2 years ago

This is how I tested:

... then some head scratching... I hacked core/includes/config.inc and added some "debug" code:

    if ($cache) {
      $loaded_configs[$type][$config_file] = $config;
    }
    elseif ($config_file == 'system.date') {
      // The debug hack:
      print 'x';
    }

Wow, for 100 date fields I got 305 "x".

Performance-wise the static cache isn't noticeable with only 100 nodes (checked in browser dev tools). So I added 400 more. With 500 nodes (and dates), the performance difference gets noticeable. It might be more significant if the disks are slow (on an older or very busy server for example).

Reading the same config file again and again for one page call really makes no sense, so this PR works for me. :+1:

bugfolder commented 1 year ago

Went to review code, and noticed that there are conflicts and unresolved queries in the PR. @quicksketch, can you rebase?

quicksketch commented 11 months ago

Thanks @bugfolder, rebased and resolved conflicts. https://github.com/backdrop/backdrop/pull/4214 is ready for review again.

klonos commented 11 months ago

PHPCS complaining about irrelevant arrays/lines exceeding 80 characters aside, the code LGTM 👍🏼

quicksketch commented 11 months ago

Thanks @klonos, @indigoxela, and @bugfolder! I merged https://github.com/backdrop/backdrop/pull/4214 into 1.x and 1.26.x.