Because it doesn't make any sense, and because it leads to 70 pointless function calls in vanilla head, a lot more on real sites.

CommentFileSizeAuthor
#8 572452-8-replace-two-instances-of-drupal_static.patch1010 bytesAnonymous (not verified)
#4 head.png512.3 KBcatch
#4 patch.png531 KBcatch
#3 drupal_static.patch1.13 KBcatch
#3 drupal_static.patch1.13 KBcatch
#1 drupal_static.patch1.12 KBcatch
drupal_get_filename.patch738 bytescatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
Priority: Minor » Normal
StatusFileSize
new drupal_static.patch1.12 KB

Same thing with drupal_load() - attached patch does both. This is in the critical path for any full Drupal bootstrap, so back to normal status

c960657’s picture

c960657 commented
Status: Needs review » Needs work 
+  // Whether a file is loaded or not can not change during a request, so do
+  // not use drupal_static() here.

This doesn't sound right. Whether a file is loaded can change from false to true, but not the other way around.

The code looks good, though. It is only wording in the comment, I am objecting to.

catch’s picture

catch
he/him
Primary language English
commented
Title: drupal_get_filename() should not use drupal_static() » drupal_get_filename() and drupal_load() should not use drupal_static()
Status: Needs work » Needs review
StatusFileSize
new drupal_static.patch1.13 KB
new drupal_static.patch1.13 KB

Makes sense, re-rolled with the comment change.

Also attached before/after webgrind screenshots which show drupal_load() and drupal_get_filename() in the top ten most expensive functions (on a vanilla install with no content).

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new patch.png531 KB
new head.png512.3 KB
pwolanin’s picture

pwolanin commented

Right - doesn't make sense for this to be a drupal_static and this might be one of the reasons why simpltest explodes if you do a call to drupal_static_reset() (i.e. reset everything) in setUp

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

HEAD broken.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 572452-8-replace-two-instances-of-drupal_static.patch1010 bytes
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Test bot is green, while I had a hand in this it's an obvious patch so marking RTBC.

pwolanin’s picture

pwolanin commented

Agree this is RTBC - simply a matter of logic.

casey’s picture

casey commented

Same for module_implements(); functions (hook implementations) don't disappear during a request.

casey’s picture

casey commented

Never mind, it's cache is reused in module_implements_write_cache().

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Performance

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Performance

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.