The following patch makes it possible to reduce the number of calls to drupal_static(), by making it possible to reference a static variable to drupal_static. Pretty simple, but makes a huge difference.

I also included an altered version of module_implements, so it only calls drupal_static() once (until drupal_static_reset('module_implements') is called).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

casey’s picture

Status: Needs review » Active

Note that you cannot directly assign references to static variables.

So unfortunately this is not possible (declarations of static variables don't accept expressions).

  static $implementations = &drupal_static(__FUNCTION__, array());

An this neither (it'll lose the reference: http://php.net/manual/en/language.variables.scope.php#language.variables...):

  static $implementations;
  if (!isset($implementations)) {
   $implementations = &drupal_static(__FUNCTION__, array());
  }

But this is:

  static $cache = array();
  if (!isset($cache['implementations'])) {
   $cache['implementations'] = &drupal_static(__FUNCTION__, array());
  }
  $implementations = &$cache['implementations'];
casey’s picture

Status: Active » Needs review
Issue tags: -Perfomance +Performance
catch’s picture

effulgentsia’s picture

Status: Active » Needs review

I like this, and if there's too much resistance to globals for #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa to be community accepted, then I think this is a really clever way to handle it. Note, we'd need to handle the case of drupal_static_reset() being called with no parameter (drupal_static() would need to loop through $data and set each item to NULL).

casey’s picture

Another approach, I might even like better:

/**
 * Central static variable storage.
 *
 * @return
 *   Returns a variable by reference.
 */
function &_drupal_static() {
  static $data = array();
  return $data;
}

/**
 * Get a centrally stored static variable.
 *
 * @param $name
 *   Name of the static variable to return.
 * @param $default_value
 *   Optional default value.
 *
 * @return
 *   Returns a variable by reference.
 */
function &drupal_static($name, $default_value = NULL) {
  $data = &_drupal_static();

  if (!isset($data[$name])) {
    $data[$name] = $default_value;
  }

  return $data[$name];
}


/**
 * Assign a reference to a centrally stored static variable.
 *
 * @param $name
 *   Globally unique name for the variable. For a function with only one static,
 *   variable, the function name (e.g. via the PHP magic __FUNCTION__ constant)
 *   is recommended. For a function with multiple static variables add a
 *   distinguishing suffix to the function name for each one.
 * @param $reference
 *   Reference to store.
 */
function drupal_static_assign($name, &$reference) {
  $data = &_drupal_static();
  $data[$name] = &$reference;
}

/**
 * Reset one or all centrally stored static variable(s).
 *
 * @param $name
 *   Name of the static variable to reset. Omit to reset all variables.
 */
function drupal_static_reset($name = NULL) {
  $data = &_drupal_static();

  if (isset($name)) {
    $data[$name] = NULL; // Set data to NULL
    unset($data[$name]); // Remove reference
  }
  else {
    foreach (array_keys($data) as $name) {
      $data[$name] = NULL; // Set data to NULL
    }
    $data = array();
  }
}

Example usage:

function module_implements($hook) {
  static $implementations;
  if (!isset($implementations)) {
    $implementations = array();
    drupal_static_assign(__FUNCTION__, $implementations);
  }

}

function module_implements_write_cache() {
  $implementations = &drupal_static('module_implements');
  
}
casey’s picture

FileSize
3.81 KB

patch

effulgentsia’s picture

@casey: With more thinking about it, I like your solution more than the one in #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa, so I marked that one a duplicate. However, please see the conversation from http://drupal.org/node/620452#comment-2217666 on down. Would it be possible to revert to your original patch, except for adding the setting of each item to NULL when drupal_static(NULL, NULL, TRUE) is called?

casey’s picture

Sure

casey’s picture

Included a comment + some extra cached drupal_static(() calls in functions that get called multiple times.

Maybe somebody can benchmark this?

sun’s picture

yeah - before debating this any longer, we need numbers here.

catch’s picture

As I'd expect there's no measurable change with ab on node/1

NB: all benchmarks done with xdebug extension disabled, APC enabled

ab -c1 -n1000
HEAD:
15.45 [#/sec] (mean)
15.29 [#/sec] (mean)

Patch:
15.18 [#/sec] (mean)
15.52 [#/sec]

However that's a very simple page, we might see something measurable on a page which calls more links or has more potential path aliases to look up.

In micro-benchmarks of url() - which can be called several hundred times on some pages and calls drupal_lookup_path() , there's a decent improvement though:

1 million calls to url():

Head:
31.5417618752

Patch:
25.4791371822 seconds

I also microbenchmarked the equivalent of what the patch does (using attached script):

1 million requests:
drupal_static(): 2.7214422226 seconds
pattern used by this patch: 1.52654600143 seconds
Just a static: 0.876840829849

That means each 1,000 calls to drupal_static() cost about 2.7 milliseconds (given ab runs on HEAD give times in the region of 70-110ms that's not insignificant), this patch almost halves that time despite my test functions doing a couple of other things as well - assuming my test script isn't stupid, still a bit early in the morning here.

To get an idea how much we currently call drupal_static() in HEAD on more complex pages, I profiled node/1 with the default of 50 comments displayed (but with #607244: Decrease performance impact of contextual links applied since that completely skews all results otherwise) - that generated 2300 calls to drupal_static().

I then profiled the same page, but with 300 comments displayed - same as d.o issue queue. This gives us c. 10,900 calls to drupal_static() - which takes up about 1% of the page execution time according to webgrind, despite also executing 460 database queries on that page, a bunch of rendering etc.

So:
# The patch gives us about a 50% speedup in cases where drupal_static() is called in the same spot many, many times during a request.

# It lets us use drupal_static() in situations such as #619566: Don't invoke hook_url_outbound_alter() if there's nothing to invoke - where a straight static is risky, and drupal_static() is still some overhead compared to zero if we hadn't committed hook_url_outbound_alter() at all.
# However, we need to be very careful to document when and where this pattern should be used. There are only 11 potential candidates in the 300 comments example, and that's one of the most extreme example pages in core.

But, if my numbers are right (would be lovely if someone verifies this either way) and 10,000 calls to drupal_static() ~=27ms, then dropping this to 17ms, with no impact on testability is nice to have.

catch’s picture

With attachments...

catch’s picture

Title: Reduce calls to drupal_static() » Introduce new pattern for drupal_static() in performance-critical functions

Re-titling.

chx’s picture

See #620688: drupal_static_reset is broken . I posted a description of what's going on here sorry for not crosslinking earlier. In short: yes to the pattern of calling drupal_static as little as possible but the fixes in drupal_static are fixing a symptom and not the cause and are hackish.

effulgentsia’s picture

Confirming catch's numbers. Here's a patch that does it in more functions. The way I chose which functions to put it into was profiling a node with 50 comments served to an anonymous user with "access comments" permission. For every function that uses drupal_static() and is called more than 100 times for that page, I replaced it with the more optimized version this patch allows. This resulted in a savings of 1550 calls to drupal_static(). Apache benchmarks attached (a difference of 3.1ms, or 1.6%, or 2us per saved invocation).

As I mention in http://drupal.org/node/591794#comment-2228344, if that issue's patch gets committed, then committing this one as well (and applying this patch's optimization in drupal_alter()) will result in a signficant reduction to that function's overhead. And reducing the overhead of a key function like that has enormous impact to module developers.

I appreciate that replacing 1 line of ugly code with 5 lines of ugly code has its drawbacks, but I think we can apply the following rule to maintain sanity:

1. If a function doesn't need a resettable static, just use the static keyword.
2. If a function needs a resettable static and will likely be called less than ~100 times per page request (the cutoff number can be up for discussion), use the one line drupal_static() pattern.
3. If a function needs a resettable static and will likely be called more than ~100 times per page request on at least some performance-critical pages, then use this nasty 5 line pattern. We're talking about what, maybe a dozen functions or so, so a total of ~60 lines of ugliness among how many total lines of code that make up drupal!

effulgentsia’s picture

If we're gonna make this a pattern, we don't need a comment explaining it each time it's used. Also, how's this for a little syntax polish? The ugliness is now 3 lines, but the first and last are at least somewhat understandable. The middle line will make eyes bleed, but it can be copied from place to place without any changes, and so we can just train ourselves to look past it.

effulgentsia’s picture

Nope, #16 wasn't as fast as it could be. The pattern is complex enough without adding default support. Besides, most of the time, defaulting to NULL is just fine, and where it isn't, code can be written to add in a default after the pattern as shown here in drupal_lookup_path(). This approach makes the first two lines identical everywhere used, keeps the 2nd line shorter, and is fast.

moshe weitzman’s picture

Nice explanation. I reviewed the code, and it looks good to me. From my perspective, is RTBC. Lets let others chime in.

catch’s picture

The changes to drupal_static_reset() itself are probably better handled in #620688: drupal_static_reset is broken (although that's not working yet), but otherwise, despite being a bit of an eyesore, the last patch looks good to me.

While this is tweaky, it only affects a handful of highly critical functions, and if you need it, you can happily copy and paste it without knowing what's going on. And it finally lets us have our cake and eat it between performance and testability.

effulgentsia’s picture

W00t! drupal_alter() static caching from #591794: Allow themes to alter forms and page landed. So, this patch adds it to the list of performance critical functions. Also, I updated the comments in drupal_static().

I can't promise whether it will have community buy-in, but after this issue lands, I'll open an issue to remove the static variable hack in url() that tries to circumvent drupal_alter(). If we get drupal_alter() to be nearly free, IMO, that particular optimization is not worth the ugliness that it introduces, but we'll see what others think of that. But, before that's even up for discussion, this needs to land.

catch’s picture

OK I can't get #602668 to work at all - the problem is that the default for module_hook_info() can be set to NULL, before it gets set to an empty array during testing. Since the change to drupal_static() here works, and if anything is more readable, I think we should proceed here.

I'd also support a rollback of the url() static once this is in.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

already got +1 from catch and myself

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Patch now fails due to #620688: drupal_static_reset is broken. Would be awesome if someone can fix this today. Otherwise, I will tomorrow.

catch’s picture

Status: Needs work » Needs review
FileSize
6.17 KB

Re-rolled, only change is that we no longer need a change to drupal_static()

chx’s picture

Status: Needs review » Reviewed & tested by the community

Moshe, catch and I like it, the bot likes it... what's not to like :)

catch’s picture

Just mentioned this one to webchick in irc, and she said it almost certainly needs sign-off from Dries before commit.

On the off-chance that this gets rejected, I have also opened #629452: Roll back drupal_static() - we could also just use static $var in these functions and see if anything breaks in testing.

effulgentsia’s picture

Well, let's minimize the chances of him rejecting it. This patch includes PHPDoc for the drupal_static() function. It also changes the arg() function to not use a resettable static at all, because I was looking for an example to use for the PHPDoc for where that's ok, and there are very few places where it's so clearly ok.

effulgentsia’s picture

And just to prove my point that it's hard to come up with an obvious example for where a static doesn't need to be resettable, I realized that arg() doesn't fit the bill. So, this patch reverts it to what it was in #26 (and adds a comment explaining why resettability is needed there), and uses a different example for drupal_static()'s PHPDoc.

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ includes/bootstrap.inc	11 Nov 2009 19:27:42 -0000
@@ -2080,6 +2080,77 @@ function registry_rebuild() {
+ * function user_role_permissions($roles = array()) {
+ *   $cache = &drupal_static(__FUNCTION__, array());
+ *   $role_permissions = array();
+ *   // Determine the permissions allowed for the roles. This requires
+ *   // a database query. However, if user_role_permissions() is called
+ *   // multiple times during the same page request for the same role, we
+ *   // don't want to keep re-querying the database, so store the results in
+ *   // the $cache variable, and on subsequent calls, return them quickly.
+ *   ...
+ *   return $role_permissions;
+ * }

As this will be the guideline for implementors, we want to provide a standard and proper examples to follow.

1) Rename $cache to $role_permissions.

2) Do not define a default of array() for the static.

3) Wrap the inner contents with if (!isset($role_permissions)) {

4) Perhaps additionally make clear that one should only set a default for a static if there is a real default (and thereby also use empty() instead of !isset()). It always depends on the cached data that is built, whether there is a possibility of an empty return value. Only the !isset() prevents the builder from running multiple times when there is no data. Hence, the rule of thumb in most cases is: Either there is statically cached data, or there is not.

+++ includes/bootstrap.inc	11 Nov 2009 19:27:42 -0000
@@ -2080,6 +2080,77 @@ function registry_rebuild() {
+ * function user_access($string, $account = NULL) {
+ *   // This function is called so often, that an extra call to drupal_static()
+ *   // each time would have a noticeable impact on overall performance.
+ *   // Therefore, use the following 3-line pattern to combine the benefits
+ *   // of the drupal_static() function and the "static" keyword.
+ *   static $drupal_static = array();
+ *   isset($drupal_static[__FUNCTION__]) || ($drupal_static[__FUNCTION__] = &drupal_static(__FUNCTION__));
+ *   $perm = &$drupal_static[__FUNCTION__];

We should move the large explanation in the inline comment above the example snippet, and, insert a common standard inline comment for these code snippets. Suggestion:

// Called very often; use the advanced pattern.

And, that inline comment we want to prepend to all instances of core in this patch.

I'm on crack. Are you, too?

effulgentsia’s picture

With sun's feedback, except instead of changing how user_role_permissions() is implemented, I picked an example that's more inline with the syntax we want to showcase.

sun’s picture

Thanks!

+++ includes/common.inc	11 Nov 2009 22:06:24 -0000
@@ -2275,7 +2275,12 @@ function format_interval($timestamp, $gr
+  // This function needs its static variable to be resettable, but it is
+  // called very often, so use the advanced drupal_static() pattern.

We can shorten this to:

  // This function needs a resettable static cache, but is called very often, so use the advanced drupal_static() pattern.

Ideally though, we'd find a explanatory statement that fits on one line. Perhaps:

  // Use the advanced drupal_static() pattern, since this is called very often.
                                                                                | (80 chars)
effulgentsia’s picture

Love the 1 line explanation! Also added a link to this issue in the PHPDoc.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

I like the new phpdoc a lot, encapsulates much of the discussion on this thread and other related ones, and better than littering core with inline comments all over the place.

webchick’s picture

Wow. Damn. Those docs are really nice. With those, all of my concerns with this approach and its total lack of code legibility are now satisfied. We document the WTF with a basic "yes, we know" comment, and the drupal_static() docs have been wildly expanded to include instructions for people on how to properly utilize it.

I'd still like to give Dries a crack at this patch if he wants it, so I'll leave this in the RTBC queue another 48 hours, and then commit it if no objections.

catch’s picture

Great that this will get in. Just for reference, I profiled the field API conversion patch with 50 comments, and got the following results - that's 6% of page execution time for just under 4,000 calls to it - so this really isn't micro-optimization given the vast majority of those are from < 10-20 functions calling it dozens or hundreds of times each during the request.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

catch reminded me of this patch again tonight.

The syntax of this still gives me the heebie jeebies, and makes me wonder again whether or not drupal_static() was a good idea to begin with if we have to resort to these kinds of back-flips to make it perform. :\ But the patch here does make the best out of the situation, and the docs are a much-needed improvement. So as promised, I've committed this to HEAD.

I think this also marks the last of the "stupid functions called bazillions of times that gum up code profilers" patches, so hopefully this will help the performance efforts along as well.

Let's get a mention of this new pattern under UNSTABLE-11 on the 6.x => 7.x upgrade guide: http://drupal.org/update/modules/6/7

catch’s picture

FileSize
819 bytes

Hmm, the actual example is broken in user_access() - drupal_static() was still getting called every time, calling drupal_static(__FUNCTION__, array()) instead of with no default value fixes this. Leaving this at needs work since this pattern ought to work with no default.

chx’s picture

Although array_key_exists is slower than isset, let's use that. It's ... not significantly slower -- as a function call yeah it's slower but it's only measurable for real lots of calls. Which is, accidentally, what we do here :/ Alternatively make sure to not use NULL as a default and doc it. However, d_s is only called as long it's NULL right? That's not a lot of calls...?

catch’s picture

Without this patch, 84 calls to user_access() = 84 calls to drupal_static() - and drupal_static() still takes 2.2% of the overall request (which puts it in the top ten expensive functions for self time).

With the patch, that drops to 1.83%. OK that's only 0.4% of the total request, but we want to make sure that doesn't accumulate over 10 functions and turn into 4% just due to NULL defaults not working :/

I think we probably want to not use NULL as a default when using the 'advanced pattern' - however note that this was changed in #35 due to sun's review (which is probably what broke it, since previous profiling it was working fine). So I'd like to see him chime in here before rolling the patch to change that back to how it was.

sun’s picture

+++ modules/user/user.module	20 Nov 2009 06:56:39 -0000
@@ -681,7 +681,7 @@ function user_access($string, $account =
   static $drupal_static = array();
-  isset($drupal_static[__FUNCTION__]) || ($drupal_static[__FUNCTION__] = &drupal_static(__FUNCTION__));
+  isset($drupal_static[__FUNCTION__]) || ($drupal_static[__FUNCTION__] = &drupal_static(__FUNCTION__, array()));
   $perm = &$drupal_static[__FUNCTION__];

To start with, can someone explain once again why $drupal_static variable is an array and why we have all those __FUNCTION__ keys in there?

array() can certainly only be used as default, when we are 1000% sure that it will be !empty() after initial population. Of course, the builder condition elsewhere in the function needs to be updated accordingly to check for empty() instead of !isset().

An alternative would indeed be to replace isset() with array_key_exists() in all advanced patterns, like chx proposed.

This review is powered by Dreditor.

casey’s picture

Because this is not possible (static keyword works like a reference; see #1)

static $perm = array();
if (!isset($perm)) {
   $perm = &drupal_static(__FUNCTION__, array());
}

So $drupal_static already is a reference, but we can create references inside an array; that's what this does.

__FUNCTION__ can be replaced by something else

  static $drupal_static = array();
  isset($drupal_static[0]) || ($drupal_static[0] = &drupal_static(__FUNCTION__, array()));
  $perm = &$drupal_static[0];

I agree it's ugly. Honestly I would prefer drupal_static() to be never existed.

casey’s picture

Thats why I suggested #5; always use static keyword, and use drupal_static_assign() when you want it to be accessible/resetable centrally.

I think that approach is at least less ugly.

effulgentsia’s picture

I think we DO want to use NULL as the default in the advanced pattern always. Because that lets us keep the complicated line identical everywhere it's used, so it can be copy/pasted without being understood. There's no other default value that we can standardize on that would make sense generically. In every other place we're using the advanced pattern, having NULL as the default isn't a problem, because it's always followed by some sort of initialization to something. Even in user_access(), the only way that $perm doesn't get initialized away from NULL is when user_access() is called for uid 1. As soon as it's called for any other uid, it gets initialized. Is this something that needs optimization? Are extra calls to drupal_static() only when the root user is logged in that much of a problem?

We can't use array_key_exists() here, because in this particular place, it's specifically the place where extra function calls need to be avoided, and because it will always be true, even the first time after a reset.

webchick’s picture

Hm. So should I roll this back, or..?

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
761 bytes

re: #47, No. What catch brings up is a localized issue to user_access() called for the root user, and is no worse, performance-wise, than before the advanced pattern landed. The other places are resulting in noticeable performance benefit. Sun and casey bring up good points that the pattern can be made a little bit shorter by replacing some of the uses of __FUNCTION__ with 0. But let's deal with that after we resolve the user_access() issue.

Here's a patch to resolve the user_access() for root user issue. I don't know if a performance benefit for the root user is worth it. The one nice thing it does is allows profiling to be done as the root user and have less ways in which that gives different results vs. a different user.

effulgentsia’s picture

This one's a better way to solve the user_access() problem, and is a no-brainer as far as being commit-worthy. Once it lands, this issue can be used to discuss any remaining improvements people want to float (like the drupal_static_assign() concept).

@catch: Since you first noticed the problem in #40, can you verify that this solves it, and if so, set it to RTBC? Thanks.

casey’s picture

It's getting more nerdy, but this would also work...

  static $drupal_static = array();
  isset($drupal_static[0]) || $drupal_static[0][1] = &drupal_static(__FUNCTION__);
  $implementations = &$drupal_static[0][1];
effulgentsia’s picture

@casey: Dude! Nice! I'm impressed yet again by your cleverness. However, let's keep this issue focused for now on #49 until it lands, which solves the specific problem of #40 in a non-controversial way. But once it lands, I would definitely like us exploring both the drupal_static_assign() idea and the #50 idea, either or both of which could make for improved syntax and edge case robustness.

catch’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
91.35 KB

#49 is fixed, apologies for not noticing this one is restricted to user/1 - that change simply moves the code block below the user/1 check which is completely sane though.

Dries’s picture

To me, that looks like a pretty ugly pattern. Personally, I don't think a 0.4% performance gain is worth that code.

catch’s picture

The 0.4% gain is only from moving the identical ugly code hunk from one spot to another, I don't see that it's any more or less ugly before or after the $user->uid ==1 check.

However the original patch is up to 6% improvement on pages where drupal_static() gets called a lot (up to 3,000 calls or more on some pages), and a good 1-2% on most pages.

Personally I'm thinking we should have made the test framework jump through hoops rather than all of core's performance-critical functions in regards to static caching, but I don't see us rolling back drupal_static() to straight static $var this release cycle, so this pattern at least curbs the worst excesses.

webchick’s picture

Status: Reviewed & tested by the community » Active

Ok. I committed #49 to HEAD, since that is basically clean-up from the last patch that was already committed. Marking back to 'active' now for further discussion.

I share Dries's concern about the total illegibility of this pattern, and have never liked this drupal_static() stuff to begin with: it's a Drupalism, and a particularly ugly one at that, plus it requires silly belly-dancing like this to perform properly. However, since #601584: setUp() function for unit and web test cases should reset all resettable statics was just committed today, we do (finally) have a use case for this in core.

However (again), I've heard Jimmy say that this won't be required in version 7.x-2.x of SimpleTest, which he's currently working on at http://drupal.org/project/simpletest (I have no idea what this means or how extensive changes would be required, etc.). Dries also itemized a lot of really sound reasons why $reset parameters are better over at http://drupal.org/node/422370#comment-1438846 and I've noticed that most of our key load, etc. functions still retain $reset.

So it begs the question, do we even still need/want this pattern in core? Everyone was ready to start a lynch mob back in #254491: Standardize static caching, but what have we really gained?

moshe weitzman’s picture

Personally I'm thinking we should have made the test framework jump
through hoops rather than all of core's performance-critical functions
in regards to static caching, but I don't see us rolling back
drupal_static() to straight static $var this release cycle, so this
pattern at least curbs the worst excesses.

Agreed.

catch’s picture

Well the issue in #254491: Standardize static caching was less that we all loved drupal_static() and more that:

1. We needed the test suite to be able to run in one go without phantom failures.
2. We didn't want to abuse cache_set() / cache_get() or have that issue turn into something it wasn't.
3. We also didn't want a verbose pattern like variable_set_temporary() / variable_get_temporary() which couldn't be easily interchanged with static $var. &drupal_static() at least gives us a single line replacement, and even the 'advanced' pattern lets the rest of the function continue as before. Note that also makes it relatively easy to roll back ;)
4. We thought we could kill $reset parameters throughout core.

If #1 can be dealt with another way, then #2 and #3 don't even come into it, and #4 we haven't done and doesn't seem to be anyone's itch any more either. I don't think anyone really, really wants to keep it in at this point.

effulgentsia’s picture

Status: Active » Needs review
FileSize
7.81 KB

There are some very good arguments for removing the use of drupal_static() entirely, because it does introduce all sorts of ugliness. I'm not convinced that a reset parameter in every function that wants a static variable is any less problematic. But, clearly we do not yet have community consensus on how to solve the problem of how to have resettable statics and how to reset them. Our current answer is a combination of drupal_static()/drupal_static_reset(), functions with reset parameters, and *_reset() functions. Let's continue the conversation about how to improve this situation in #629452: Roll back drupal_static() and #581626: Use a consistent/clean pattern for using $reset or drupal_static().

If discussion on those issues leads to a community-accepted way of removing drupal_static() entirely, or at least from performance-critical functions, then great! However, as long as we continue to use drupal_static() in performance-critical functions, then we need to have a way to do it in an optimized way. That's what this issue is about.

When this issue started, drupal_static() was written in such a way that drupal_static_reset() disassociated the reference, and therefore drupal_static() had to be called again after a reset, which it could only detect by an isset(), which leads to the theoretical problem that if you use this advanced pattern, but then don't initialize the static variable to something other than NULL after retrieving it, then in every invocation of the function, you keep calling drupal_static(), which while not causing a functional bug, it does return you to the performance problem this issue was meant to solve. This situation does not occur in actuality in any place currently using the advanced pattern (it did in user_access() for the root user, and #49 fixed that), but it does remain a theoretical problem.

Fortunately, #620688: drupal_static_reset is broken fixed the problem of reference disassociation, and therefore, this theoretical problem can be fixed along the lines of casey's brilliant approach in #50, but with an even further simplification:

  static $drupal_static;
  isset($drupal_static) || ($drupal_static[0] = &drupal_static(__FUNCTION__));
  $implementations = &$drupal_static[0];

This patch makes this change to all places using the advanced pattern. This makes the optimization effective even in situations where the static variable remains NULL. It's also a little bit shorter than the code in HEAD (for whatever that's worth).

jrchamp’s picture

Reposting webchick's reminder for Documentation: "Let's get a mention of this new pattern under UNSTABLE-11 on the 6.x => 7.x upgrade guide: http://drupal.org/update/modules/6/7"

effulgentsia’s picture

So what do folks think of #58? Should we decide on whether we want to leave HEAD as it is for now or have #58 (or some refinement of it) land before we document the new pattern on the module upgrade guide?

casey’s picture

+1 As it seems we're not rolling back drupal_static() on the short term, I think #58 is a good thing as it clarifies/optimizes the pattern.

catch’s picture

#58 looks good to me too.

For now I've changed the upgrade documentation to point to api.drupal.org for drupal_static(), we have an outstanding documentation/code style issue which is likely to require changes, and the phpdoc is already very good and more likely to be updated with each patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs review

Benchmarks?

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

#58 is only a tiny optimization. Its 2 main benefits are:

1. Shorter and slightly more understandable syntax (less uses of __FUNCTION__).
2. It creates more robustness for edge cases that don't exist in HEAD yet (see #58 paragraph 3). The one edge case that was found in #40 was removed in #49.

However, it does create a tiny optimization as well. Calling drupal_alter() 1,000,000 times for an alter hook not implemented by anything:

HEAD: 2.94s
#58: 2.80s

Code:

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

$data = array();
$t0 = microtime(TRUE);
for ($i=0; $i<1000000; $i++) {
  drupal_alter('xxxx', $data);
}
$t1 = microtime(TRUE);
print ($t1 - $t0);
effulgentsia’s picture

Same code as #58, but added more PHPDoc to drupal_static() explaining the advanced pattern.

jrchamp’s picture

It definitely feels a lot better and the small performance benefit is nice too.

casey’s picture

#66 is just an improvement of previous patch so lets get this thing fixed.

Status: Reviewed & tested by the community » Needs review

Re-test of drupal_static_advanced_cleanup-619666-66.patch from comment #66 was requested by webchick.

casey’s picture

Status: Needs review » Reviewed & tested by the community

per #65

effulgentsia’s picture

Just wanting to point out that there is now an RTBC issue for adding another use of the advanced pattern: #672480: field_multilingual_available_languages() does not use advanced drupal_static() pattern. And on that issue, it would be convenient to pass the $default_value parameter when calling drupal_static(). I commented on that issue for why I think having #66 land first would be preferable.

effulgentsia’s picture

Title: Introduce new pattern for drupal_static() in performance-critical functions » Make performance-critical usage of drupal_static() grokkable
Status: Reviewed & tested by the community » Needs review
FileSize
10.02 KB

Apologies for messing with a patch that's been RTBC for a while, but in light of looking at #672480: field_multilingual_available_languages() does not use advanced drupal_static() pattern, I think this one's better.

It has equivalent performance as HEAD (using the test from #58, calling drupal_alter() 1,000,000 times clocks in at 2.92s for HEAD and this patch), so it's not quite as fast as #66, but we're really talking about tiny amounts here (#66's appeal was less about shaving 0.1 microseconds per invocation than about making the code more readable). For comparison, this issue gave us a 1-2 microsecond improvement compared to not having the advanced pattern at all, so an additional 0.1 microseconds, while a nice side benefit if we could get it without sacrificing readability, isn't worth a decrease in readability.

However, I think this one is an improvement in terms of readability, and, IMO, has the following benefits:

  • No tricky use of short-circuited OR statement, so more approachable to non-geeks.
  • Calling the variable $drupal_static_fast removes the need for the comment above each usage of this pattern. It's self documenting, and we can reasonably assume that someone wanting to know more will look at the documentation for drupal_static().
  • Using the name of the variable (instead of 0) as the key for $drupal_static_fast makes it more clear what one would need to do if they want a second drupal_static() variable in there.
  • With this syntax, if in some places we want to pass a $default_value to drupal_static(), as we want to in #672480: field_multilingual_available_languages() does not use advanced drupal_static() pattern, we can, and the code remains understandable.

As a reminder, the reason this issue didn't start off with this more understandable way of writing the code, was that drupal_static_reset() was broken, so there was more trickiness needed to make this work. But because drupal_static_reset() has been fixed, there's much less trickiness needed, and that's reflected in this patch.

catch’s picture

Status: Needs review » Reviewed & tested by the community

This is 100x more readable, RTBC. I've marked the field languages issue postponed since that's a lot easier to re-roll, and can go in whenever, whereas it'd be nice to nail this down finally soon.

sun’s picture

+++ includes/path.inc	5 Jan 2010 20:55:56 -0000
@@ -327,10 +329,11 @@ function drupal_set_title($title = NULL,
-  // Use the advanced drupal_static() pattern, since this is called very often.

We should keep the standard comments though.

Powered by Dreditor.

effulgentsia’s picture

We should keep the standard comments though.

I don't mind, but why? Does the variable name $drupal_static_fast not make it clear? If you look at the comment trail on #672480: field_multilingual_available_languages() does not use advanced drupal_static() pattern, it seems like catch and yched (two Drupal experts) weren't sure about whether or not to include a comment vs. whether we should assume someone would know to look at the drupal_static() documentation. So what are we trying to achieve with the 1-line comment? And should it be changed to say @see drupal_static() as part of it?

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Ah-ha! now this is awesome. :)

I do agree with sun though that I'd like the comments added back. The code is relatively self-explanatory, I agree, but the name of the variable is going to make people think they want to use this all the time (I mean, when wouldn't you want a fast drupal_static?!) when it's really only for special cases.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.66 KB

OK

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal_static_advanced_cleanup-619666-77.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.66 KB

Can't replicate failures locally. Re-uploading #77.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Great. Committed to HEAD.

Marking back to "needs work" for documentation.

effulgentsia’s picture

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

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