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
Comment | File | Size | Author |
---|---|---|---|
#5 | 707098-5_improved_performance.patch | 7.02 KB | alex_b |
#1 | feeds.module.nodeapi.patch | 5.63 KB | pingers |
feeds.module.nodeapi.patch | 5.62 KB | pingers | |
Comments
Comment #1
pingers CreditAttribution: pingers commentedMissed the 'presave' condition... please use this one.
Comment #2
drewish CreditAttribution: drewish commentedWhy not avoid some indenting and do something like:
Comment #3
alex_b CreditAttribution: alex_b commented+1 for #2
BTW, I'd love to address the overall performance hit by DB caching the result of feeds_get_importer_id():
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.
Comment #4
pingers CreditAttribution: pingers commentedTotally 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$
Comment #5
alex_b CreditAttribution: alex_b commentedThis 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?
Comment #6
alex_b CreditAttribution: alex_b commented#5 is committed now.
Comment #7
pingers CreditAttribution: pingers commentedAh, sorry I missed this - nicely done!
I'm sure I'll reap the benefits soon enough :)