Load multiple nodes and terms at once

Here's a new patch, again, completely untested.

Quick summary of what it will do, a lot of this isn't in the patch yet:

We add a new function - node_load_multiple($nids = array(), $options = array(), $reset = FALSE)

This lets me do node_load_multiple(array(1)) or node_load_multiple(NULL, array('article')), or node_load_multiple($lots_of_nids, array('article')) - and get back an array of node objects matching the nids, the conditions on the {node} table or a combination of the two.

The node objects are fetched in a single query and put into an array. This is then divided by type, and passed to hook_load(). The result of that is put into an array, and passed to hook_nodeapi_load(). This means that all the hooks operate on a single array of node objects - which means they can also fetch their data in a single query and pass it back to node_load.

So instead of $queries = $number_of_nodes * $hook_implementations we now have $queries = 1 + $hook_implementations in the vast majority of cases.

As suggested on irc by pwolanin, node_load() becomes a wrapper around node_load_multiple(), which takes an nid, gets a node object back, and nothing else.

revisions will move to node_revision_load() since having them in the same function is just messy and they aren't statically cached anyway.

Another revision, still untested.

Changes since the last patch:
1. We now unset $nodes from the cached array if they don't match the conditions in $options.
2. Nodes arrayed by type are passed into the node specific hook_load() (which needs a new name) then merged back before being passed to hook_nodeapi_load()
3. node_load is now a wrapper around the new function, and only accepts $nid and $reset as params.

And some tidy up.

TODOs:
1. Pass the $typed_nodes and $nodes arrays to their respective hook implementations properly, so those functions can act on the entire array in one go.
2. Make it work and write/fix some tests.
3. Add node_load_revisions() to replace that functionality which has been removed from node_load() and node_load_multiple().
4. Convert hook_nodeapi_load() and hook_load() implementations in core to the new API.
5. Fix any calls to node_load which aren't a simple node_load($nid).

I'm fairly happy with how this is shaping up, although I've got a feeling some of the array treatments could be optimized a lot (I'm using array_diff then array_merge when I really just want to check the keys for example). If anyone's got time to have a high level look that'd be great (thanks to those who've already done so in irc). When I've done 1 & 2 I'm going to abuse 'code needs review' to get some eyes on it.

node_load() works at least on the front page and node/1

Reworked the hooks. Still need tests, converting all the hook implementations (hence many notices), node_revision_load().

However, I'd like confirmation from a few people that the new API for node_load() node_load_multiple(), hook_node_type_load() (which isn't really a hook but lets' not deal with that in this patch) and hook_nodeapi_load() makes sense before going much further with conversion, so while this is nowhere near ready yet, it needs review.

Some cleanup following discussion with Damien and pwolanin in irc, a lot of these changes direct from Damien via pastebin.

My first introduction to the node_hook() function.

Still a few miscellaneous patch failures, but hook implementations are converted (some optimised, some not).

Couple more.

Pretty much the only fail I get now is file.test, which ought to be unaffected by this patch...

All core tests pass.

The existing tests test hook_load, hook_nodeapi_load, and some of the arguments to node_multiple_load() - so it needs an extra test or two for conditions, then I think we're good. Really, really needs some thorough review now though.

Note I've not converted the node front page etc. to use the new function yet either.

A few revisions - here's a summary of the patch at it's current state:

Adds the function
node_multiple_load($nids = array(), $conditions = array(), $revision = NULL, $reset = FALSE)

This returns an array of node objects - the core node tables are queried in one query. Conditions are ANDed together and can be combined with the $nids argument to return a subset. I've added some simpletests for the new function, existing tests cover node_load() itself.

Node load is simplified, and is primarily wrapper around node_multiple_load().
node_load($nid = NULL, $revision = NULL, $reset = FALSE)
This should be used whenever loading a single node or revision by the ID, otherwise use node_multiple_load directly.

hook_nodeapi_load() now takes an array of $nodes. book_node_api_load() and comment_nodeapi_load() have been optimised to use one query to load their information into the node objects. Some others like upload_nodeapi_load() and taxonomy_node_api_load() do a foreach on $nodes - this is because I'll have to write a proper multiple load function for taxonomy terms and maybe other stuff to be able to utilise them properly for this new API. This is already a significant enough change that I think we should do those once it gets in - I'd like to do a taxonomy_term_multiple_load() at least.

A lot of tests use $node = node_load(array('title' => $title)) as a way to fetch back a node object after form conditions. This particular pattern is pretty much only found in tests, and in real life you never know how many nodes you might get back with the same title, so I've added a small helper method to the web test case - drupalGetNodeByTitle() to replace this for tests only. In normal use you'd use node_multiple_load(NULL, array('title' => $title)) then deal with the array you get back.

I've also changed the default /node page and taxonomy/term pages to use node_multiple_load(). With 10 nodes on a page, each with comments enabled, we'd be looking at a reduction of 18 queries per page load. When we have multiple object loading functions for taxonomy and contrib modules it could easily knock 100 queries off the default front page.

I've had to use the query builder in the nodeapi_load hooks because you can't use IN() for static queries. See #314464: Convert db_placeholders() to DBTNG.

In answer to Dries, one thing that was suggested in irc when I brought this patch up, and which might help ease the transition for upgrades and new developers, would be to do something along these lines:

// Run all hook_nodeapi_load() much the same as now.
foreach ($nodes as $node) {
  node_invoke_nodeapi($node);
  $nodes[$node->nid] = $node;
}

// Run hook_nodeapi_multiple_load() for modules which can take advantage of the performance gains as in the current patch.

foreach (module_implements('nodeapi_multiple_load') as $module) {
  $function = $module . '_nodeapi_load';
  call_user_func($function, &$nodes,  array_keys($typed_nodes));
}

That'd mean if your function is always going to be checking things against every individual node anyway (say poll_load()) then you just use hook_nodeapi_load(), but if you can take advantage of loading information at once, you use hook_nodeapi_multiple_load() - you only have to deal with the extra complexity (which is only getting and passing back an array of objects instead of a single object) if you want the extra benefits.

This would mirror the simplified and 90% backwards compatible node_load() versus node_multiple_load() too.

Rough performance indications from alpritt on #301902: Allow more users to see the node admin page (paraphrased for clarity):

I applied the multiple_node_load patch as a proof of concept here, and checked times with the devel module:
1. With 50 nodes, 62ms and with multiple_node_load around 42ms.
2. Loaded up with taxonomy terms, 174ms and 142ms with the addition of multi load.

That's a 20-30% improvement in query execution times. I'm planning to do the same thing for taxonomy terms as well where IMO it's an even better fit than node_load.

Marked #329454: Load multiple terms at once as duplicate since this is a lot easier to keep track of in one patch.

Taxonomy terms are now loaded for all nodes with two queries (one to get the tids from the nodes, one to load the full term objects). In HEAD this is currently done with a custom query that doesn't even touch taxonomy_term_load() - there's a lot of that in taxonomy module which I'll want to go through later in followup patches.

hook_taxonomy_term_load() is modified to follow the same pattern as hook_nodeapi_load().

All the directly relevant tests pass except forum, which has something weird going on with error 500 responses - I've manually verified that forum module is working though. Since both comment and taxonomy are optimised for multiple loading now, we should get a fair bit of difference in performance, it ought to be worth benchmarking now.

Minor cleanup, haven't looked at the shockingly bad performance yet. All tests pass.

Did my own benchmarks on the latest patch with 5,000 terms, 5,000 nodes - 2,500 nodes have taxonomy terms and comments, 2,500 don't (upgraded from d6) - benchmarked on my desktop with query_cache off and no APC. Only default profile modules enabled.

ab -c1 -n500 http://d7.7/node

With this I got a 10% improvement.

devel says 61 queries per page instead of 81. About 20% reduction in query execution times.

Similar results for taxonomy/term/1

All the multiple load queries are between 0.3 to 0.8 milliseconds - comparable to the equivalent single load queries, except of course they're run once per page (or at least per call to node_multiple_load()) instead of once per node.

I got about a 1% performance penalty on node/1 - this is could be due to dynamic queries, IN() instead of = in the SQL, more logic in the hook implementations, but it's only 1% and we gain much more elsewhere.

Attached the output and some of the notes from devel output as a .txt

Based on this I'm marking back to needs review.

Spoke to kbahey in irc and got a bit closer to his dataset for benchmarking, 30 nodes on front page, all core modules enabled.

/node

devel says:
before patch: 270-5 queries in 186.8-220 millisecond

after patch: 172 queries in 117.54 - 150.45 milliseconds

ab says:

before patch

Document Path:          /node
Document Length:        46992 bytes
Time taken for tests:   270.36141 seconds
Requests per second:    1.85 [#/sec] (mean)
Transfer rate:          86.03 [Kbytes/sec] received

after patch:

Document Path:          /node
Document Length:        46992 bytes
Time taken for tests:   227.417881 seconds
Requests per second:    2.20 [#/sec] (mean)
Transfer rate:          102.15 [Kbytes/sec] received

/node/1

before patch:

Time taken for tests:   146.582315 seconds
Requests per second:    3.41 [#/sec] (mean)
Time per request:       293.165 [ms] (mean)
Time per request:       293.165 [ms] (mean, across all concurrent requests)
Transfer rate:          28.30 [Kbytes/sec] received

after patch:

Time taken for tests:   176.714281 seconds
Requests per second:    2.83 [#/sec] (mean)
Time per request:       353.429 [ms] (mean)
Time per request:       353.429 [ms] (mean, across all concurrent requests)
Transfer rate:          23.47 [Kbytes/sec] received

So, loading one node is a lot more expensive when firing more hooks. I'll look into fixing things up on that front.

I'm thinking at least make it = for a single node, will try with OR and do a comparison - do you happen to know what the limit is? Since we have the query builder, we could even use both where appropriate.

Found it! Wasn't using the static cache properly, so hook implementations were getting called multiple times for every call to node_load() even though the node table fields themselves were cached.

node/1 is now within margin of error (it came out a couple of ticks faster in my benchmarks).

node/ is 15% faster.

node/1
Before patch:

Time taken for tests:   131.771640 seconds
Requests per second:    3.79 [#/sec] (mean)
Time per request:       263.543 [ms] (mean)
Transfer rate:          31.48 [Kbytes/sec] received

After patch:

Time taken for tests:   128.562064 seconds
Requests per second:    3.89 [#/sec] (mean)
Time per request:       257.124 [ms] (mean)
Transfer rate:          32.26 [Kbytes/sec] received

/node
Before patch:

Concurrency Level:      1
Time taken for tests:   238.355726 seconds
Requests per second:    2.10 [#/sec] (mean)
Time per request:       476.711 [ms] (mean)
Transfer rate:          97.46 [Kbytes/sec] received

After patch:

Concurrency Level:      1
Time taken for tests:   207.189637 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      23789000 bytes
HTML transferred:       23496000 bytes
Requests per second:    2.41 [#/sec] (mean)
Time per request:       414.379 [ms] (mean)
Time per request:       414.379 [ms] (mean, across all concurrent requests)
Transfer rate:          112.12 [Kbytes/sec] received

Found another optimisation with the static cache, also added some additional code comments.

/node with 30 nodes enabled and all core modules is now

without patch:
308 queries

with patch:
206 queries.

chx reviewed it on #drupal-dev, patched as we went. (edit, crossposted with kbahey, no major changes, so above benchmarks should stand).

I've gone through, simplified some of the code, added a bunch of code comments, and a new test for multiple taxonomy term loading. All tests pass. This should be the last revision for a while until someone else rips it apart :)

Summary of the API changes again:

node_load() is simplified to be just:

node_load($nid = NULL, $vid = NULL, $reset = FALSE)
This should only be used for loading an individual node or revision when you already have the id.

Then there's a new function for loading multiple nodes at once: node_multiple_load($nids = array(), $conditions = array(), $revision = NULL, $reset = FALSE)
This is used in the following ways:

$nids = array(1, 2, 3, 4);
node_multiple_load($nids);

get back an array of four fully populated node objects, indexed by nid.

$conditions = array('type' => 'article', 'promote' => 1);
node_multiple_load(NULL, $conditions);

get back an array of all nodes of type article and promoted to the front page.

node_multiple_load($nids, $conditions);

Get back all the nodes in $nids which are of type 'article' and promoted to the front page.

If you call node_load and node_multiple_load more than once on the same page request, it'll use a single static cache to meet all nids and conditions wherever possible - see the code comments for more explanation, or try it out with the devel query logger.

hook_nodeapi_load() now takes an array of node objects and acts on them by reference (worst case is you do a foreach over them). hook_taxonomy_term_load() uses the same pattern.

A similar function has been added for taxonomy - taxonomy_term_multiple_load($tids = array(), $conditions = array(), $reset = FALSE)
Again, this is a wrapper around a newly simplified taxonomy_term_load($tid, $reset = FALSE)

Will take a look at node_access(). Although having said that, there's a lot of places that can be optimised after this patch gets in - most taxonomy functions which do their own custom queries of the term data table will be able to utilise the new API for example, but I'd like to keep this to the minimum size for having it working, testable and benchmarkable - then followup quickly later with applying it to other places.

Here's a re-roll which fixes the phpdoc spacing and renames $revision to $vid. Thanks cwgordon :)

I did have a quick look, and I agree node_access doesn't need to be touched by this patch.

On the assumption that #314464: Convert db_placeholders() to DBTNG will get resolved, I've used db_placeholders() instead of db_select() for the IN() queries where there's otherwise no need for a dynamic query.

Still 15% faster on a standard front page.

objects are passed by reference all the time in PHP5, so this one's a little more readable.

re-roll based on initial feedback from webchick. Hopefully this caught everything.

Found a small optimisation in node_multiple_load(): If some nodes have been loaded from cache, and some haven't, then we don't need to re-run hooks on the already cached nodes. This also simplifies the function a little bit. removes a couple of unnecessary/awkward checks etc. While we don't have any hook_taxonomy_term_load() implementations in core yet, the same is true there.

Worked on this a little with cwgordon in irc - we came up with a way to get rid of one of the foreaches :)

There's no measurable performance impact in either direction compared to the foreach, but it's prettier.

edit: it also causes tests to fail. Leaving this one for tonight.

Here it is with array_flip, array_intersect_key, array_diff_key and array_intersect_assoc instead of the three foreach loops. Tests pass this time. Seems like a lot of work to avoid a foreach loop. ;) No measurable performance difference on node/1 as far as I can tell - which calls node_load() nine times on a single nid.

chx: reviewed this on irc, and said "let's keep the foreach if that's the only way to do it" on the first one, although removing the nested foreach stays (and is much nicer)

comment_nodeapi_load hunk no longer applied after dbtng conversion.

recidive - if we keep a singular hook_nodeapi_load(), I think that'd make it very unclear which one you're supposed to use (since either could work) - I actually suggested it on #drupal, and the idea was shot down... Also just to make it 100% clear, node_load calls node_multiple_load() to get data now - saves a lot of code duplication, and allows them to share the same static cache.

Was thinking about hook_query_alter() to add joins, but I'd be concerned that a single module could alter in a badly indexed join and slow everything down to a crawl. If we could get it to work, would be a good thing to try out in a followup patch though.

My main hope for further performance improvements is - #333171: Optimize the field cache - which will give us everything in a single query if the cache is populated.

Re-rolled for taxonomy.test commit.

Re-roll for book dbtng conversion.

That's a few independent benchmarks showing a significant improvement, and there's been no negative feedback on the approach taken in the patch for a while plus several positive reviews - so anyone want to bump this to RTBC (or set it to needs work if we've missed something)?

Thanks Damien.

* All implementations now use both params for consistency - we should probably look into doing this elsewhere in core since it's a common pattern to skip unneeded paramaters which I copied.
* conditions/cache - makes sense - I'll make sure this is added to the API docs after commit.
* Fixed hook_nodeapi_forum()
* array_diff_assoc() works, and its more concise that way too. Lovely.
* Changed all assertions to use @count - this helps to work out what the problem was when you break it.
* node_page_default - fixed the query to use fetchCol(). Damz also mentioned using $nodes instead of $nids for the $output concatenation in case any node isn't loaded from the database. We can't do this because $nodes isn't necessarily keyed in the same order as $nids, but I added a comment to this effect and a sanity check there.
* poll_loa(d) - fetchAllKeyed() only covers two column result sets, so doesn't look possible to use it here. Code is also copied direct from the current function and only converted to dbtng 'cos I was moving lines around, so I'd rather leave it as is for the purposes of this patch.
Removed the pointless isset though and fixed indentation.
* drupalGetNodeByTitle() Well spotted! Added an array_reverse().

I think this covers everything either in the new patch or in the answers. Ran all tests and they continue to pass.

two more via Damien in irc:

poll_load now uses fetchAllAssoc('chid', PDO::FETCH_ASSOC) (edit: this is currently only in a comment on the dbtng docs - could do with rolling in)

drupalGetNodeByTitle uses reset() instead of stupidity.

Using array_diff_assoc() lets us remove one additional line of code there. On PHP type hinting, I'm all for being stricter, but I don't think this patch is the place to introduce it.

missing parenthesis. There's an issue for adding type hinting to every param over at #318016: Type hint array parameters - so I'm coming down firmly on the side of letting that issue handle it consistently across core. Also, while they didn't post any patches, much of the nastiness in previous patches was removed by Damien in irc (and chx and others), so they need crediting on commit for this one.

Went through this one more time with Damien. Some recent changes from node loading hadn't made it into taxonomy terms + a little bit more dbtng goodness. Also added additional phpdoc to node_multiple_load() and taxonomy_term_multiple_load(). That's it for tonight, and hopefully there's nothing much left to nitpick now.

@kika I think its good to have _load() as a consistent suffix for loading core objects - taxonomy_get_term() was renamed to taxonomy_term_load() in D7 due to menu system requirements - and is extended with taxonomy_term_multiple_load() in this patch too. It also matches hook_nodeapi_load() and hook_load() (again, those suffixes recently introduced when hook_nodeapi was de-opped). Does that help explain the thinking behind it?

On next steps, - just a note that I made a start on the caching already at #333171: Optimize the field cache - which would mean a single query to load node objects on the front page even with CCK and other nodeapi modules enabled once the cache is primed. It would also give us gains on individual node pages, outweighing the very small overhead introduced here.

On Damien's point about the extra taxonomy query on individual node views in #64, that raises an important point which I don't think I've written up yet anywhere about how taxonomy terms are currently loaded. I've started a new issue to answer it rather than making this one any longer #337295: Convert the rest of taxonomy module to use multiple loading - obviously I'd rather leave conversion to a separate issue since this is already approaching a 50k patch.

Also, while writing this patch, I noticed one pointless query on node/n pages - #330674: node_comment_mode() causes pointless queries, so we can at least even it out.

taxonomy.test has new helper functions createTerm() and createVocabulary() as of the past day or so. Re-rolled to use those and for offset. No other changes.

node_feed() wasn't converted to use the new function, now it is.

edit: ran all tests, passed.

Thanks webchick!

This comment doesn't reflect what the code below is doing. Is the code wrong, or the comment?

Comment was wrong, but the code could be simplified too - I'd modified both the comments and the code to make this more self-documenting.

book_nodeapi_load now has $types as the second argument - I should note that most core hook implementation just ignore any extra arguments which aren't needed (look at $a3, $a4 in the current nodeapi implementations) - and we need to go clear that up everywhere else too.

   $vid = variable_get('forum_nav_vocabulary', '');
+  // If no forum vocabulary is set up, return.
+  if (!$vocabulary = taxonomy_vocabulary_load($vid)) {
+    return;
+  }

?>
It should just return, since it has 'nothing to do here' - however I've re-ordered the code a little bit to make it more self documenting, and it saves a taxonomy_vocabulary_load() if there really is nothing to do now.

I don't understand how that could work, since you selected tid not tid AS forum_tid.

+  if (!empty($vids)) {
+    $result = db_query('SELECT nid, tid  FROM {forum} WHERE vid IN(' . db_placeholders($vids) . ')', $vids);
+    foreach ($result as $record) {
+      $nodes[$record->nid]->forum_tid = $record->tid;
+    }
   }

$record->tid is OK, forum_tid is the object property I'm assigning the variable to - which needs to be explicitly set now we're operating directly on the node object.

Also, since nids is plural, it should be $nids_were_passed? Tough to say since technically you mean $the_nids_argument_was_passed_into_the_function. :P

Do we need this variable at all? Is !empty($nids) not clear enough by itself?

Added lots of code comments and also re-ordered the code a bit - I think it should be 'was_passed' since it specifically refers to the variable. Short version is that $nids drops down to empty if we're loading everything from cache, and we need to know that it wasn't empty at some point so we can skip the database entirely if everything's been fetched from cache before we get to the query.

Is there a way to use a standard hook function like module_invoke_all() here? These $function($thingy) things make it insanely difficult to figure out where hooks get fired. If this is for performance then fine.

call_user_func_array() is expensive, so I've left this as is.

poll_load() ->
You have to have a GROUP BY when using the MAX like that, however that's all pointless since a db_query_range() works fine - and should've been used in the first there place in poll.module - I just fixed half of it instead of all of it, now it's better.

I'm getting turned around with all of those vids that mean completely different things. Can we please make $vids $node_vids

Done, - we should really change vid to rid in node_revisions though IMO.

$has_rows - killed this completely, was a holdover from the previous implementation.

tsk tsk at path_node_api_load()? I don't want to touch drupal_get_path_alias() or drupal_lookup_path() here. And if necessary I'll start posting pictures of kittens in issue followups to avoid doing so :p

That should be everything I think. Here's a re-roll.

ack, found an actual bug in forum_nodeapi_load(), fixed for that. And the foreach whitespace in path_nodeapi_load() ;)

Was some offset with patches applied this morning, so re-rolled for that, also found one typo in a code comment.

edit: On the assumption that #314870: UNSTABLE-4 blocker: Add api.php for every core module will go in first, I've attached straight diffs to add documentation for the altered hooks. Note there's no current documentation for hook_nodeapi_load() at all - we've still got hook_nodeapi() in there.

Since these aren't in HEAD yet, these are ghetto diffs rolled from in the module directories, one each from node and taxonomy. Hopefully this is enough to avoid another round re-roll to keep up with HEAD, not withstanding other changes needing to be made - obviously I'm not going to disappear once this gets in so will do any extra followup documentation that's required - but as good as possible for now.

testing.drupal.org doesn't like me today. No changes, just trying to get it tested.

Re rolled with hook docs.

Re-rolled after: #338403: Use {@inheritdoc} on all class methods (including tests)

I think the concern might be that this patch now creates two ways of doing something we used to only have one way for, so it's a DX detriment, in that respect.

Well, let's see how much of a DX detriment.

The current node_load() function signature:

function node_load($param = array(), $revision = NULL, $reset = NULL)

This can be

node_load($nid)
node_load(array('nid' => $nid))
// or if loading a revision.
node_load($nid, $vid)
node_load(array('nid' => $nid, 'vid' => $vid))

As opposed to node_load($nid) or node_load($nid, $vid) as standardised by this patch.

So we already have two ways of doing exactly the same thing, just with the one function. A quick search for node_load in contrib shows that both ways are still used.

On the other hand if you do node_load(array('title' => $title)) - you'll have to hope that you have unique titles in whichever database your code is being called on, because you'll only ever get one back. Loading nodes by something non-unique, even by timestamp, is a fundamentally different thing to doing so by nid or vid, so not really the same thing at all.

While hook_nodeapi_load() taking an array adds a little bit of complexity (in the worst case, a foreach loop around the code that's already there) - a quick look at the API docs shows that you can grab all pertinent information from a simple module and stick it into the node object in four lines of code - saving 9 queries per hook on a standard front page (or RSS feed, or taxonomy/term listing).

+function hook_nodeapi_load($nodes, $types) {
+  $result = db_query('SELECT nid, foo FROM {mytable} WHERE nid IN(' . db_placeholders(array_keys($nodes)) . ')', array_keys($nodes));
+  foreach ($result as $record) {
+    $nodes[$record->nid]->foo = $record->foo;
+  }
+}

While we could certainly mess with the function signatures to node_load - making it internally a more complex, but allowing for node_load(array(1,2,3,4)) etc. , at the moment it's at least as readable as what's currently there.

kbahey has already benchmarked this patch, using the same methodology as http://2bits.com/articles/performance-benchmarking-drupal-512-drupal-66-... Drupal 7 is 30% slower than Drupal 6 currently according to those benchmarks, this patch gets us at least half the way towards pulling that back.

Er, didn't mean to change title, or only in a freudian sense anyway ;)

Some more background on the motivation for this patch -

When introducing hook_taxonomy_term_load() (making terms into a first class object), there's some critical taxonomy functions where the extra information loaded into term objects won't be available - taxonomy_get_tree() and taxonomy_node_get_terms() are the most obvious ones - where the database is queried directly and/or adds extra information internally. Think modules like taxonomy_image which might want to replace terms with icons on teasers. To have consistency of term objects as returned by these functions, in its current state, we'd have to load every term one by one from the database.

On a front page with 30 nodes x 5 unique terms each, that's 150 queries (+ another 150 for each term hook invocation). This compared to 30 currently, and 2 with this patch (+1 for each term hook invocation). taxonomy_node_get_terms() is converted here. taxonomy_get_tree() needs its own rewrite - since it currently brings sites to their knees just by itself. We can either 1. punt on that extra flexibility, and have taxonomy terms in a limbo between first class Drupal object and what they are in D6, 2. introduce a massive performance hit, or 3. use something similar to what's proposed here to give an actual performance gain with the same level of flexibility.

Prior to starting on this, I was also looking at #111127: Cache node_load. Assuming this goes in at some point, the next step is to re-roll that patch with some alterations to fit the new mechanism, which would allow us to load every node on a standard front page in a single database query. Since that patch would require this one, I've not written it yet, but we're looking at another 58 queries saved on nodes with uploads and path aliases. About a 50% reduction compared to where we stand currently. Even if we're able to make some gains elsewhere with lazy loading etc., we're not going to be able to make the same gains being made here.

So at the moment, node_load() does an extra join on the user table to grab name and picture - which is a nasty hack caused by the lack of an API such as this. If we did go ahead and refactor user_load() in a similar way (which I'd be very happy to do), we could load full user objects here instead very inexpensively, decoupling the code from node_load entirely and using our own APIs for what they're supposed to be used for. We don't have to do this, but it's at least an option (and would be an option in contrib too). With Field API on the horizon, this opens up a lot of possibilities for adding richer metadata to core objects without the massive hit that 'everything is a node' approaches have been dogged with in the past - and which we'd be running the risk of introducing everywhere else too when CCK fields can be applied to any object. However in the interest of my own sanity and kittens, I stuck with the most obvious ones here - and potential inconsistency notwithstanding, we still get a nice fat performance gain either way.

Just for reference, taxonomy.module does this for terms, already, for similar reasons - just in a very nasty self-enclosed way, and taxonomy_get_vocabularies() does the same 'return array of objects' routine as here, comment_render does the same 'load everything in one query' thing too. So this isn't completely new - it's in large part refactoring old stuff that desperately needs refreshing given the things people are trying to do with Drupal now.

So, I can keep mouthing off about performance and API gains, but with only a total of five sentences to go on in over five weeks, none of which express specific concerns with the approach, it's a little hard to know where Dries' reservations lie.

how does contrib know when to create one and when to not?

The rules for contrib following this would be pretty clear IMO:

"When your module creates a new 'top level' object with its own foo_*_load() hook, and which will be shown in lists to site visitors." I can't think of any contrib modules that do this though.

Found a way to fix something webchick and Damien brought up previously - I'd documented as a behaviour, but it became easier to solve properly as a product of another change. The $nodes array returned by node_load_multiple() is now always in the same order as the original $nids array that gets passed in. Also removed the foreach loop which was pulling nodes/terms from cache - PHP's built in array functions used instead (and a bit nicer than the version of this we had before). Since there's been a few small changes since the last round of benchmarks, I'll try to run another set just to verify nothing expensive has been introduced, or if anyone else has a chance, that'd be great too.

grrr, doc typos do not show themselves until uploaded to drupal.org.

Fresh benchmarks as promised. Site has 5,000 nodes, a mixture of page and article, some with comments and taxonomy terms.

Benchmarked /node /node/10 /rss.xml and /taxonomy/term/10 using ab -c1 -n500.

I got between 10-38% speed up on the listings pages - depending on number of nodes per page and what else is shown. Still around a 2% decrease on node/10 (which calls node_load() 17 times apparently suggesting the array handling inside node_multiple_load for cache fetching hasn't caused any harm since the last set of benchmarks, which were also between 1-3% hit).

All numbers are requests per second:

node (30 nodes)
before:
3.40
after:
4.71
(38% faster)

node/10 (1 node)
before:
11.77
after:
11.56
(2% slower)

rss.xml (10 nodes)
before:
10.92
after:
14.19
(36% faster)

taxonomy/term/10 (8 nodes)
before:
4.27
after:
4.68
(10% faster)

There's the following places where extra work is done in the single case.

* pulling stuff out of cache once when there's multiple requests to node_load() on a single page (see below)
* One extra query to pull taxonomy terms in (but as noted, the current query bypasses taxonomy_term_load() when pulling terms out which hamstrings the new hooks, so to keep a balance between performance and flexibility, it's our best option IMO even with a small hit).
* db_placeholders(), IN() and foreach in hook_nodeapi_load.
* (maybe) Using the dynamic query builder in node_load_multiple() and taxonomy_term_load_multiple() (HEAD isn't converted to the new API yet).

Stuff we can do:
* Special case the single case pulling stuff from cache in node_load() - this pulled 1% back without nodeapi modules enabled (see below))
* I'd also expect us to get at least that 2% back with caching of node_load() (and a decent improvement on listings too). I'm planning to jump into that after this one, but could work up a proof of concept patch to gauge the performance difference pretty quickly.
* Like Damien said, there's bound to be some more micro-optimisations which could be done.

New patch which uses reset() instead of array_* to pull things from the node cache in the single case. Same site as above, all core modules enabled, terms and comments on the nodes. node/10

HEAD:
13.58
13.51
13.34
New patch: (2% slower)
13.17
13.23
13.22
Patch from #106 (also 2%, but looks like the new one is a tiny bit faster).
13.12
13.16
13.16

Same thing but with all nodeapi modules disabled:
Head:
16.60
16.86
16.85
Patch with special casing for single cached nodes (1% slower, nearly margin of error):
16.60
16.62
16.64
Patch from #106 (Still 2% slower):
16.38
16.56
16.29

My feeling is that there's lots of places in core we can optimise to pull a couple of percent from on a single node view (caching node_load, using the registry properly etc.), but not the same opportunity to get 20-40% back on node listings. Where it's more important will depend on the site - Drupal.org has both a front page which presumably gets a large proportion of traffic, and also a very long tail of individual node pages. If you have a lot of RSS feed subscriptions and taxonomy term listings, you'd see plenty of benefit, a small brochure site probably wouldn't.

Anyone has time to do their own benchmarks, that'd be great to verify these one way or the other. I'll see about rolling the cache patch so it's benchmarkable - not that we couldn't do it by itself, but it'd be a single query on a node listing so hopefully lightning fast. edit: and see what Justin comes up with :)

killes sent me an html dump of the 'url details' awstats report for Drupal.org (as of yesterday, presumably these are for November so far)

19370924 pages total

Most of the top pages are node listings, others include user/register /forum /project /cvs /xml.rpc /planet/feed etc. which are neither node listings nor individual nodes - those get 90k-300k each.

Here's the top ten node listing pages:
1016099 - /node
702970 - /node/feed
650053 - project/issues/rss
256192 - /security/rss.xml
146356 - taxonomy/term/14/0/feed
75554 - /project/Modules/name
74926 - /taxonomy/term/51/0/feed
72191 - rss.xml
64090 image/tid/39
64040 taxonomy/term/8/0/feed

These 10 pages make up 15% of total traffic to the whole site.

The first individual node page on the list is /node/258 at 62015 - between 30-60k views per month, it's a mixture of non-node content, more listings, and pages like /project/cck which are for individual nodes.

Divvying that up - this is educated guesswork but close enough for our purposes - let's say of the 19 million, 20% is non node content - user* tracker* project* forum* etc. - that leaves 15 million. Of that we'll say 25% is node listings.

So that leaves 3.8 million listing views, and 11.2 million individual node views.
Now we pretend my dev install is drupal.org, and compared node/ and /node/10 - now time per request instead of requests per second. Then add 2% per request for node/10 and split the difference at taking 18% off for node/ when the patch is applied.
node/10
HEAD: 83.6ms
patch: 85.2ms

node/
HEAD: 262ms
patch: 215ms

Multiply the pages served by the time taken to serve them, then add it together:

HEAD              Patch:
936320000	954240000 (node/10)
995600000	817000000 (node/)
-----------
1931920000	1771240000 (totals)

Justin's analysis of node_load_multiple() also applies to taxonomy_term_load_multiple() - which uses the query builder too. So if it's really 1ms extra for the query builder, then 2ms on a page request is about our 2% hit. A straight patch to convert node_load() would also introduce the same penalty (but likely not be benchmarked since we have to convert it at some point anyway). If we can verify this then there's not a lot I can do here.

However obvious next step to improve node_load() performance across the board is to cache node_load() - that's out of scope for this issue, but as noted, my next stop if/when this goes in.

OK, minus a missing !empty($conditions) check, the straight dbtng conversion from #114 works great - so benchmarked that vs. #113 on node/10. Also attached it in patch form if anyone else wants to run the same thing.

Benchmarks are now inconclusive between HEAD, dbtng and the patch on node/10 (my laptop is running on battery this time so could be that).

Noticeable hit with straight dbtng conversion on /node though - about 6%- presumably because the query is built 30 times for the 30 different nodes.

All numbers are #reqs/second
node/10  (middle result of three benchmarks - doesn't match earlier ones so we can write this off to laptop energy saving maybe)
HEAD:
11.86

node_load converted to dbtng only
11.96

Multiple load (inc dbtng):
12.12


/node
HEAD
3.90
3.89
3.90

node_load converted to dbtng only: (about 6% slower).
3.70
3.60
3.61

Multiple load (inc dbtng), faster, as before.
5.08
5.13
5.07

Would be good to get a set of benchmarks from someone else too, but looks like all the performance hit is due to dbtng, and a quite significant extra hit on listings if we just did a straight converson without the multiple load stuff alongside it.

edit: also attached ab output for /node - fresh benchmarks because I didn't save this the first time round, but same general results as above.

With my laptop plugged in I get slightly more reliable results for node/10 - these pretty much match what Justin reckoned. One bench each for HEAD, dbtng conversion only, and multiple load. If someone has a proper benchmarking environment to run similar on, that'd be really useful here - I getting quite a bit of variation for each ab run - so while I've tried to pick averages, it's hard to know if this is just margin of error when there's only a couple of percent in it anyway.

Since the 2% hit isn't down to anything internal with the patch, up to date version which removes the red-herring special casing for single node loads.

One thing we could maybe do for API clarity is hook_nodeapi_load($nodes, $nids, $types); - to save people using array_keys($nodes). Trivial patch to roll, but I'll leave it here for feedback first.

Here's a straight re-roll after the singular tables patch went in. Will roll hook_nodeapi_load_multiple($nodes, $nids, $types); next for comparison.

and the patch.

So the two main places where the hook_nodeapi_load functions get longer are comment and taxonomy.

As described earlier, taxonomy.module currently takes a shortcut to load term objects - since it bypasses its own API to load terms. I don't see a way to fix this in a scalable way without loading full term objects in the one database call (as opposed to dozens of taxonomy_term_load()) - so the extra length is partly due to combining the nodeapi_load conversion and the taxonomy api changes in one patch, although they're fairly hard to separate. This should simplify a lot of things in taxonomy.module and the contrib that depends on it down the line though, if that's any consolation.

With comment_nodeapi_load() - there's some extra length there because I tried to optimise for not querying the database unless absolutely necessary. In Drupal 6, we issue a query whether comments are enabled or not, this was fixed in Drupal 7 - see #250729: Comment nodeapi unnecessary query. comment.module is fairly unusual in having an individual setting per node as well as the per-node type default - it's also got a symbiotic relationship with node.module - i.e. http://drupal.org/node/330674 and http://drupal.org/node/148849 - so I'd hope that it's the exception rather than the rule in terms of additional length.

In the basic case, it's just an extra foreach loop, and swapping an = for an IN() which isn't too bad I think. But yes, some of the hook implementations are going to get a bit longer - especially highly optimised ones, but then we save dozens of queries on node listings from that extra code weight - and it's run once instead of over and over.

Here it is with the additional $nids parameter to save hook implementations doing an array_keys() in just straight queries.

Typos in the api docs, thanks test bot.

hmm. Tests pass locally, and I can't see any syntax errors in the patch file either. Another try.

re-rolled with changelog.txt entry

Thanks swentel. Re-rolled.

Nice!
Upgrade docs here: http://drupal.org/node/224333#node_load

No chance of a backport, this is a big API change.