In _db_query(), we call variable_get('dev_query', 0) to know if we should cache the query or not. In fact, we call it twice. While the value of that variable does, in fact, change over the course of a complete page run it never changes within a single _db_query() call, so there's no good reason to issue two function calls. Attached patch changes it to just one call that we save to a variable, which saves us one function call per query issued regardless of whether dev_query is TRUE or not. (That's 50-100 function calls on many sites.)

CommentFileSizeAuthor
#3 dev_query.benchmark.tar_.gz_.txt3.43 KBhswong3i
dev_query.patch2.72 KBCrell

Comments

hswong3i’s picture

hswong3i commented

may i have some solid supporting about performance improvement? as varibale_get is just something as simple as this format:

<?php
function variable_get($name, $default) {
  global $conf;

  return isset($conf[$name]) ? $conf[$name] : $default;
}
?>

an additional function call may reduce performance (which is usually truth), but it may be something need not to be consider, too. i guess it may have 0.5% of overall performance improvement, but it will need to be proved by benchmarking :-)

Crell’s picture

Crell commented

Fresh checkout of HEAD, devel-generate 300 nodes, put 30 on the front page. Total queries for the front page: 291. Running ab with 100 requests:

Requests per second:    3.06 [#/sec] (mean)
Time per request:       326.409 [ms] (mean)

Apply patch, rerun test:

Requests per second:    3.04 [#/sec] (mean)
Time per request:       328.697 [ms] (mean)

I have to say I'm surprised at that. In D5, we were able to get a 5% boost by skipping init_theme() in theme(), which is called a few zillion times as well. (In that case init_theme() was also just doing an if(isset($foo)) return; check, so it was trivial as well.) Perhaps function calls are less expensive in PHP 5.2 (what I run now) than they were in 5.1 (what I was running then)?

hswong3i’s picture

hswong3i commented
StatusFileSize
new dev_query.benchmark.tar_.gz_.txt3.43 KB

i would like to give a hand for an indeed benchmarking, so we will able to have a much complete picture about the case (again, same configuration as http://drupal.org/node/172541#comment-298711):

                  +-------------+---------------+----------------+
                  | c1, n500    | c5, n500      | c10, n500      |
+-----------------+-------------+---------------+----------------+
| D6 HEAD         | 287 +- 9.5  | 1471 +- 350.5 | 2862 +- 629.5  |
+                 +-------------+---------------+----------------+
|                 | 291 +- 13.2 | 1452 +- 422.3 | 2856 +- 1363.5 |
+                 +-------------+---------------+----------------+
|                 | 290 +- 63.7 | 1437 +- 466.3 | 2934 +- 1128.3 |
+-----------------+-------------+---------------+----------------+
| D6 HEAD + patch | 288 +- 12.4 | 1453 +- 394.5 | 2972 +- 659.3  |
+                 +-------------+---------------+----------------+
|                 | 289 +- 9.3  | 1433 +- 428.6 | 2914 +- 777.3  |
+                 +-------------+---------------+----------------+
|                 | 287 +- 19.5 | 1469 +- 617.8 | 2916 +- 1799.2 |
+-----------------+-------------+---------------+----------------+
chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Common sense says this is a performance boost, no doubt small but that's no reason to not do it. benchmarking these small steps are not easy at this stage because what you can bench with? node_page_default which runs what, a dozen queries? A complex Drupal install runs hundreds and hundreds of queries on a page. Every little piece helps.

hswong3i’s picture

hswong3i commented

@chx: it is not only a default installation: i active ALL core module (+ devel) when doing these benchmarking. i shouldn't forget to mention about this, it is totally my fault :(

BTW, if this benchmarking it not meaningful, even though having 3 different concurrency level (1, 5, 10), with each 3 set of result samples, for totally 18 times of benchmarking on top of same platform and settings, i would really interesting about what you would like to believe with?

i don't care to do a much larger scale of benchmarking if i am able to do so (with the help of a very simple script, all we need is time), but may you give me some hints about how can let you believe it as a scientific benchmarking result? i really hope to know what should we believe with, other than these solid result :(

P.S. i am not opposing this patch, but only hope to provide a better and complete picture about the real case, and so we will able to support what we are believing with, with some solid factors :)

dries’s picture

dries commented
Status: Reviewed & tested by the community » Closed (won't fix)

I don't think this is worth fixing. It's not necessarily faster as we have more code to parse.