Use drupal_static() in dashboard_regions()
sun - October 31, 2009 - 22:25
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | dashboard.module |
| Category: | bug report |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | needs review |
Description
Dashboard is invoked on all pages.
I started to work on a patch, but...
...debug_backtracing through dashboard module I get the feeling that we need to retitle this issue "Region and block building + rendering eats our performance"...
#1
Contains my cufa() tweak from #615822: Figure out why D7 is so much slower than D6 to be able to see those function calls in the first place.
#2
#3
Yes, it IS the block building and rendering. This is just a start, but the whole thing needs to be revamped.
#4
Confirming it's slightly faster with the patch... but just slightly.
ab -c1 -n100 -C [session cookie] http://localhost/admin/dashboard
unpatched: 6.69 req/sec
patched: 6.76 req/sec
#5
Could you elaborate on the problem with block building and rendering? We don't usually have many blocks on a page so I would surprised if thats a significant contributor.
#6
I think when you look at a call graph, block rendering includes the rendering of the main content block, so that's probably what's showing up, but haven't looked into it much.
We are currently attempting to render the dashboard on every single page though, which is stupid.
@sun - can you re-roll without the call_user_func_array() changes?
#7
As far as I recall, we specifically make sure not to render it - see http://api.drupal.org/api/function/dashboard_block_info_alter/7
So I'm not sure I understand how this issue is about anything more than micro-optimization? (Not to say that it isn't worth doing, of course, but I don't understand how this would lead to major performance improvements.)
#8
Here's what shows up when viewing the front page.
#9
Without cufa() changes.
#10
Looks about right to me.
What is the rationale for using static rather than drupal_static() here?
#11
Much more accurate title and priority, IMO :)
#12
What's the rationale for using drupal_static?
#13
Committed to CVS HEAD. Thanks!
#14
Well, I thought the default policy was to use drupal_static() unless there is a really good reason not to... but the reality is that there is no policy :)
In any case, I think we do need it for dashboard_regions() since the results of that function come from module_invoke_all(), so it seems like they could change. Probably don't need it for dashboard_is_visible().
#15
I thought the policy is to use drupal_static only where you need to be able to reset the cache in special situations (those situations where a $reset parameter was formerly used). If you wouldn't have used a $reset parameter before, you wouldn't use a drupal_static now. So as sun noted, the question should always be: What's the rationale for using drupal_static? instead of the other way around.
About the module_invoke_all(). Can't this only change if a new module is activated? In that case it would be before dashboard_regions() is called for the first time, or not?
#16
Well, module_enable() can be called at any time in a page request or while running a script, and we can't predict when that will be in relation to anything else...
#17
1) A script doesn't count, because you won't see a dashboard without a UI.
2) If you expect that module_enable() will have any effect within the same request... then you have strange expectations IMHO. Drupal's overall caching doesn't allow for that. Arbitrary results is all what you may get, depending on when during the request you invoke it. Even if you convert to a drupal_static here, who will reset this static when a module is enabled?
#18
No longer a performance issue.