- Development - lists.drupal.org

[drupal-devel] Cleaner Directory structure
by K B 12 May '05

12 May '05

There has been a discussion in the forums of having a cleaner directory structure for Drupal. Here http://drupal.org/node/22269#comment-38302 I said: -- BEGIN This will simplify upgrades and other things. It isolates Drupal in its own directory, so other stuff can exist in their own directories. All data files, image files, and other things have to be outside drupal. Only additional modules, themes and settings.php files should go under the Drupal directory. As a matter of fact, even local modules and local themes, and even the sites directory can be located in its own directory For example: public_html - .htaccess - index.php - drupal/ - drupal-local/ Under drupal-local we have: - drupal-local/sites/ - drupal-local/sites/site1.com/settings.php - drupal-local/sites/site1.com/themes/ - drupal-local/sites/site1.com/modules/ - drupal-local/sites/site2.com/settings.php - drupal-local/sites/site2.com/themes/ - drupal-local/sites/site2.com/modules/ - drupal-local/themes (themes shared by all sites) - drupal-local/modules (modules available to all sites) Drupal can search for drupal-local/ first for configuration files, then search drupal/modules and drupal/themes for modules and themes, then search drupal-local for modules and themes. The "drupal" directory should contain the standard Drupal distribution with no modifications whatsover, with instructions to move or copy the .htaccess and index.php to the directory above, so local changes can be merged on upgrades manually. This way, upgrades will be much easier: everyone should leave the drupal/ directory alone, and only mess with the drupal-local directory. -- END What do other developers think of this? It will address one gripe that people have, which is the difficulty of doing upgrades.

13 32

[drupal-devel] [task] Performance issues with path aliases
by Dries 12 May '05

12 May '05

Issue status update for http://drupal.org/node/22035 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: mathias Reported by: mikeryan Updated by: Dries Status: patch That last patch doens't seem to work. None of my URLs are being substituted by their path alias. Also, mind to rename the src action to source? Dries Previous comments: ------------------------------------------------------------------------ May 5, 2005 - 03:10 : mikeryan See Investigate use of conf_url_rewrite [1] for context... The current core support for translating incoming path aliases to the internal Drupal path (drupal_get_path_alias) and substituting aliases when generating links to internal paths (drupal_get_normal_path) does not scale well with many aliases. The ease with which pathauto enables site administrators to generate large numbers of aliases exposes this issue, but it is inherent in the core implementation, because drupal_get_path_map reads in the entire url_alias table at bootstrap time. I'd like to discuss ideas on how to improve the performance... Most obviously, why not simply query the url_alias table as needed instead of loading the whole table? In the incoming case, only a single simple SELECT is necessary, which will always be more efficient than reading the table. In the outgoing case, there might be some slight performance advantage to caching the table with a small number of aliases, but the disadvantage can become huge as the alias table grows. One note - I've noticed that the src column in url_alias is not indexed, I think adding an index should significantly help the performance in the outgoing case if we were to do individual SELECTs. Any other thoughts? [1] http://drupal.org/node/21938 ------------------------------------------------------------------------ May 5, 2005 - 04:02 : mikeryan Attachment: http://drupal.org/files/issues/bootstrap.inc_2.patch (1.06 KB) What the hell, I went ahead and gave it a shot... On my home system, where I'm testing out 4.6.0, page loads have been taking several seconds, which I attributed to the fact that it's an old computer and I'm multi-tasking like crazy. I implemented my own suggestion, and now pages load in about one second, it makes an incredible difference (FWIW, my url_alias table has over 4000 rows). Patches attached, go give it a try... ------------------------------------------------------------------------ May 5, 2005 - 04:02 : mikeryan Attachment: http://drupal.org/files/issues/common.inc_11.patch (723 bytes) One attachment per note, that's tedious... ------------------------------------------------------------------------ May 5, 2005 - 04:03 : mikeryan Attachment: http://drupal.org/files/issues/path.module_0.patch (1.34 KB) ------------------------------------------------------------------------ May 5, 2005 - 04:03 : mikeryan Attachment: http://drupal.org/files/issues/database.mysql_4.patch (553 bytes) ------------------------------------------------------------------------ May 5, 2005 - 04:04 : mikeryan Attachment: http://drupal.org/files/issues/database.pgsql_2.patch (502 bytes) ------------------------------------------------------------------------ May 5, 2005 - 04:07 : mikeryan Note: I just noticed that the database.*sql patches showed an extra diff (not mine) to the location field in locales_source. I did my diffs against a freshly-updated DRUPAL-4-6, and had made the edits against a release from a few days ago... Those locales_source changes should probably be removed from my patches before applying. ------------------------------------------------------------------------ May 8, 2005 - 01:13 : mikeryan I upgraded Fenway Views [2] to Drupal 4.6.0 today, incorporating these patches. Performance is very noticeably improved. [2] http://fenway-views.com/ ------------------------------------------------------------------------ May 8, 2005 - 15:31 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/pathalias.patch (4.98 KB) I've merged the patch into one. Much more convenient. I also changed it to cvs as only there new features will be added. Upgrade path needs to be added. Mike, can you run some more tests? We are especially interested in "hard" data ie numbers with possibly error bars. it would be interesting to know how this patch affects sites that have only a few path aliases vs those with a lot of them. Also, how pages with a lot of node links (tracker) are affected. It might be worthwhile to add static caching in drupal_get_normal_path if it gets calle dmore than once for a link on one page view. ------------------------------------------------------------------------ May 8, 2005 - 19:21 : mikeryan Thanks for merging the patches, I wasn't aware you could patch multiple files at once. If this is accepted, of course, adding the index on src should be incorporated into updates.inc. In terms of hard data, I've never profiled PHP code - what tool(s) do you use? I couldn't find anything referenced in the PHP manual... re: pages with lots of links - on Fenway Views the big test is the calendar [3], and the performance improvement seems even more dramatic here. I don't know why - the url_alias table is read once per page, so I would expect as you do that preloading the table would tend to look better when you have a hundred internal links on the page. Maybe we're both underestimating exactly how fast a simple MySQL query on an indexed key can be (I'm using MySQL 4.0.20, BTW)... [3] http://fenway-views.com/calendar ------------------------------------------------------------------------ May 8, 2005 - 19:26 : killes(a)www.drop.org As we speak, Mathias is doing some tests. The tool of choice is usually apache bench. If you have a lot of url aliases, the generated array will be huge. Part of the improvement could be due to the fact that you need less memory now. ------------------------------------------------------------------------ May 8, 2005 - 19:28 : mikeryan Great, thanks! ------------------------------------------------------------------------ May 8, 2005 - 19:34 : Dries On drupal.org it takes 800 ms or more to generate a page. Of these 800 ms, only 4 ms is spent building the path alias map (incl. the SQL query time which takes about 1 ms). That is, building the map takes 0.5% of the total time which is neglible. In our url_alias table are only 321 aliases though. How many aliases do you have? What is more, when we build drupal.org's main page, we query this map 215 times. I believe your patch would introduce 215 SQL queries ... I'm afraid that if we'd apply your patch, we'd pay a serious performance penalty (unless we have many more aliases). Can you provide some numbers/measurements? ------------------------------------------------------------------------ May 8, 2005 - 19:37 : Dries Note: my measurements did not take into account the time spent searching the map. On the main page, we search this array 215 times. ------------------------------------------------------------------------ May 8, 2005 - 20:08 : puregin I believe that it's felt that adding path aliases would improve the Drupal documentation. This may well add 400+ paths to the URL aliases table on Drupal.org ------------------------------------------------------------------------ May 8, 2005 - 20:27 : Dries I spent some more time investigating this. The relevant code in bootstrap.inc is this: function drupal_get_path_alias($path) { if (($map = drupal_get_path_map()) && ($newpath = array_search($path, $map))) { return $newpath; } ... Turns out that the time to build the map is neglible (< 5ms, see previous comments), however the total time spent on 'path aliasing' it about 70ms per page! 50ms of these 70ms are spent on $map = drupal_get_path_map(). The remaining 20ms is spent on $newpath = array_search($path, $map). The first call to drupal_get_path_map() takes 3 to 5 ms, each subsequent call takes about 0.3 ms. Searching the array costs 0.1 ms. However, if you have to do this 130+ times, this adds up to a whopping 70 ms. Remind that we have 321 aliases in our map. As drupal_get_path_alias() is typically called from code>url(), which in turn is typically called from l() I set out to investigate the time spent in l(). Looks like l() easily gets called 120 times/page, and that each call to l() costs about 0.9 ms. That is about 2 or 3 times the cost of the /average SQL query/. The total time spent in l() is 115 ms! ------------------------------------------------------------------------ May 9, 2005 - 00:25 : adrian This is a complete aside, and probably off-topic, but I would like to mention that I was actually looking at rewriting url() to be able to handle 'external urls' , because I want to do aliases using the site subdomain, like for instance : http://category.sitename.com/article/title_goes_here -> node/23 Rewriting url to allow this would also allow us to use l() for external links of any kind, getting rid of a bunch of inline html. ------------------------------------------------------------------------ May 9, 2005 - 00:46 : chx adrian, I have written a patch for l to support external links but Ber said that a module like weblink shall handle those. ------------------------------------------------------------------------ May 9, 2005 - 01:03 : mikeryan I've got over 4000 aliases on Fenway Views - another pathauto user reported over 6000. I don't have a profiling tool to give timing data, but adding a quick counter shows that for the Fenway Views calendar [4], drupal_get_path_alias() is called 404 times and drupal_get_normal_path is called 227 times. And trust me, this page loads MUCH faster with my patch than it does with the path map. I think those SQL queries are a lot cheaper than one might expect... [4] http://fenway-views.com/calendar ------------------------------------------------------------------------ May 9, 2005 - 06:24 : mathias Attachment: http://drupal.org/files/issues/pathalias-with-caching.patch (5.62 KB) Some benchmarks, with MySQL and Drupal caching disabled. 2 SETS OF TESTS ======================================= 1) 500 nodes and aliases 2) 20,000 nodes and aliases 3 TYPES OF TESTS PER SET ======================================= 1) Baseline unmodified Drupal 2) The following change inside drupal_get_path_map(): - if (is_null($map)) { + if ($map === NULL) { $map = array(); // Make $map non-null in case no aliases are defined. 3) Pull aliases only when needed rather than loading the entire alias table. SET 1 - 500 Nodes and 500 Aliases using: ab -c 10 -n 100 [homepage] ======================================= Baseline Time taken for tests: 13.00 seconds Requests per second: 3.85 [#/sec] (mean) Transfer rate: 56.35 [Kbytes/sec] received is_null() VS NULL Time taken for tests: 12.463 seconds Requests per second: 4.01 [#/sec] (mean) Transfer rate: 57.63 [Kbytes/sec] received Per Instance Alias Lookup (32 additional queries) Time taken for tests: 11.502 seconds Requests per second: 4.35 [#/sec] (mean) Transfer rate: 63.30 [Kbytes/sec] received SET 2 - 20,000 Nodes and 20,000 Aliases using: ab -c 5 -n 50 [homepage] ======================================= Baseline Time taken for tests: 204.583 seconds Requests per second: 0.24 [#/sec] (mean) Transfer rate: 3.34 [Kbytes/sec] received is_null() VS NULL Time taken for tests: 127.629 seconds Requests per second: 0.39 [#/sec] (mean) Transfer rate: 5.35 [Kbytes/sec] received Per Instance Alias Lookup (32 additional queries) Time taken for tests: 28.788 seconds Requests per second: 1.74 [#/sec] (mean) Transfer rate: 23.59 [Kbytes/sec] received ANALYSIS ======================================= Converting 'is_null()' to '=== NULL' is a no brainer and results in a nice performance gain. And while per instance alias lookups also give a huge boost for site with thousands of aliases, they're benefits are entirely dependent on the number of system URLs and menu items visible per request. As noted above I tested the standard homepage which added 32 additional queries. A request for something like the admin interface would presumably show less benefits. I would've tested this, but I didn't know how to invoke an authenticated page request with 'ab'. A positive of this approach is we no longer would be storing all the aliases in memory. What other concerns do we have with this last approach? Am I properly testing the strain on the database? ------------------------------------------------------------------------ May 9, 2005 - 07:08 : Dries Can you try making the following changes? 1. To common.inc: function drupal_rebuild_path_map() { - drupal_get_path_map('rebuild'); + drupal_get_path_map(TRUE); } 2. To bootstrap.inc: -function drupal_get_path_map($action = '') { +function drupal_get_path_map($rebuild = FALSE) { static $map = NULL; - if ($action == 'rebuild') { + if ($rebuild) { $map = NULL; } That is, replace the string comparison with a boolean comparison. Not sure it is going to make a significant difference but it might be another micro-improvement. I'll test your patch shortly. ------------------------------------------------------------------------ May 9, 2005 - 14:21 : mathias Dries. Those changes had no real performance gain. 20,000 Nodes and 20,000 Aliases using: ab -c 5 -n 50 [homepage] ======================================= Baseline Time taken for tests: 204.583 seconds Requests per second: 0.24 [#/sec] (mean) Transfer rate: 3.34 [Kbytes/sec] received String VS Boolean Time taken for tests: 204.190 seconds Requests per second: 0.24 [#/sec] (mean) Transfer rate: 3.34 [Kbytes/sec] received ------------------------------------------------------------------------ May 9, 2005 - 15:01 : Dries Before this patch can be committed, we need to do more testing. I'd like to know how this behaves on sites with few or no path aliases, and on sites like drupal.org, with a modest amount of path aliases. The reason I'm asking is because MySQL is often the bottleneck and not PHP/Apache. This is the case on drupal.org, for example. The proposed patch moves some of the processing costs from PHP to MySQL. On drupal.org, the amount of SQL queries/page would double. Needless to say, this is somewhat scary. ;) More numbers would be appreciated. ------------------------------------------------------------------------ May 10, 2005 - 04:56 : mikeryan I'm curious, why test with MySQL caching disabled? Since much of the issue is the expense of making (potentially many) more queries, this wouldn't seem to reflect the performance gain in practice. The caching (compared to making the query each time) seems like overkill to me - with MySQL caching enabled, I would think this complicates the code for little (if any) performance gain. Did you do any profiling using my original (cacheless) patch? Thanks. ------------------------------------------------------------------------ May 10, 2005 - 07:23 : mathias In an ideal world everyone would be running a query cache but I wanted to see how this would hold up under the worst-case scenario. I tested the original patch which didn't use static variables and the amount of queries doubled. For example, to load the front page, Drupal queried the url_alias table 22 times looking for an alias for 'node'. There's no need to send duplicate queries to the DB since it seems that's where most bottlenecks seem to lie. ------------------------------------------------------------------------ May 11, 2005 - 00:41 : mikeryan re: worst-case scenario - I understand that. re: # queries - I would not want to assume adding to the number of queries is necessarily a significant performance hit (with database caching on, of course). I understand the suspicion - I've been around long enough to remember when you did everything you could to avoid making one single SQL query more than you absolutely had to. But modern databases (including MySQL) are very well optimized for this kind of application (lots and lots of small queries). And, I have seen real-life cases where adding caching of data that's already being cached somewhere upstream actually degrades performance. As long as we're examining this particular area of performance under a microscope, let's make sure we squeeze every millisecond of savings we can out of it - I'd just like to see the numbers for a cached database/no Drupal caching combination for comparison. Thanks. ------------------------------------------------------------------------ May 11, 2005 - 07:12 : mathias Dries asked me to do some tests using the drupal.org database. Once again it appears that plucking aliases out of the database as needed performed better than grabbing the entire alias table at the beginning of the request lifecycle, even if the alias table is small. Here's the numbers. using: ab -c 5 -n 50 [homepage] Drupal.org Baseline ============= Time taken for tests: 18.081 seconds Requests per second: 2.77 [#/sec] (mean) Transfer rate: 52.69 [Kbytes/sec] received 1 page request was 83 queries in 60ms for a cycle execution time of 255ms Drupal.org Per Instance Query ================== Time taken for tests: 16.536 seconds Requests per second: 3.02 [#/sec] (mean) Transfer rate: 56.63 [Kbytes/sec] received 1 page request was 125 queries in 65ms for a cycle execution time of 230ms Notice that the database does work a little harder to retrieve the queries one by one. And it also doesn't make sense to run 40 extra queries on a site with only 50 aliases. I think at this point we have a couple of options to explore: 1. Continue to try and squeeze out any other optimizations we can in the original path aliasing code. Maybe look at output buffering or a mechnism that'll allow us to start working the database resultset as soon data is fed into the pipe rather than waiting for the request to finish. 2. Try to identify the point (e.g., the number aliases) at which it becomes more effecient to fetch aliases and change patterns on the fly. As with most optimizations, the end result will probably be a combination of both. ------------------------------------------------------------------------ May 11, 2005 - 07:59 : Dries Looks like in all cases, performance actually improves. I've run a couple tests myself using the drupal.org database and measured performance improvements up to 15%. Of course, this was done on a machine where the MySQL database wasn't hammered on by 50+ concurrent requests. It would be appreciated if you could extend your patch with a couple lines of documentation to document the behavior (so people understand why we are doing it this way), the code and the API. Maybe add an entry to CHANGELOG.txt? Do we need an update for updates.inc? As for future work, I think we're better off looking at l(). As said, one call to l() takes about 0.9 ms and we easily do 100+ such calls per page. Caching at the level of l() is likely to buy us more but I don't know whether it is feasible. Maybe we can think of other optimizations too. If anything, it might be a good idea to add a TODO/NOTE to the PHPdoc of l(). Or, if you feel like messing with l() first, by all means! Count me in -- it's fun. :-) ------------------------------------------------------------------------ May 12, 2005 - 04:29 : mathias Attachment: http://drupal.org/files/issues/alias-query-on-demand.patch (5.72 KB) I think this patch is now commit-worthy. I did some more profiling using xdebug 2.0 [5] and kcachegrind [6] and sure enough, drupal_get_path_map() was a resource hog on sites with many aliases. This should be quite an improvement. I also found a couple other small optimizations such as replacing array_key_exists() with a NULL check inside the arg() function (before [7] / after [8]) and calling isset() in order to avoid calls to error_handler. [5] http://xdebug.org [6] http://kcachegrind.sourceforge.net/cgi-bin/show.cgi/KcacheGrindIndex [7] http://www.asitis.org/tmp/before_arg.png [8] http://www.asitis.org/tmp/after_arg.png

1 0

[drupal-devel] [task] Performance issues with path aliases
by mathias 12 May '05

12 May '05

Issue status update for http://drupal.org/node/22035 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal -Assigned to: Anonymous +Assigned to: mathias Reported by: mikeryan Updated by: mathias Status: patch Attachment: http://drupal.org/files/issues/alias-query-on-demand.patch (5.72 KB) I think this patch is now commit-worthy. I did some more profiling using xdebug 2.0 [1] and kcachegrind [2] and sure enough, drupal_get_path_map() was a resource hog on sites with many aliases. This should be quite an improvement. I also found a couple other small optimizations such as replacing array_key_exists() with a NULL check inside the arg() function (before [3] / after [4]) and calling isset() in order to avoid calls to error_handler. [1] http://xdebug.org [2] http://kcachegrind.sourceforge.net/cgi-bin/show.cgi/KcacheGrindIndex [3] http://www.asitis.org/tmp/before_arg.png [4] http://www.asitis.org/tmp/after_arg.png mathias Previous comments: ------------------------------------------------------------------------ May 4, 2005 - 19:10 : mikeryan See Investigate use of conf_url_rewrite [5] for context... The current core support for translating incoming path aliases to the internal Drupal path (drupal_get_path_alias) and substituting aliases when generating links to internal paths (drupal_get_normal_path) does not scale well with many aliases. The ease with which pathauto enables site administrators to generate large numbers of aliases exposes this issue, but it is inherent in the core implementation, because drupal_get_path_map reads in the entire url_alias table at bootstrap time. I'd like to discuss ideas on how to improve the performance... Most obviously, why not simply query the url_alias table as needed instead of loading the whole table? In the incoming case, only a single simple SELECT is necessary, which will always be more efficient than reading the table. In the outgoing case, there might be some slight performance advantage to caching the table with a small number of aliases, but the disadvantage can become huge as the alias table grows. One note - I've noticed that the src column in url_alias is not indexed, I think adding an index should significantly help the performance in the outgoing case if we were to do individual SELECTs. Any other thoughts? [5] http://drupal.org/node/21938 ------------------------------------------------------------------------ May 4, 2005 - 20:02 : mikeryan Attachment: http://drupal.org/files/issues/bootstrap.inc_2.patch (1.06 KB) What the hell, I went ahead and gave it a shot... On my home system, where I'm testing out 4.6.0, page loads have been taking several seconds, which I attributed to the fact that it's an old computer and I'm multi-tasking like crazy. I implemented my own suggestion, and now pages load in about one second, it makes an incredible difference (FWIW, my url_alias table has over 4000 rows). Patches attached, go give it a try... ------------------------------------------------------------------------ May 4, 2005 - 20:02 : mikeryan Attachment: http://drupal.org/files/issues/common.inc_11.patch (723 bytes) One attachment per note, that's tedious... ------------------------------------------------------------------------ May 4, 2005 - 20:03 : mikeryan Attachment: http://drupal.org/files/issues/path.module_0.patch (1.34 KB) ------------------------------------------------------------------------ May 4, 2005 - 20:03 : mikeryan Attachment: http://drupal.org/files/issues/database.mysql_4.patch (553 bytes) ------------------------------------------------------------------------ May 4, 2005 - 20:04 : mikeryan Attachment: http://drupal.org/files/issues/database.pgsql_2.patch (502 bytes) ------------------------------------------------------------------------ May 4, 2005 - 20:07 : mikeryan Note: I just noticed that the database.*sql patches showed an extra diff (not mine) to the location field in locales_source. I did my diffs against a freshly-updated DRUPAL-4-6, and had made the edits against a release from a few days ago... Those locales_source changes should probably be removed from my patches before applying. ------------------------------------------------------------------------ May 7, 2005 - 17:13 : mikeryan I upgraded Fenway Views [6] to Drupal 4.6.0 today, incorporating these patches. Performance is very noticeably improved. [6] http://fenway-views.com/ ------------------------------------------------------------------------ May 8, 2005 - 07:31 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/pathalias.patch (4.98 KB) I've merged the patch into one. Much more convenient. I also changed it to cvs as only there new features will be added. Upgrade path needs to be added. Mike, can you run some more tests? We are especially interested in "hard" data ie numbers with possibly error bars. it would be interesting to know how this patch affects sites that have only a few path aliases vs those with a lot of them. Also, how pages with a lot of node links (tracker) are affected. It might be worthwhile to add static caching in drupal_get_normal_path if it gets calle dmore than once for a link on one page view. ------------------------------------------------------------------------ May 8, 2005 - 11:21 : mikeryan Thanks for merging the patches, I wasn't aware you could patch multiple files at once. If this is accepted, of course, adding the index on src should be incorporated into updates.inc. In terms of hard data, I've never profiled PHP code - what tool(s) do you use? I couldn't find anything referenced in the PHP manual... re: pages with lots of links - on Fenway Views the big test is the calendar [7], and the performance improvement seems even more dramatic here. I don't know why - the url_alias table is read once per page, so I would expect as you do that preloading the table would tend to look better when you have a hundred internal links on the page. Maybe we're both underestimating exactly how fast a simple MySQL query on an indexed key can be (I'm using MySQL 4.0.20, BTW)... [7] http://fenway-views.com/calendar ------------------------------------------------------------------------ May 8, 2005 - 11:26 : killes(a)www.drop.org As we speak, Mathias is doing some tests. The tool of choice is usually apache bench. If you have a lot of url aliases, the generated array will be huge. Part of the improvement could be due to the fact that you need less memory now. ------------------------------------------------------------------------ May 8, 2005 - 11:28 : mikeryan Great, thanks! ------------------------------------------------------------------------ May 8, 2005 - 11:34 : Dries On drupal.org it takes 800 ms or more to generate a page. Of these 800 ms, only 4 ms is spent building the path alias map (incl. the SQL query time which takes about 1 ms). That is, building the map takes 0.5% of the total time which is neglible. In our url_alias table are only 321 aliases though. How many aliases do you have? What is more, when we build drupal.org's main page, we query this map 215 times. I believe your patch would introduce 215 SQL queries ... I'm afraid that if we'd apply your patch, we'd pay a serious performance penalty (unless we have many more aliases). Can you provide some numbers/measurements? ------------------------------------------------------------------------ May 8, 2005 - 11:37 : Dries Note: my measurements did not take into account the time spent searching the map. On the main page, we search this array 215 times. ------------------------------------------------------------------------ May 8, 2005 - 12:08 : puregin I believe that it's felt that adding path aliases would improve the Drupal documentation. This may well add 400+ paths to the URL aliases table on Drupal.org ------------------------------------------------------------------------ May 8, 2005 - 12:27 : Dries I spent some more time investigating this. The relevant code in bootstrap.inc is this: function drupal_get_path_alias($path) { if (($map = drupal_get_path_map()) && ($newpath = array_search($path, $map))) { return $newpath; } ... Turns out that the time to build the map is neglible (< 5ms, see previous comments), however the total time spent on 'path aliasing' it about 70ms per page! 50ms of these 70ms are spent on $map = drupal_get_path_map(). The remaining 20ms is spent on $newpath = array_search($path, $map). The first call to drupal_get_path_map() takes 3 to 5 ms, each subsequent call takes about 0.3 ms. Searching the array costs 0.1 ms. However, if you have to do this 130+ times, this adds up to a whopping 70 ms. Remind that we have 321 aliases in our map. As drupal_get_path_alias() is typically called from code>url(), which in turn is typically called from l() I set out to investigate the time spent in l(). Looks like l() easily gets called 120 times/page, and that each call to l() costs about 0.9 ms. That is about 2 or 3 times the cost of the /average SQL query/. The total time spent in l() is 115 ms! ------------------------------------------------------------------------ May 8, 2005 - 16:25 : adrian This is a complete aside, and probably off-topic, but I would like to mention that I was actually looking at rewriting url() to be able to handle 'external urls' , because I want to do aliases using the site subdomain, like for instance : http://category.sitename.com/article/title_goes_here -> node/23 Rewriting url to allow this would also allow us to use l() for external links of any kind, getting rid of a bunch of inline html. ------------------------------------------------------------------------ May 8, 2005 - 16:46 : chx adrian, I have written a patch for l to support external links but Ber said that a module like weblink shall handle those. ------------------------------------------------------------------------ May 8, 2005 - 17:03 : mikeryan I've got over 4000 aliases on Fenway Views - another pathauto user reported over 6000. I don't have a profiling tool to give timing data, but adding a quick counter shows that for the Fenway Views calendar [8], drupal_get_path_alias() is called 404 times and drupal_get_normal_path is called 227 times. And trust me, this page loads MUCH faster with my patch than it does with the path map. I think those SQL queries are a lot cheaper than one might expect... [8] http://fenway-views.com/calendar ------------------------------------------------------------------------ May 8, 2005 - 22:24 : mathias Attachment: http://drupal.org/files/issues/pathalias-with-caching.patch (5.62 KB) Some benchmarks, with MySQL and Drupal caching disabled. 2 SETS OF TESTS ======================================= 1) 500 nodes and aliases 2) 20,000 nodes and aliases 3 TYPES OF TESTS PER SET ======================================= 1) Baseline unmodified Drupal 2) The following change inside drupal_get_path_map(): - if (is_null($map)) { + if ($map === NULL) { $map = array(); // Make $map non-null in case no aliases are defined. 3) Pull aliases only when needed rather than loading the entire alias table. SET 1 - 500 Nodes and 500 Aliases using: ab -c 10 -n 100 [homepage] ======================================= Baseline Time taken for tests: 13.00 seconds Requests per second: 3.85 [#/sec] (mean) Transfer rate: 56.35 [Kbytes/sec] received is_null() VS NULL Time taken for tests: 12.463 seconds Requests per second: 4.01 [#/sec] (mean) Transfer rate: 57.63 [Kbytes/sec] received Per Instance Alias Lookup (32 additional queries) Time taken for tests: 11.502 seconds Requests per second: 4.35 [#/sec] (mean) Transfer rate: 63.30 [Kbytes/sec] received SET 2 - 20,000 Nodes and 20,000 Aliases using: ab -c 5 -n 50 [homepage] ======================================= Baseline Time taken for tests: 204.583 seconds Requests per second: 0.24 [#/sec] (mean) Transfer rate: 3.34 [Kbytes/sec] received is_null() VS NULL Time taken for tests: 127.629 seconds Requests per second: 0.39 [#/sec] (mean) Transfer rate: 5.35 [Kbytes/sec] received Per Instance Alias Lookup (32 additional queries) Time taken for tests: 28.788 seconds Requests per second: 1.74 [#/sec] (mean) Transfer rate: 23.59 [Kbytes/sec] received ANALYSIS ======================================= Converting 'is_null()' to '=== NULL' is a no brainer and results in a nice performance gain. And while per instance alias lookups also give a huge boost for site with thousands of aliases, they're benefits are entirely dependent on the number of system URLs and menu items visible per request. As noted above I tested the standard homepage which added 32 additional queries. A request for something like the admin interface would presumably show less benefits. I would've tested this, but I didn't know how to invoke an authenticated page request with 'ab'. A positive of this approach is we no longer would be storing all the aliases in memory. What other concerns do we have with this last approach? Am I properly testing the strain on the database? ------------------------------------------------------------------------ May 8, 2005 - 23:08 : Dries Can you try making the following changes? 1. To common.inc: function drupal_rebuild_path_map() { - drupal_get_path_map('rebuild'); + drupal_get_path_map(TRUE); } 2. To bootstrap.inc: -function drupal_get_path_map($action = '') { +function drupal_get_path_map($rebuild = FALSE) { static $map = NULL; - if ($action == 'rebuild') { + if ($rebuild) { $map = NULL; } That is, replace the string comparison with a boolean comparison. Not sure it is going to make a significant difference but it might be another micro-improvement. I'll test your patch shortly. ------------------------------------------------------------------------ May 9, 2005 - 06:21 : mathias Dries. Those changes had no real performance gain. 20,000 Nodes and 20,000 Aliases using: ab -c 5 -n 50 [homepage] ======================================= Baseline Time taken for tests: 204.583 seconds Requests per second: 0.24 [#/sec] (mean) Transfer rate: 3.34 [Kbytes/sec] received String VS Boolean Time taken for tests: 204.190 seconds Requests per second: 0.24 [#/sec] (mean) Transfer rate: 3.34 [Kbytes/sec] received ------------------------------------------------------------------------ May 9, 2005 - 07:01 : Dries Before this patch can be committed, we need to do more testing. I'd like to know how this behaves on sites with few or no path aliases, and on sites like drupal.org, with a modest amount of path aliases. The reason I'm asking is because MySQL is often the bottleneck and not PHP/Apache. This is the case on drupal.org, for example. The proposed patch moves some of the processing costs from PHP to MySQL. On drupal.org, the amount of SQL queries/page would double. Needless to say, this is somewhat scary. ;) More numbers would be appreciated. ------------------------------------------------------------------------ May 9, 2005 - 20:56 : mikeryan I'm curious, why test with MySQL caching disabled? Since much of the issue is the expense of making (potentially many) more queries, this wouldn't seem to reflect the performance gain in practice. The caching (compared to making the query each time) seems like overkill to me - with MySQL caching enabled, I would think this complicates the code for little (if any) performance gain. Did you do any profiling using my original (cacheless) patch? Thanks. ------------------------------------------------------------------------ May 9, 2005 - 23:23 : mathias In an ideal world everyone would be running a query cache but I wanted to see how this would hold up under the worst-case scenario. I tested the original patch which didn't use static variables and the amount of queries doubled. For example, to load the front page, Drupal queried the url_alias table 22 times looking for an alias for 'node'. There's no need to send duplicate queries to the DB since it seems that's where most bottlenecks seem to lie. ------------------------------------------------------------------------ May 10, 2005 - 16:41 : mikeryan re: worst-case scenario - I understand that. re: # queries - I would not want to assume adding to the number of queries is necessarily a significant performance hit (with database caching on, of course). I understand the suspicion - I've been around long enough to remember when you did everything you could to avoid making one single SQL query more than you absolutely had to. But modern databases (including MySQL) are very well optimized for this kind of application (lots and lots of small queries). And, I have seen real-life cases where adding caching of data that's already being cached somewhere upstream actually degrades performance. As long as we're examining this particular area of performance under a microscope, let's make sure we squeeze every millisecond of savings we can out of it - I'd just like to see the numbers for a cached database/no Drupal caching combination for comparison. Thanks. ------------------------------------------------------------------------ May 10, 2005 - 23:12 : mathias Dries asked me to do some tests using the drupal.org database. Once again it appears that plucking aliases out of the database as needed performed better than grabbing the entire alias table at the beginning of the request lifecycle, even if the alias table is small. Here's the numbers. using: ab -c 5 -n 50 [homepage] Drupal.org Baseline ============= Time taken for tests: 18.081 seconds Requests per second: 2.77 [#/sec] (mean) Transfer rate: 52.69 [Kbytes/sec] received 1 page request was 83 queries in 60ms for a cycle execution time of 255ms Drupal.org Per Instance Query ================== Time taken for tests: 16.536 seconds Requests per second: 3.02 [#/sec] (mean) Transfer rate: 56.63 [Kbytes/sec] received 1 page request was 125 queries in 65ms for a cycle execution time of 230ms Notice that the database does work a little harder to retrieve the queries one by one. And it also doesn't make sense to run 40 extra queries on a site with only 50 aliases. I think at this point we have a couple of options to explore: 1. Continue to try and squeeze out any other optimizations we can in the original path aliasing code. Maybe look at output buffering or a mechnism that'll allow us to start working the database resultset as soon data is fed into the pipe rather than waiting for the request to finish. 2. Try to identify the point (e.g., the number aliases) at which it becomes more effecient to fetch aliases and change patterns on the fly. As with most optimizations, the end result will probably be a combination of both. ------------------------------------------------------------------------ May 10, 2005 - 23:59 : Dries Looks like in all cases, performance actually improves. I've run a couple tests myself using the drupal.org database and measured performance improvements up to 15%. Of course, this was done on a machine where the MySQL database wasn't hammered on by 50+ concurrent requests. It would be appreciated if you could extend your patch with a couple lines of documentation to document the behavior (so people understand why we are doing it this way), the code and the API. Maybe add an entry to CHANGELOG.txt? Do we need an update for updates.inc? As for future work, I think we're better off looking at l(). As said, one call to l() takes about 0.9 ms and we easily do 100+ such calls per page. Caching at the level of l() is likely to buy us more but I don't know whether it is feasible. Maybe we can think of other optimizations too. If anything, it might be a good idea to add a TODO/NOTE to the PHPdoc of l(). Or, if you feel like messing with l() first, by all means! Count me in -- it's fun. :-)

1 0

[drupal-devel] Release critical bugs for May 12, 2005
by drupal-devel＠drupal.org 12 May '05

12 May '05

Bbcode / bug reports email spam filtering does not "see" email addresses when they are NOT preceded by a word age: 16 weeks 4 days url: http://drupal.org/node/15654 Drupal / bug reports Search memory limit problems after 4.6 upgrade age: 1 day 20 hours url: http://drupal.org/node/22497 Warning: Wrong parameter count for mysql_connect() assigned: jselva(a)drupal.org age: 5 days 9 hours url: http://drupal.org/node/22188 Teaser problem assigned: childhood age: 3 weeks 1 day url: http://drupal.org/node/20843 UTF-8 node title gets truncated at wrong position age: 3 weeks 5 days url: http://drupal.org/node/20537 database table "term_node" not updated when removing a term from a node under editing and submitting age: 4 weeks 2 days url: http://drupal.org/node/20315 Attached files can not be deleted age: 4 weeks 2 days url: http://drupal.org/node/20307 Reassigning a node to a different term does not work properly age: 4 weeks 3 days url: http://drupal.org/node/20278 4.6 aggregator showing/not interpreting HTML tags age: 5 weeks 4 days url: http://drupal.org/node/19874 Search ranking broken on pgsql age: 6 weeks 1 day url: http://drupal.org/node/19629 New submit error, non-admin user, postgresql database assigned: adrian age: 9 weeks 1 day url: http://drupal.org/node/18552 When I delete a user, any forum in which s/he has commented also disappears. assigned: Jill6q age: 12 weeks 8 hours url: http://drupal.org/node/17428 OPML Import for Aggregator assigned: arnabdotorg age: 14 weeks 6 days url: http://drupal.org/node/16282 Change to sesssion.inc violates UID database constraint age: 16 weeks 4 days url: http://drupal.org/node/15666 forum.module problem with postgresql v7.4.1 age: 17 weeks 2 days url: http://drupal.org/node/15411 PHP 5 gives error when session table has no session assigned: adschar age: 17 weeks 2 days url: http://drupal.org/node/15399 postgresql error age: 18 weeks 3 hours url: http://drupal.org/node/15175 Entering an empty title broke drupal age: 25 weeks 2 days url: http://drupal.org/node/12974 Argument NOT must be type boolean age: 25 weeks 2 days url: http://drupal.org/node/12950 Comments in approval queue still show up in tracker age: 29 weeks 4 days url: http://drupal.org/node/11647 Twin problems with comments assigned: Junyor age: 30 weeks 6 days url: http://drupal.org/node/11366 Prevent browser page caching of dynamic content assigned: mathias age: 1 year 12 weeks url: http://drupal.org/node/5900 Drupal / feature requests Add a footer that is specific to a particular book, shown only on certain pages age: 5 weeks 6 days url: http://drupal.org/node/19757 moving pages en masse age: 5 weeks 6 days url: http://drupal.org/node/19754 Taxonomy module should use nodeapi 'load' age: 9 weeks 8 hours url: http://drupal.org/node/18631 Auto PHP memory maximisation age: 11 weeks 2 days url: http://drupal.org/node/17663 Don't reset existing password on request, prevent DoS password reset abuse age: 13 weeks 3 days url: http://drupal.org/node/16909 User info exposed to anonymous user age: 23 weeks 5 days url: http://drupal.org/node/13545 Allow named anchors to work without specifying full path to node age: 1 year 24 weeks url: http://drupal.org/node/4213 Drupal / support requests Please help me out i am stuck here :( assigned: neerajshokeen age: 4 days 20 hours url: http://drupal.org/node/22227 Fatal error: Unknown column 'headers' in 'field list' query assigned: Eagle-i age: 8 weeks 1 day url: http://drupal.org/node/18928 Strange session behavior age: 8 weeks 3 days url: http://drupal.org/node/18821 user redirected to homepage when logging in age: 9 weeks 5 days url: http://drupal.org/node/18381 Problem with Forums age: 17 weeks 4 days url: http://drupal.org/node/15299 Drupal / tasks Consistency assigned: stefan nagtegaal age: 32 weeks 5 days url: http://drupal.org/node/11045 Event / bug reports search over fields.inc fields conceptual error age: 27 weeks 6 days url: http://drupal.org/node/12259 fields.inc screws up with multible multiselect fields age: 27 weeks 6 days url: http://drupal.org/node/12257 Still GMT/TZ issues with CVS age: 28 weeks 6 days url: http://drupal.org/node/11870 Time entry is GMT even when site default is not age: 29 weeks 4 days url: http://drupal.org/node/11630 Start Time of Event is NOT Preserved age: 46 weeks 6 days url: http://drupal.org/node/8565 Feature / bug reports Menu item not showing age: 31 weeks 5 days url: http://drupal.org/node/11210 File / bug reports segmentation fault related to filestore age: 1 year 32 weeks url: http://drupal.org/node/3001 Filestore / support requests File store module - Overwriting Filename....Help needed assigned: harishkm age: 1 year 5 weeks url: http://drupal.org/node/6827 Unserialize error at line 104 age: 1 year 12 weeks url: http://drupal.org/node/5857 Filestore module- sql errors rectified assigned: harishkm age: 1 year 13 weeks url: http://drupal.org/node/5619 Filestore Module Problems unaddressed assigned: harishkm age: 1 year 15 weeks url: http://drupal.org/node/5365 Gallery / bug reports Gallery error age: 2 weeks 2 days url: http://drupal.org/node/21290 Fatal Error: variable_get() age: 7 weeks 3 days url: http://drupal.org/node/19189 No Gallery Image Block age: 7 weeks 6 days url: http://drupal.org/node/19045 Mini applet can't find gallery at url / age: 8 weeks 4 days url: http://drupal.org/node/18785 Fatal error: Undefined class name 'galleryembed' age: 11 weeks 1 day url: http://drupal.org/node/17746 Gallery / support requests Can't Embed Gallery into Drupal age: 10 weeks 3 days url: http://drupal.org/node/18029 Groups / bug reports Drupal-4.4-rc breaks registration setups and groups.module age: 1 year 8 weeks url: http://drupal.org/node/6392 Image / bug reports Image module thumbnail/preview stop working after l10n age: 2 weeks 6 days url: http://drupal.org/node/21036 Cannot upload .jpgs age: 10 weeks 3 days url: http://drupal.org/node/18076 Wrongly formed html in image gallery age: 11 weeks 6 days url: http://drupal.org/node/17462 Image.Module does not know the correct filename age: 25 weeks 1 day url: http://drupal.org/node/13032 SAFE MODE Restriction in effect... age: 28 weeks 6 days url: http://drupal.org/node/11843 Uploading jpeg makes Imagemagick do high compression age: 30 weeks 3 days url: http://drupal.org/node/11445 Image module failing on Preview because of a blank node id age: 32 weeks 4 days url: http://drupal.org/node/11069 Similar problem with Directory Upload Slow age: 32 weeks 4 days url: http://drupal.org/node/11064 Image / support requests path problems on upgrading... age: 1 week 3 days url: http://drupal.org/node/21725 Node Aggregator / bug reports Please update Node_Aggregator age: 7 weeks 5 days url: http://drupal.org/node/19096 Import doesn't like XML/RSS feeds with no Title element age: 49 weeks 4 days url: http://drupal.org/node/8128 Node Aggregator / feature requests RSS items overwriting edited items age: 47 weeks 5 days url: http://drupal.org/node/8455 Notify / bug reports Organic Groups subscription notifications are being sent to *all* group members instead of just group manager age: 1 week 4 days url: http://drupal.org/node/21635 Notify / feature requests Notify.module ignores the true submission queue age: 1 year 28 weeks url: http://drupal.org/node/3765 Pdfview / feature requests Internationalization support for PDF assigned: TheLibrarian age: 1 year 6 weeks url: http://drupal.org/node/6795 Project / bug reports Date for latest is incorrect age: 3 weeks 6 days url: http://drupal.org/node/20485 Invalid link to release files age: 11 weeks 3 days url: http://drupal.org/node/17625 blank page after install age: 14 weeks 1 day url: http://drupal.org/node/16575 "Page Not Found" when trying to view issues or add new release age: 22 weeks 6 days url: http://drupal.org/node/13815 Project pages display only title and description age: 25 weeks 6 hours url: http://drupal.org/node/13082 Fatal error: Cannot use string offset as an array in includes/tablesort.inc on line 113 assigned: publian age: 31 weeks 6 days url: http://drupal.org/node/11187 Can't create new project with issue tracker age: 38 weeks 20 hours url: http://drupal.org/node/10148 Project / support requests Help with everything - nothing seems to work in the latest build age: 24 weeks 2 days url: http://drupal.org/node/13310 Series / bug reports Can't get the inline tag to show series age: 1 day 10 hours url: http://drupal.org/node/22520 Taxonomy dhtml / bug reports MySQL Error age: 6 weeks 6 days url: http://drupal.org/node/19413 javascrip downloads square.gif for every category age: 12 weeks 1 day url: http://drupal.org/node/17366 Weblink / bug reports drupal special chars error age: 3 weeks 3 days url: http://drupal.org/node/20687

1 0

[drupal-devel] [feature] Add timing information to statistics page
by moshe weitzman 12 May '05

12 May '05

Issue status update for http://drupal.org/node/22623 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: moshe weitzman Status: patch Now I realize that the Total and Average columns are using different units. Thats not ideal, but understandable in this case. I can't think of a better solution. moshe weitzman Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 15:05 : Dries Attachment: http://drupal.org/files/issues/timer.patch (8.56 KB) I extended the current timer_start() function in bootstrap.inc and added two new function, timer_read() and timer_stop(). I've used these to profile drupal.org. (A small change will be needed to devel.module.) I've also extended the accesslog-table to keep track of page generation times. The information is shown on the "top pages" page (admin/logs/pages). It shows each page's average page generation time, and the total page generation time. This lets one identify (i) slow pages and (ii) resource heavy pages. This is the most essential and most useful information when tuning your Drupal site. Please review, or I'll commit it as is. ------------------------------------------------------------------------ May 11, 2005 - 15:13 : Dries Attachment: http://drupal.org/files/issues/timer.pdf (22 KB) Screenshot. ------------------------------------------------------------------------ May 11, 2005 - 16:04 : Dries Setting to patch. Take a look at the patch + screenshot please. ------------------------------------------------------------------------ May 11, 2005 - 16:19 : moshe weitzman looks ok ... note that there are some unrelated (i think) variable_set() changes in the patch. ------------------------------------------------------------------------ May 11, 2005 - 16:20 : Amazon It's great to have this level of logging granularity. I am concerned that all the logging would add over head when having multiple crawlers crawling the site during testing. Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page. For example I routinely run several wget crawls for different roles when testing an upgrade. If I ran 5 crawlers and each of 10, 000+ pages were being crawled then I am concerned the performance would be impacted. I understand this isn't the intent of this patch but non intrusive logging is important in generating accurate performance measurements. Thank-you for your consideration. Kieran ------------------------------------------------------------------------ May 11, 2005 - 16:20 : moshe weitzman in the pdf, i think you are swapping 'total' and 'average' column headers ------------------------------------------------------------------------ May 11, 2005 - 16:34 : Bèr Kessels I like the idea. a lot. But from the pdf it is not clear that timer deals with the generation tilme o a page. * Are we not boing too geeky here (again)? devel.module does farily granual timing already. Is Joe User really waiting for timers on how long it took to make a page. I think not. * If we really want this in core, it should at least be made very clear that this is the time it took to make a page. I presented the screenshot to a client, as a small test. And just like i thought, his reaction was " Wow, this is handy, I can now see how long a user spent at a certain page. This will tell me what posts are actually read, and which are only opened in a browser only to be closed right away". I almost made the same mistake, but the geek in me told me to look at the code and see that it is page generation times. Joe user will not ever look at the code. * What is the penalty. accesslog already is a heavy load table. Do we really want aditional data in that table. * We have access hooks (called after page is loaded) to do aditional statistics magic. IT is not userd by anything else then the stats in core. Why cant this go into an advanced stats module? ------------------------------------------------------------------------ May 11, 2005 - 17:03 : Dries Moshe: the column names are not swapped. What makes you think they are? What can be done to make it more clear? Moshe: the variable_get() was removed intentionally. The 'page' timer is always started. It's cheap. Ber: I'm aware of the possible confusion and I agree that we need to clarify this. Unfortunately, I couldn't think of better column names. The problem is that these names should be short (or the table will look ugly). Suggestions? Ber: the devel.module does not provide this functionality. It doesn't track page generation times for other users, it doesn't compute averages, nor does it compute the total time spent generating a particular page. The devel.module doesn't have an overview. This is important information for anyone who has to tune his or her Drupal site. Without this patch, you simply don't know which pages are eating your resources. Ber, Kieran: the performance impact is small. The timer values are stored as integers and fit in 2 bytes. So if there are 200.000 entries in your accesslog-table like on drupal.org, the database overhead is 400kb (a fraction of the table's total size). No additional SQL queries are introduced. As such, performance remains comparable with the current implementation. I can benchmark this if you want but I'm confident it is OK. I could easily compile a list of the users/IPs that eat most resources. This can help us track evil crawlers. I'm not sure yet how or where to integrate this UI-wise so I haven't (yet). One step at a time. Making it easy to inspect the performance characteristics of your Drupal site is a good thing. It creates awareness which in turn will encourage people to tune and tweak Drupal's performance. It also lets you identify problems -- like a module performing badly. Compare it with your operating system's "activity/process monitor" (or "top"). You don't really need it, but you can't without it either. ------------------------------------------------------------------------ May 11, 2005 - 17:35 : Jeremy(a)kerneltrap.org I love the idea very much, and think this will prove extremely useful. What about making an option that enables/disables these columns? The only reason I suggest this is because they are inherently confusing without using a lot of verbage. "Average time" needs to be something like "Average time for Drupal engine to generate this page", and "Total time" needs to be something like "Total time to generate". Or, it could be fully explained at the configurable that turns the display on and off...? ""I could easily compile a list of the users/IPs that eat most resources." " Yes, do it! :) That would be very useful. ""Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page." " Where would the data be stored until it was committed to the table? It would be possible to do something similar to what the banner module does, writing to multiple cache files. But this isn't friendly for distributed web servers. Do writes to the accesslog slow down measurably when the acceslog table grows? If so, then there could be an accesslog_cache table to which data is first written, and which is flushed to the main accesslog every n minutes, or every time it reaches a certain size, etc. This would only be useful if writing to a very large table is measurably slower than writing to a very small table. ------------------------------------------------------------------------ May 11, 2005 - 17:39 : kbahey Useful info for sure. I am with Jeremy that this needs to be parameterized. Also, are these figures real? Does the average time to generate the forum page is 30 seconds? I find this hard to swallow.

1 0

[drupal-devel] [feature] Add timing information to statistics page
by kbahey 12 May '05

12 May '05

Issue status update for http://drupal.org/node/22623 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: kbahey Status: patch Useful info for sure. I am with Jeremy that this needs to be parameterized. Also, are these figures real? Does the average time to generate the forum page is 30 seconds? I find this hard to swallow. kbahey Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 15:05 : Dries Attachment: http://drupal.org/files/issues/timer.patch (8.56 KB) I extended the current timer_start() function in bootstrap.inc and added two new function, timer_read() and timer_stop(). I've used these to profile drupal.org. (A small change will be needed to devel.module.) I've also extended the accesslog-table to keep track of page generation times. The information is shown on the "top pages" page (admin/logs/pages). It shows each page's average page generation time, and the total page generation time. This lets one identify (i) slow pages and (ii) resource heavy pages. This is the most essential and most useful information when tuning your Drupal site. Please review, or I'll commit it as is. ------------------------------------------------------------------------ May 11, 2005 - 15:13 : Dries Attachment: http://drupal.org/files/issues/timer.pdf (22 KB) Screenshot. ------------------------------------------------------------------------ May 11, 2005 - 16:04 : Dries Setting to patch. Take a look at the patch + screenshot please. ------------------------------------------------------------------------ May 11, 2005 - 16:19 : moshe weitzman looks ok ... note that there are some unrelated (i think) variable_set() changes in the patch. ------------------------------------------------------------------------ May 11, 2005 - 16:20 : Amazon It's great to have this level of logging granularity. I am concerned that all the logging would add over head when having multiple crawlers crawling the site during testing. Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page. For example I routinely run several wget crawls for different roles when testing an upgrade. If I ran 5 crawlers and each of 10, 000+ pages were being crawled then I am concerned the performance would be impacted. I understand this isn't the intent of this patch but non intrusive logging is important in generating accurate performance measurements. Thank-you for your consideration. Kieran ------------------------------------------------------------------------ May 11, 2005 - 16:20 : moshe weitzman in the pdf, i think you are swapping 'total' and 'average' column headers ------------------------------------------------------------------------ May 11, 2005 - 16:34 : Bèr Kessels I like the idea. a lot. But from the pdf it is not clear that timer deals with the generation tilme o a page. * Are we not boing too geeky here (again)? devel.module does farily granual timing already. Is Joe User really waiting for timers on how long it took to make a page. I think not. * If we really want this in core, it should at least be made very clear that this is the time it took to make a page. I presented the screenshot to a client, as a small test. And just like i thought, his reaction was " Wow, this is handy, I can now see how long a user spent at a certain page. This will tell me what posts are actually read, and which are only opened in a browser only to be closed right away". I almost made the same mistake, but the geek in me told me to look at the code and see that it is page generation times. Joe user will not ever look at the code. * What is the penalty. accesslog already is a heavy load table. Do we really want aditional data in that table. * We have access hooks (called after page is loaded) to do aditional statistics magic. IT is not userd by anything else then the stats in core. Why cant this go into an advanced stats module? ------------------------------------------------------------------------ May 11, 2005 - 17:03 : Dries Moshe: the column names are not swapped. What makes you think they are? What can be done to make it more clear? Moshe: the variable_get() was removed intentionally. The 'page' timer is always started. It's cheap. Ber: I'm aware of the possible confusion and I agree that we need to clarify this. Unfortunately, I couldn't think of better column names. The problem is that these names should be short (or the table will look ugly). Suggestions? Ber: the devel.module does not provide this functionality. It doesn't track page generation times for other users, it doesn't compute averages, nor does it compute the total time spent generating a particular page. The devel.module doesn't have an overview. This is important information for anyone who has to tune his or her Drupal site. Without this patch, you simply don't know which pages are eating your resources. Ber, Kieran: the performance impact is small. The timer values are stored as integers and fit in 2 bytes. So if there are 200.000 entries in your accesslog-table like on drupal.org, the database overhead is 400kb (a fraction of the table's total size). No additional SQL queries are introduced. As such, performance remains comparable with the current implementation. I can benchmark this if you want but I'm confident it is OK. I could easily compile a list of the users/IPs that eat most resources. This can help us track evil crawlers. I'm not sure yet how or where to integrate this UI-wise so I haven't (yet). One step at a time. Making it easy to inspect the performance characteristics of your Drupal site is a good thing. It creates awareness which in turn will encourage people to tune and tweak Drupal's performance. It also lets you identify problems -- like a module performing badly. Compare it with your operating system's "activity/process monitor" (or "top"). You don't really need it, but you can't without it either. ------------------------------------------------------------------------ May 11, 2005 - 17:35 : Jeremy(a)kerneltrap.org I love the idea very much, and think this will prove extremely useful. What about making an option that enables/disables these columns? The only reason I suggest this is because they are inherently confusing without using a lot of verbage. "Average time" needs to be something like "Average time for Drupal engine to generate this page", and "Total time" needs to be something like "Total time to generate". Or, it could be fully explained at the configurable that turns the display on and off...? ""I could easily compile a list of the users/IPs that eat most resources." " Yes, do it! :) That would be very useful. ""Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page." " Where would the data be stored until it was committed to the table? It would be possible to do something similar to what the banner module does, writing to multiple cache files. But this isn't friendly for distributed web servers. Do writes to the accesslog slow down measurably when the acceslog table grows? If so, then there could be an accesslog_cache table to which data is first written, and which is flushed to the main accesslog every n minutes, or every time it reaches a certain size, etc. This would only be useful if writing to a very large table is measurably slower than writing to a very small table.

1 0

[drupal-devel] [feature] Add timing information to statistics page
by Jeremy 12 May '05

12 May '05

Issue status update for http://drupal.org/node/22623 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: Jeremy(a)kerneltrap.org Status: patch I love the idea very much, and think this will prove extremely useful. What about making an option that enables/disables these columns? The only reason I suggest this is because they are inherently confusing without using a lot of verbage. "Average time" needs to be something like "Average time for Drupal engine to generate this page", and "Total time" needs to be something like "Total time to generate". Or, it could be fully explained at the configurable that turns the display on and off...? ""I could easily compile a list of the users/IPs that eat most resources." " Yes, do it! :) That would be very useful. ""Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page." " Where would the data be stored until it was committed to the table? It would be possible to do something similar to what the banner module does, writing to multiple cache files. But this isn't friendly for distributed web servers. Do writes to the accesslog slow down measurably when the acceslog table grows? If so, then there could be an accesslog_cache table to which data is first written, and which is flushed to the main accesslog every n minutes, or every time it reaches a certain size, etc. This would only be useful if writing to a very large table is measurably slower than writing to a very small table. Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 15:05 : Dries Attachment: http://drupal.org/files/issues/timer.patch (8.56 KB) I extended the current timer_start() function in bootstrap.inc and added two new function, timer_read() and timer_stop(). I've used these to profile drupal.org. (A small change will be needed to devel.module.) I've also extended the accesslog-table to keep track of page generation times. The information is shown on the "top pages" page (admin/logs/pages). It shows each page's average page generation time, and the total page generation time. This lets one identify (i) slow pages and (ii) resource heavy pages. This is the most essential and most useful information when tuning your Drupal site. Please review, or I'll commit it as is. ------------------------------------------------------------------------ May 11, 2005 - 15:13 : Dries Attachment: http://drupal.org/files/issues/timer.pdf (22 KB) Screenshot. ------------------------------------------------------------------------ May 11, 2005 - 16:04 : Dries Setting to patch. Take a look at the patch + screenshot please. ------------------------------------------------------------------------ May 11, 2005 - 16:19 : moshe weitzman looks ok ... note that there are some unrelated (i think) variable_set() changes in the patch. ------------------------------------------------------------------------ May 11, 2005 - 16:20 : Amazon It's great to have this level of logging granularity. I am concerned that all the logging would add over head when having multiple crawlers crawling the site during testing. Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page. For example I routinely run several wget crawls for different roles when testing an upgrade. If I ran 5 crawlers and each of 10, 000+ pages were being crawled then I am concerned the performance would be impacted. I understand this isn't the intent of this patch but non intrusive logging is important in generating accurate performance measurements. Thank-you for your consideration. Kieran ------------------------------------------------------------------------ May 11, 2005 - 16:20 : moshe weitzman in the pdf, i think you are swapping 'total' and 'average' column headers ------------------------------------------------------------------------ May 11, 2005 - 16:34 : Bèr Kessels I like the idea. a lot. But from the pdf it is not clear that timer deals with the generation tilme o a page. * Are we not boing too geeky here (again)? devel.module does farily granual timing already. Is Joe User really waiting for timers on how long it took to make a page. I think not. * If we really want this in core, it should at least be made very clear that this is the time it took to make a page. I presented the screenshot to a client, as a small test. And just like i thought, his reaction was " Wow, this is handy, I can now see how long a user spent at a certain page. This will tell me what posts are actually read, and which are only opened in a browser only to be closed right away". I almost made the same mistake, but the geek in me told me to look at the code and see that it is page generation times. Joe user will not ever look at the code. * What is the penalty. accesslog already is a heavy load table. Do we really want aditional data in that table. * We have access hooks (called after page is loaded) to do aditional statistics magic. IT is not userd by anything else then the stats in core. Why cant this go into an advanced stats module? ------------------------------------------------------------------------ May 11, 2005 - 17:03 : Dries Moshe: the column names are not swapped. What makes you think they are? What can be done to make it more clear? Moshe: the variable_get() was removed intentionally. The 'page' timer is always started. It's cheap. Ber: I'm aware of the possible confusion and I agree that we need to clarify this. Unfortunately, I couldn't think of better column names. The problem is that these names should be short (or the table will look ugly). Suggestions? Ber: the devel.module does not provide this functionality. It doesn't track page generation times for other users, it doesn't compute averages, nor does it compute the total time spent generating a particular page. The devel.module doesn't have an overview. This is important information for anyone who has to tune his or her Drupal site. Without this patch, you simply don't know which pages are eating your resources. Ber, Kieran: the performance impact is small. The timer values are stored as integers and fit in 2 bytes. So if there are 200.000 entries in your accesslog-table like on drupal.org, the database overhead is 400kb (a fraction of the table's total size). No additional SQL queries are introduced. As such, performance remains comparable with the current implementation. I can benchmark this if you want but I'm confident it is OK. I could easily compile a list of the users/IPs that eat most resources. This can help us track evil crawlers. I'm not sure yet how or where to integrate this UI-wise so I haven't (yet). One step at a time. Making it easy to inspect the performance characteristics of your Drupal site is a good thing. It creates awareness which in turn will encourage people to tune and tweak Drupal's performance. It also lets you identify problems -- like a module performing badly. Compare it with your operating system's "activity/process monitor" (or "top"). You don't really need it, but you can't without it either.

1 0

[drupal-devel] [feature] Add timing information to statistics page
by Dries 12 May '05

12 May '05

Issue status update for http://drupal.org/node/22623 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: Dries Status: patch Moshe: the column names are not swapped. What makes you think they are? What can be done to make it more clear? Moshe: the variable_get() was removed intentionally. The 'page' timer is always started. It's cheap. Ber: I'm aware of the possible confusion and I agree that we need to clarify this. Unfortunately, I couldn't think of better column names. The problem is that these names should be short (or the table will look ugly). Suggestions? Ber: the devel.module does not provide this functionality. It doesn't track page generation times for other users, it doesn't compute averages, nor does it compute the total time spent generating a particular page. The devel.module doesn't have an overview. This is important information for anyone who has to tune his or her Drupal site. Without this patch, you simply don't know which pages are eating your resources. Ber, Kieran: the performance impact is small. The timer values are stored as integers and fit in 2 bytes. So if there are 200.000 entries in your accesslog-table like on drupal.org, the database overhead is 400kb (a fraction of the table's total size). No additional SQL queries are introduced. As such, performance remains comparable with the current implementation. I can benchmark this if you want but I'm confident it is OK. I could easily compile a list of the users/IPs that eat most resources. This can help us track evil crawlers. I'm not sure yet how or where to integrate this UI-wise so I haven't (yet). One step at a time. Making it easy to inspect the performance characteristics of your Drupal site is a good thing. It creates awareness which in turn will encourage people to tune and tweak Drupal's performance. It also lets you identify problems -- like a module performing badly. Compare it with your operating system's "activity/process monitor" (or "top"). You don't really need it, but you can't without it either. Dries Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 22:05 : Dries Attachment: http://drupal.org/files/issues/timer.patch (8.56 KB) I extended the current timer_start() function in bootstrap.inc and added two new function, timer_read() and timer_stop(). I've used these to profile drupal.org. (A small change will be needed to devel.module.) I've also extended the accesslog-table to keep track of page generation times. The information is shown on the "top pages" page (admin/logs/pages). It shows each page's average page generation time, and the total page generation time. This lets one identify (i) slow pages and (ii) resource heavy pages. This is the most essential and most useful information when tuning your Drupal site. Please review, or I'll commit it as is. ------------------------------------------------------------------------ May 11, 2005 - 22:13 : Dries Attachment: http://drupal.org/files/issues/timer.pdf (22 KB) Screenshot. ------------------------------------------------------------------------ May 11, 2005 - 23:04 : Dries Setting to patch. Take a look at the patch + screenshot please. ------------------------------------------------------------------------ May 11, 2005 - 23:19 : moshe weitzman looks ok ... note that there are some unrelated (i think) variable_set() changes in the patch. ------------------------------------------------------------------------ May 11, 2005 - 23:20 : Amazon It's great to have this level of logging granularity. I am concerned that all the logging would add over head when having multiple crawlers crawling the site during testing. Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page. For example I routinely run several wget crawls for different roles when testing an upgrade. If I ran 5 crawlers and each of 10, 000+ pages were being crawled then I am concerned the performance would be impacted. I understand this isn't the intent of this patch but non intrusive logging is important in generating accurate performance measurements. Thank-you for your consideration. Kieran ------------------------------------------------------------------------ May 11, 2005 - 23:20 : moshe weitzman in the pdf, i think you are swapping 'total' and 'average' column headers ------------------------------------------------------------------------ May 11, 2005 - 23:34 : Bèr Kessels I like the idea. a lot. But from the pdf it is not clear that timer deals with the generation tilme o a page. * Are we not boing too geeky here (again)? devel.module does farily granual timing already. Is Joe User really waiting for timers on how long it took to make a page. I think not. * If we really want this in core, it should at least be made very clear that this is the time it took to make a page. I presented the screenshot to a client, as a small test. And just like i thought, his reaction was " Wow, this is handy, I can now see how long a user spent at a certain page. This will tell me what posts are actually read, and which are only opened in a browser only to be closed right away". I almost made the same mistake, but the geek in me told me to look at the code and see that it is page generation times. Joe user will not ever look at the code. * What is the penalty. accesslog already is a heavy load table. Do we really want aditional data in that table. * We have access hooks (called after page is loaded) to do aditional statistics magic. IT is not userd by anything else then the stats in core. Why cant this go into an advanced stats module?

1 0

[drupal-devel] [feature] Add timing information to statistics page
by Bèr Kessels 11 May '05

11 May '05

Issue status update for http://drupal.org/node/22623 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: Bèr Kessels Status: patch I like the idea. a lot. But from the pdf it is not clear that timer deals with the generation tilme o a page. * Are we not boing too geeky here (again)? devel.module does farily granual timing already. Is Joe User really waiting for timers on how long it took to make a page. I think not. * If we really want this in core, it should at least be made very clear that this is the time it took to make a page. I presented the screenshot to a client, as a small test. And just like i thought, his reaction was " Wow, this is handy, I can now see how long a user spent at a certain page. This will tell me what posts are actually read, and which are only opened in a browser only to be closed right away". I almost made the same mistake, but the geek in me told me to look at the code and see that it is page generation times. Joe user will not ever look at the code. * What is the penalty. accesslog already is a heavy load table. Do we really want aditional data in that table. * We have access hooks (called after page is loaded) to do aditional statistics magic. IT is not userd by anything else then the stats in core. Why cant this go into an advanced stats module? Bèr Kessels Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 21:05 : Dries Attachment: http://drupal.org/files/issues/timer.patch (8.56 KB) I extended the current timer_start() function in bootstrap.inc and added two new function, timer_read() and timer_stop(). I've used these to profile drupal.org. (A small change will be needed to devel.module.) I've also extended the accesslog-table to keep track of page generation times. The information is shown on the "top pages" page (admin/logs/pages). It shows each page's average page generation time, and the total page generation time. This lets one identify (i) slow pages and (ii) resource heavy pages. This is the most essential and most useful information when tuning your Drupal site. Please review, or I'll commit it as is. ------------------------------------------------------------------------ May 11, 2005 - 21:13 : Dries Attachment: http://drupal.org/files/issues/timer.pdf (22 KB) Screenshot. ------------------------------------------------------------------------ May 11, 2005 - 22:04 : Dries Setting to patch. Take a look at the patch + screenshot please. ------------------------------------------------------------------------ May 11, 2005 - 22:19 : moshe weitzman looks ok ... note that there are some unrelated (i think) variable_set() changes in the patch. ------------------------------------------------------------------------ May 11, 2005 - 22:20 : Amazon It's great to have this level of logging granularity. I am concerned that all the logging would add over head when having multiple crawlers crawling the site during testing. Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page. For example I routinely run several wget crawls for different roles when testing an upgrade. If I ran 5 crawlers and each of 10, 000+ pages were being crawled then I am concerned the performance would be impacted. I understand this isn't the intent of this patch but non intrusive logging is important in generating accurate performance measurements. Thank-you for your consideration. Kieran ------------------------------------------------------------------------ May 11, 2005 - 22:20 : moshe weitzman in the pdf, i think you are swapping 'total' and 'average' column headers

1 0

[drupal-devel] [feature] Add timing information to statistics page
by moshe weitzman 11 May '05

11 May '05

Issue status update for http://drupal.org/node/22623 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: moshe weitzman Status: patch in the pdf, i think you are swapping 'total' and 'average' column headers moshe weitzman Previous comments: ------------------------------------------------------------------------ May 11, 2005 - 15:05 : Dries Attachment: http://drupal.org/files/issues/timer.patch (8.56 KB) I extended the current timer_start() function in bootstrap.inc and added two new function, timer_read() and timer_stop(). I've used these to profile drupal.org. (A small change will be needed to devel.module.) I've also extended the accesslog-table to keep track of page generation times. The information is shown on the "top pages" page (admin/logs/pages). It shows each page's average page generation time, and the total page generation time. This lets one identify (i) slow pages and (ii) resource heavy pages. This is the most essential and most useful information when tuning your Drupal site. Please review, or I'll commit it as is. ------------------------------------------------------------------------ May 11, 2005 - 15:13 : Dries Attachment: http://drupal.org/files/issues/timer.pdf (22 KB) Screenshot. ------------------------------------------------------------------------ May 11, 2005 - 16:04 : Dries Setting to patch. Take a look at the patch + screenshot please. ------------------------------------------------------------------------ May 11, 2005 - 16:19 : moshe weitzman looks ok ... note that there are some unrelated (i think) variable_set() changes in the patch. ------------------------------------------------------------------------ May 11, 2005 - 16:20 : Amazon It's great to have this level of logging granularity. I am concerned that all the logging would add over head when having multiple crawlers crawling the site during testing. Would it be possible to support batching the log entries and submitting them to the database as batch query at set increments instead of for every page. For example I routinely run several wget crawls for different roles when testing an upgrade. If I ran 5 crawlers and each of 10, 000+ pages were being crawled then I am concerned the performance would be impacted. I understand this isn't the intent of this patch but non intrusive logging is important in generating accurate performance measurements. Thank-you for your consideration. Kieran

1 0