Picture this scenario:

We move all registry style hooks (menu, theme, etc.) to separate files so that they can be skipped on 99% of page loads. Nice! Now, multiple people are using a site at the same time. Person A triggers a menu cache rebuild. However, Person B hits node/5 before person A hits his next page. Therefore, the menu rebuild happens on node/%node. No biggie, it's just an extra half-second load on that page. Except that the registry will then cache all of the *.registry.inc files as needed on all node/%node pages. Hilarity ensues.

To fix that, we introduce the concept of a cacheable and non-cacheable file in module info files. It's simply another layer in the files array, which we can already parse. We then sort out files into cacheable or non-cacheable. A file that is marked as cacheable gets cached when used just as everything does now. A file that is marked non-cacheable works exactly as it does now, except that the file will never get flagged for pre-loading by the menu_execute_active_item() function. Thus the problem described above is solved.

The attached patch does that. I've for the moment marked all .install files as non-cacheable, as that seemed logical, but it's trivial to swap files back and forth. Note that since this patch modifies the registry tables it doesn't provide another update hook, and therefore a fresh install will be required.

CommentFileSizeAuthor
registry_no_cache.patch32.48 KBCrell

Comments

chx’s picture

chx commented
Status: Needs review » Needs work

That's terrible DX -- why not just use file[no cache][] for the small minority of noncacheables?

Anonymous’s picture

Anonymous (not verified) commented

don't have a strong opinion about the implementation.

Therefore, the menu rebuild happens on node/%node. No biggie, it's just an extra half-second load on that page. Except that the registry will then cache all of the *.registry.inc files as needed on all node/%node pages. Hilarity ensues.

until the next person hits node/%, and the list of files from the registry cache will differ from that generated in registry_cache_path_files, and the newer, smaller list will get cached. so, not sure this is as big a problem as it seems at first look.

there are other potential issues with the path based cache, such as anonymous vs logged in. at the moment the cache makes no attempt to differentiate, yet a logged in user will almost certainly lead to more code, which makes the cache less effective.

more research is needed i think.

Crell’s picture

Crell commented
Status: Needs work » Needs review

@justinrandell: chx can correct me if I'm wrong, but I'm pretty sure the cache list only grows, not shrinks. At least it did when I was working on that part of it, which is why I'm concerned about the impact here. chx, does the cache shrink or just grow?

@chx: How is it bad DX? If you have both files[] = foo.inc and files[no cache][] = bar.inc, that makes the parsing considerably more complicated since the array is uneven. I don't see requiring 'cache' in there to be any worse than the stylesheets[] array in the theme info file requiring 'all' on nearly every stylesheet, to specify the "all" media type, even though that includes the vast majority of stylesheets.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

For the case at hand, I suggest just teaching registry_cache_path_file() not to cache files when a menu rebuild happened. I see no value in that cache/no cache approach.

Anonymous’s picture

Anonymous (not verified) commented

the code will correct you. here's the snippet from registry_cache_path_files:

      // Only write this to cache if the file list we are going to cache
      // is different to what we loaded earlier in the request.
      if ($files != registry_load_path_files(TRUE)) {
        $menu = menu_get_item();
        cache_set('registry:' . $menu['path'], implode(';', $files), 'cache_registry');
      }

it just checks for equality of the list of files, and caches the newer list if they're not equal.

damien tournoud’s picture

damien tournoud commented

@justinrandell: ok, today that's a non-issue. But we should rework registry_cache_path_files() to remove the query required to cache that file list, anyway. And then, we will have to teach registry_cache_path_files() not to save the file list if a menu rebuild happened.

Crell’s picture

Crell commented

Menu rebuild is only one of many possible cases where you will get many extra files just for one single page. Do we try to teach registry_cache_path_files() about all of them? Even those that may happen in contrib, like rebuilding the Views cache (which will involve loading over 10,000 lines of extra code on a typical site)?

Crell’s picture

Crell commented
Status: Needs work » Closed (duplicate)

After discussion in IRC, this will be addressed by #340723: Make modules and installation profiles only require .info.yml files.