I noticed a number of queries executing when viewing a node.
This was proportional to the number of feed node types defined (+1 for all)

E.g. Devel query output
0.2 1 ctools_export_load_object SELECT * FROM feeds_importer
0.21 1 ctools_export_load_object SELECT * FROM feeds_importer WHERE id = 'feed'
0.21 1 ctools_export_load_object SELECT * FROM feeds_importer WHERE id = 'project_feed'
0.16 1 ctools_export_load_object SELECT * FROM feeds_importer WHERE id = 'project_twitter'

So I took a look into hook_nodeapi() and discovered the problem lies here.

The patch removes the need to check for the current node being a valid feed type (the queries above), when there isn't actually any work to be performed. I.e. it's only a load $op. The patch stops all of these queries executing unnecessarily. Please see the attached patch.

Cheers, pingers

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pingers’s picture

FileSize
5.63 KB

Missed the 'presave' condition... please use this one.

drewish’s picture

Why not avoid some indenting and do something like:

  if (!in_array($op, array('validate', 'presave', 'insert', 'update', 'delete'))) {
    return NULL;
  }
alex_b’s picture

+1 for #2

BTW, I'd love to address the overall performance hit by DB caching the result of feeds_get_importer_id():

 * @todo speed this up by DB caching the result.
 */
function feeds_get_importer_id($content_type) {

I know that the proposed solution here has still its merits - but only until we don't have a 'view' operation in feeds_nodeapi() - and that may happen soon.

Up for adding caching to feeds_get_importer_id() ? I'd accept a patch without caching but I'll have to warn that it won't be a permanent solution.

pingers’s picture

Totally correct there - I just didn't have time in this case to flesh out proper caching, just noticed looking at devel query log this was an easy fix.
I might get a chance this weekend to start a real solution.

Cheers, Pinger$

alex_b’s picture

Title: Performance of nodeapi » Performance of nodeapi and access checks
Version: 6.x-1.0-alpha10 » 6.x-1.x-dev
Priority: Minor » Normal
Status: Needs work » Needs review
FileSize
7.02 KB

This is an attack on the incredibly squanderous feeds_get_importer_id() and feeds_access().

1. Introduce helper _feeds_importer_digest() that keeps cached abridged information about enabled importers (currently only id and content type settings, but that may change).
2. Add feeds_enabled_importers() that uses _feeds_importer_digest() and returns all enabled ids.
3. Modify feeds_get_importer_id() to use _feeds_importer_digest().
4. Add feeds_cache_clear() that clears all relevant caches when modifying importers.
5. Call feeds_cache_clear() from relevant places in Feeds.
6. Use feeds_enabled_importers() in feeds_access() instead of feeds_importer_load_all() (!)
7. Use feeds_enabled_importers() in feeds_defaults_node_info().
8. Fixes insufficient cache clearing when creating or deleting importers.

Patch passes all tests.

pingers: This should address all superfluous queries described in your initial report. Care to review?

alex_b’s picture

Title: Performance of nodeapi and access checks » Improve performance of nodeapi and access checks
Status: Needs review » Fixed

#5 is committed now.

pingers’s picture

Ah, sorry I missed this - nicely done!

I'm sure I'll reap the benefits soon enough :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.