Scan: Cache results

hmm... let's say modules that need a library need to implement hook_libraries_info(), maybe something like this:

function wysiwyg_libraries_info() {
  return array(
    'fckeditor',
    'tinymce',
  );
}

Now, libraries API knows what it will be requested to find at one time or another. For example, Wysiwyg API when working to set up the node edit form, or whatever else form, could ask for the path to 'tinymce' like this:

$path = libraries_get_path('tinymce');

Here, Libraries API first would check the cached repository. If a match is not found, then a look up against the file system is triggered (this look up would search for the library in sites/all, sites/domain, etc.). If that scan fails, libraries_get_path() returns FALSE. If that scan succeeds, it caches the result and returns the path.

If a match is found in the libraries cache, it could try a simple file_exists() to make sure the library is still there. If not, then the cached information would be removed, and FALSE would be returned. If a match is found in the cache and confirmed by file_exists(), then the path would be returned.

With this method, every invocation to libraries_get_path() would mean a minimum request to the file system (file_exists()), a scan against sites/all/thing, sites/domain/thing, at a maximum.

libraries_get_path() could also have a second argument so that the caller can bypass file_exists() confirmation. Or the other way around, by default file_exists() confirmation is not performed when a match is found in the cache. The file system look up could only be forced by the calling module on the admin side or similar.

Finally, this needs some kind of garbage collector to clear cached stuff that will never be requested again, and that is not present on the file system. Maybe here the libraries API could detect when a module is disabled, and if it was implementing hook_libraries_info(), then try to clean the related libraries. Or maybe using a cron task that executed once a day or so.

Not sure if I'm missing something. :-|

hmm... it seems to me that this approach complicates things too much.

The common problem that needs to be solved, I think, is that modules need to know the location of certain libraries. Users need to know where they can install libraries required by certain modules in a directory that's independent from the module location.

For example, module_a needs library_a:

1) User can install library_a at sites/all/libraries/library_a, sites/domain.1/libraries/library_a, sites/domain.2/libraries/library_a, and so on...

That's easy to explain, easy to manage for the user. And it's similar in concept on how module/theme locations work.

2) All module_a needs to do is libraries_get_path('library_a'), and if it will get the location of library_a the user wants to have enabled for the current site. ie. if running on domain.1, it will get "sites/domain.1/libraries/library_a". if running on domain.2, it will get "sites/domain.2/libraries/library_a". if running on domain.3, it will get "sites/all/libraries/library_a".

3) Libraries don't need .info files. Libraries cannot be nested in subdirectories. Only at "sites/(all|domain)/libraries/library_a".

Isn't it simple and nice?

If that's not enough, then I think further information (and methods) attached to each library will vary too much from one library to another, so that could be implemented by each separate module, independently.

If time and experience is able to tell more common requirements related to library management, that could be added at any time. But now, it seems to me quite complex to accomplish, because we don't know what other libraries/modules need.

For the record: Here's a good reason to keep 3rd party libraries off the modules directories: #546584: Modules or themes with too many files kill drupal_system_listing performances.

This one is based off of #719896: Add a hook_libraries_info().
It puts a cache_set() in the detection and a cache_get() in the loading. That's all. I didn't roll a patch, because that would have included all of #719896, so here's the relevant code.

/**
 * Detect libraries and library versions.
 */
function libraries_detect($libraries) {
  foreach ($libraries as $name => $library) {
    libraries_detect_library($libraries[$name]);
    cache_set('libraries_' . $name, $library);
  }
  return $libraries;
}

/**
 * Loads a library.
 */
function libraries_load($library, $variant = NULL) {
  $library = cache_get('libraries_' . $library);
  if (empty($library)) {
    $library = libraries_info($library);
    libraries_detect_library($library);
  }
  libraries_load_files($library, $variant);
}

I've been reading this thread. Is it going to be module developers' jobs to provide ways to flush the library caches? Just checking because this code provides caching and getting from the cache, but not a way to clear it.

First and foremost, I noticed while testing, that there are some bugs in the code in #6. I didn't roll a (new) patch yet, because I'm juggling quite some Libraries API patches currently and didn't have the time to sort it all out.

Re #7:
Well, the cache is (currently) stored in the normal cache table, so every time you hit the 'Clear caches' button on the performance page, your libraries cache will be deleted. Also once there is a UI for Libraries, the plan is to delete the libraries cache there and do a rescan.

Makes sense.

Yeah, there are quite a few patches floating around huh.

What needs to be changed in #6? If I have time before you maybe I can give it a shot.

Kevin

I think it was a serialization issue. I didn't dig too deep but for some reason with the above code, in libraries_load() the cache_get would return an object instead of an array. If I remember correctly, I somehow got it to work, though.
I bet sun will have some expert advise on this, as well as the general approach.

This one works. And passes tests.

Details:
- A cache_set in libraries_detect() [not in libraries_detect_library(), which is what we call in libraries_load()]
- A cache_get in libraries_load. On a miss, we call libraries_detect_library (see above).

Notes:
I think it doesn't make much sense, that in order to detect all libraries you currently have to do:

$libraries = libraries_info();
libraries_detect($libraries);

I think we should call libraries_info() inside of libraries_detect() (just like we do libraries_info($library) in libraries_load()), so that the above would just be

libraries_detect();

Great, I forgot to mention the most important thing :)

Currently we don't call libraries_detect() anywhere, so that the libraries don't get cached currently until you call libraries_detect() currently.
Thinking about this, that doesn't make much sense. Alternative patch for consideration, which does the cache_set in libraries_detect_library().

Thanks for the quick review.

I think that only the load operation should try to retrieve from cache, or otherwise detect + cache - i.e., let's not squeeze caching functionality into an otherwise clean API function to detect a library.

That's what the patch in #11 does. But that leaves us with:

why would I ever want to call libraries_detect() ?

Note that this is not changed in this patch. We need to think of a code-flow that makes more sense here. Maybe, in libraries_load, if we miss the cache, just do a libraries_detect()? It's really not THAT expensive (compared to other caches), esp. because the number of libraries will be <100 for 99.99% of sites (made-up number), and that would keep the API function libraries_detect_library() clean. It doesn't really feel right, though...

This will only set or update the cache in case the library is installed. All other early-out returns in this function won't cache the known results.

I thought this was a good thing. Early-out returns means, that the library could not be properly detected, so either libraries_load() will fail, or it will load the wrong files. This is of course related to #864376: Loading of external libraries, where we (currently) rely on loading of 'error-libraries', but that's a different thing to sort out.

New patch. We update the cache both when calling libraries_detect() (for instance on /admin/reports/libraries (soon...) or maybe on module enable?) and on a missed cache on libraries_load(). libraries_detect() being more or less useless for now, I think, is something we'll have to fix or think about, when we've got a couple more issues sorted out (UI for one). We can always remove it or re-utilize it later.
Also multiple cache get and set functions are definitely useful, but again, I think for now this is all we can do, without overcomplicating the current system.

A single cache entry for all libraries would be better than the latest patch. You store tiny cache entries and you will create SQL requests for each one of them.

In order to provide the right cache clear, you should clear it a flush_all_caches() time (which will happens quite often, module enable, disable, etc).

Libraries has to be scaned only when developing, but as soon as Drupal enters in production mode (all caches activated) then no scan should ever happen, and as many cache fetch as you have libraries is really too much (it's I/O each time, it probably be even worst than scanning the FS if you have a fast harddrive).

When you don't find a library, you can mark it in the current cached structure as "non existing" and update the cache. It'll avoid later rescan, you could keep only one cache entry for all the libraries. This cached entry will be updated on new library scan or at cache clear time, which seems quite easy to implement (it doesn't imply any complex algorithm).

If users upload new libraries, you have to rescan for them, but it needs a user action that is the download itself, so why not letting him press the "cache clear" button (or another "library cache clear" more specific button)?

Whatever happens, if you install 3 or 4 libraries, it's in order to use them. Their metadata has a neglictable impact on memory, on the opposite to something like, menu cached entries or theme registry. I really think that making one cache entry per library is an error. If you have ten library are you really willing to make 10 SQL request, will the latency it produces (even worst if in a distant memcache or such)? It's all about finding a balance between latency and memory usage, and in this case, I think latency wins, since you cache almost nothing except some pathes.
That's probably why element_info() is not cached (except static cache), and such. It's used tons of time in the same page, but there isn't any bin for it, because the time needed to build the info array is probably smaller than causing latency using a distant storage. Same thing with libraries, not exactly the same problem, but as the data you store represent almost nothing in term of memory usage, it may worth the shot of doing only one cache entry.

It's only an opinion and I let you do your choices, it's not my decision to make, nevertheless when core gives you a direction, it doesn't mean you cant discuss it for such edge cases.

I now, I do deal with everyday. But it also suffers from too many SQL queries. Sometimes core alone just to display a content listing can go up to 200 requests, which is way too much. You have two problems, but dealing with one by making the second worst is not really an option. All is balance, and in this case memory consuption is almost nothing.

But I kind like the fact that some libraries could have a different caching policy, this is a good idea, but libraries such as WYSIWYG remains edge cases.

EDIT: Putting in a place a solid caching policy is engeneering, but caching almost anything everywhere is often over-engeneering. I would advise to you to use a compromise here, meaning cache only the existence or not of libraries in the cache array (with their original path), rebuild the rest at runtime, it would ensure only one really small cache entry (and avoid more than one I/O for fetching back data from a distant storage), and it event may be faster than caching anything.

It could lead to something like ((average_path_length + average_name_length) * sizeof(char)) memory consuption, which means even with 50 libraries (arbitrary number, a lot), let's say with 90 char average path length (written "/var/www/my_maginificient_site/www/sites/all/libraries/flash/superb_library/real_library" to compute this, as example) and 16 average name length would cost only 5Kb (some Views from the Views module often consume up to 10M which is something like 2000 times more. On my box with a simple site, core and only 5 modules (sample is my blog) Drupal consumes something like 25M per page, which is 5000 times my worst case scenario here. But, making 50 SQL requests over 300 shows here a ratio of 1/6 which is non neglictable.

Re-Edit: 5kb is nothing, and it's more likely that MySQL buffers, PDO buffers, considering the known memory leaks in PDO in some PHP versions, objects spawn in order to retrieve cache, database query objects from the database layer, and other environmental stuff will eat may be 100 times more than this.

I apologize I was comparing the code with the wrong branch. 2.x seems indeed to store a lot more of data than legacy 1.x

Reroll.

I think, if this passes tests, we should commit this as is, because the master branch is currently unusable for any serious site because it has to reprocess all information on every page load. I agree that there might be better caching strategies and we can advance on those in the future, but this really seems like a good first step.

Yes, it's late :) ...

Maybe something more along these lines...

Uploading fixed patch.
Will commit this as soon as the previous one comes back green.

Now with less PHP errors. Guess I better wait for this one to come back...

Oh boy.... Sorry for the spam. Now for this one I actually went through the trouble of setting up a dev site, so I'm pretty sure there's no harm in this one.
I'll wait for it to come back green and then commit this one.

Marking fixed. http://drupal.org/commitlog/commit/10030/0793cd8444a0d0bf5cfac93cc85c25d...
If you want to leave this issue open for more advanced caching strategies, help yourself.

http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

Hmmm.... I don't think we need to provide dedicated cache flushing functionality, but at least drush cc should work:

/**
 * Implements hook_flush_caches.
 */
function libraries_flush_caches() {
  return array('cache_libraries');
}

Marking needs work.

For clarity, I meant:
I don't think we need to provide dedicated cache flushing functionality yet...

I tested this patch (Drush ftw) and it works. If it passes tests, I'll commit this. I like to place hook invokations (this is our first one...) at the top of the module file, but I also don't really care, so if you feel differently please reopen.

http://drupal.org/commitlog/commit/10030/b259580bd1fb0348960bfbe65e0f963...