- Development - lists.drupal.org

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

08 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 Reported by: mikeryan Updated by: Dries Status: patch Note: my measurements did not take into account the time spent searching the map. On the main page, we search this array 215 times. 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?

1 0

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

08 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 Reported by: mikeryan Updated by: Dries Status: patch 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? 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!

1 0

[drupal-devel] [task] Performance issues with path aliases
by mikeryan 08 May '05

08 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 Reported by: mikeryan Updated by: mikeryan Status: patch Great, thanks! mikeryan Previous comments: ------------------------------------------------------------------------ May 4, 2005 - 20: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 4, 2005 - 21: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 - 21:02 : mikeryan Attachment: http://drupal.org/files/issues/common.inc_11.patch (723 bytes) One attachment per note, that's tedious... ------------------------------------------------------------------------ May 4, 2005 - 21:03 : mikeryan Attachment: http://drupal.org/files/issues/path.module_0.patch (1.34 KB) ------------------------------------------------------------------------ May 4, 2005 - 21:03 : mikeryan Attachment: http://drupal.org/files/issues/database.mysql_4.patch (553 bytes) ------------------------------------------------------------------------ May 4, 2005 - 21:04 : mikeryan Attachment: http://drupal.org/files/issues/database.pgsql_2.patch (502 bytes) ------------------------------------------------------------------------ May 4, 2005 - 21: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 - 18: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 - 08: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 - 12: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 - 12: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.

1 0

[drupal-devel] [task] Performance issues with path aliases
by killes 08 May '05

08 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 Reported by: mikeryan Updated by: killes(a)www.drop.org Status: patch 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. killes(a)www.drop.org 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

1 0

[drupal-devel] [task] Performance issues with path aliases
by mikeryan 08 May '05

08 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 Reported by: mikeryan Updated by: mikeryan Status: patch 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 [1], 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)... [1] http://fenway-views.com/calendar mikeryan Previous comments: ------------------------------------------------------------------------ May 4, 2005 - 20:10 : mikeryan See Investigate use of conf_url_rewrite [2] 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? [2] http://drupal.org/node/21938 ------------------------------------------------------------------------ May 4, 2005 - 21: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 - 21:02 : mikeryan Attachment: http://drupal.org/files/issues/common.inc_11.patch (723 bytes) One attachment per note, that's tedious... ------------------------------------------------------------------------ May 4, 2005 - 21:03 : mikeryan Attachment: http://drupal.org/files/issues/path.module_0.patch (1.34 KB) ------------------------------------------------------------------------ May 4, 2005 - 21:03 : mikeryan Attachment: http://drupal.org/files/issues/database.mysql_4.patch (553 bytes) ------------------------------------------------------------------------ May 4, 2005 - 21:04 : mikeryan Attachment: http://drupal.org/files/issues/database.pgsql_2.patch (502 bytes) ------------------------------------------------------------------------ May 4, 2005 - 21: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 - 18:13 : mikeryan I upgraded Fenway Views [3] to Drupal 4.6.0 today, incorporating these patches. Performance is very noticeably improved. [3] http://fenway-views.com/ ------------------------------------------------------------------------ May 8, 2005 - 08: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.

1 0

[drupal-devel] [bug] theme_image doesn't accept html attributes
by jjeff 08 May '05

08 May '05

Issue status update for http://drupal.org/node/21397 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: jjeff Updated by: jjeff -Status: fixed +Status: patch Attachment: http://drupal.org/files/issues/theme-img_attr_fix.patch (1 KB) Oops... the new fix sets the default value for $attributes to NULL. It should at least be array(), now that $attributes is an array. This patch fixes the default value and adds some other error checking that should help this error that people may be seeing when they load a page with images: Invalid argument supplied for foreach() in /Users/jeff/Sites/drupal/includes/common.inc on line 1536 -Jeff jjeff Previous comments: ------------------------------------------------------------------------ April 26, 2005 - 21:45 : jjeff Here's the theme_image function from theme.inc <?php function theme_image($path, $alt = '', $title = '', $attr = '', $getsize = true) { if (!$getsize || (file_exists($path) && (list($width, $height, $type, $attr) = @getimagesize($path)))) { return "<img src=\"$path\" $attr alt=\"$alt\" title=\"$title\" />"; } } ?> Any arguments sent to the function for $attr get clobbered by the getimagesize() call. And $getsize defaults to true, so in a call to: <?php theme_image('files/images/foo', 'alt_text', 'title_text', 'align="left"') ?> the 'align="left"' never makes it to the page. The two attribute values really should be able to coexist, allowing for both getsize attributes and additional 'argumented' attributes as well. I recommend changing the $attr in the second line to $imgattr, then adding $imgattr to the tag returned in the third. -Jeff ------------------------------------------------------------------------ May 2, 2005 - 10:44 : jjeff Attachment: http://drupal.org/files/issues/theme_image-attr_fix.patch (1.51 KB) Okay, here's a patch to fix the theme_image attribute problem. I've also added a handler so that attributes can either be a string (as they were in the past - although it wasn't working) or an associative array. I've marked this issue as critical because the current version of theme_image() does not accept attributes at all. -Jeff ------------------------------------------------------------------------ May 2, 2005 - 10:47 : jjeff Attachment: http://drupal.org/files/issues/theme_image-attr_fix_0.patch (1.51 KB) Whoops... Found a mistake in my code... Here's the correction. -Jeff ------------------------------------------------------------------------ May 2, 2005 - 11:04 : jjeff Attachment: http://drupal.org/files/issues/theme_image-attr_fix_1.patch (1.51 KB) It's amazing how many mistakes one person can make in two lines of code. New patch... this one works! -jeff ------------------------------------------------------------------------ May 2, 2005 - 11:16 : Steven Attributes should be consistently passed as an array, and rendered with drupal_attributes(). This is to ensure data is escaped correctly. ------------------------------------------------------------------------ May 5, 2005 - 10:09 : jjeff Attachment: http://drupal.org/files/issues/theme_image-attr_fix_2.patch (1.34 KB) Now the $attr argument requires an array. And the $attr argument gets sent through drupal_attributes(). Simple and easy, really. -Jeff ------------------------------------------------------------------------ May 6, 2005 - 04:02 : Dries Tidied up the code a little, and committed it to HEAD and DRUPAL-4-6. Thanks.

1 0

[drupal-devel] [task] Performance issues with path aliases
by killes 08 May '05

08 May '05

Issue status update for http://drupal.org/node/22035 Project: Drupal -Version: 4.6.0 +Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Anonymous Reported by: mikeryan Updated by: killes(a)www.drop.org -Status: active +Status: patch 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. killes(a)www.drop.org 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/

1 0

[drupal-devel] [feature] Order taxonomy terms by vocabulary weight, then term weight
by grohk 08 May '05

08 May '05

Issue status update for http://drupal.org/node/7684 Project: Drupal Version: cvs Component: taxonomy.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: MrRoot Updated by: grohk -Status: active +Status: patch Attachment: http://drupal.org/files/issues/taxonomy_vocab_weight.patch (901 bytes) I have been looking for this funtionality for a long time, so I have attempted to bring this patch up-to-date and clean it up as per the suggestions. grohk Previous comments: ------------------------------------------------------------------------ May 10, 2004 - 21:33 : MrRoot Attachment: http://drupal.org/files/issues/taxonomy.module_2.patch (866 bytes) This is a trivial patch to the taxonomy_node_get_terms() function. Currently, it was only sorting the results by term weight; the patch joins in the vocabulary table and adds the vocabulary weight as a second preference ordering. I wrote this because I wanted to force ordering of vocabularies when listing the taxonomies of a node using the taxonomy_link() function. Basically, what I was doing was this: I created a "Story Type" vocab which described the overall *type* of post ( News, Rant, Review, ... ) and a "Topic" vocab which described the *topic* of the post ( Programming, Math, Political, Family, ... ). Now, when you added a new story, you picked a Story Type and a Topic. When the story is displayed, it shows the taxonomy assignments as a list of links. The ordering of that list was by term weight, term name. That meant that if I wanted to control the left to right layout of that information, I'd have to set the weight on every term in each vocabulary. That seemed tedious, especially since the vocabularies themselves already had a weight associated with them. Hence, I dug around for about 15 minutes and wrote this patch. It's been tested and seems to work quite well. I can now *force* which taxonomy vocabulary is listed when, and thereby always make the posts appear as "Story Type | Topic". Hope it's useful.. :) ------------------------------------------------------------------------ May 10, 2004 - 21:40 : MrRoot Ack - that patchfile was generated against the current CVS tree, so I suppose the Version should have been 'cvs'? Sorry about that if it was misplaced. ------------------------------------------------------------------------ May 10, 2004 - 22:48 : moshe weitzman this patch makes a lot of sense ... however, your sql is non standard for drupal. we use the x JOIN y ON x.field = y.field syntax. Your SQL doe snot use this JOIN syntax. Please resubmit and then I'll bet that this will be accepted into core. ------------------------------------------------------------------------ May 10, 2004 - 23:47 : MrRoot Attachment: http://drupal.org/files/issues/taxonomy.module_3.patch (872 bytes) Ah.. I was wondering about that.. Looks like most of the rest of taxonomy.module does not make use of JOIN statements. So I was following that. Anyhow, I updated the patch to use JOIN on all of the tables. Cheers ------------------------------------------------------------------------ May 11, 2004 - 16:15 : Dries Is that ANSI SQL? If these are 'left' joins, please use 'LEFT JOIN'. If these are 'inner' joins, please use 'INNER JOIN'. Also, I'd add a small comment to code to explain the rationale behind that query. ------------------------------------------------------------------------ August 12, 2004 - 11:28 : killes(a)www.drop.org Patch doesn't apply.

1 0

[drupal-devel] [bug] Missing ) in locale.inc
by robertDouglass 08 May '05

08 May '05

Issue status update for http://drupal.org/node/22307 Project: Drupal Version: cvs Component: locale.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: robertDouglass Updated by: robertDouglass Status: patch Attachment: http://drupal.org/files/issues/locale.inc.patch.txt (1.11 KB) missing ) - syntax error. patch might not apply (was full of false character recognition problems), so please just update manually in that case. robertDouglass

2 1

[drupal-devel] PHPTemplate hits core ... finally
by Dries Buytaert 08 May '05

08 May '05

Some changes: 1. I committed the PHPTemplate engine to core and removed the XTemplate engine from core. 2. I replaced the XTemplate-based version of Bluemarine with the PHPTemplate-based version. 3. I moved the XTemplate engine and the Pushbutton theme to the contributions repository. (For almost 2 years XTemplate has been part of Drupal core.) Some TODO-list items: 1. Figure out the menu configuration. High priority. 2. Add one more PHPTemplate-based theme to core. Medium priority. Thanks Adrian et al, -- Dries Buytaert :: http://www.buytaert.net/

15 35