Performance: Static cache output of views_arg_load()

subscribe

subscribing.

Patch works for me and similarly reduces time spent in views_arg_load() and unpack_options(). Also applies to 6.x-2.x with minor fuzz.

I like this patch.

The method of generating the cache key looks like it could lead to false positives because value and index look like they could accidentally come together to form numbers.

I know it is *highly* unlikely to happen, but that's what edge cases are for.

I recommend:

$key = $name . ':' . $display_id . ':' . $value . ':' . $index;

Then use $key in all the places the calculation is used.

The only place that : is a valid value is $value so you shouldn't get any weird combinations that can masquerade as other combinations.

The $key change is included in this updated patch, plus a correction for the function comment block.

Definitely RTBC now. Sadly out of gas for committing for the day, will try to get this one in my next burst.

Committed to all branches.

I was profiling a site on my own and while going through different points of where a lot of time was spent on views_arg_load().

Let me explain first my scneario:

It's an ecommerce site with a somehow big menu structure populated by taxonomy using taxonomy_menu, and because each of those taxonomy/term/xxx that gets into the menu a views_arg_load is executed.

By applying this patch, the keys I get are as follows:

taxonomy_term:page:108:2
taxonomy_term:page:105:2
taxonomy_term:page:114:2
taxonomy_term:page:115:2
taxonomy_term:page:119:2
taxonomy_term:page:117:2
taxonomy_term:page:120:2
taxonomy_term:page:116:2
taxonomy_term:page:118:2
taxonomy_term_all:page:105:2

And a lot more of course. views_arg_load it's being executed 180 times collecting around 3.x seconds of execution time which is a lot.

This patch does nothing to me, because the key are too specific and for each of this menu the key is new so no real cache is being made. Also, although view::load() caches the views, views_get_view only clones an empty view, so on my case, each time the function is execution the same view and same display is initialized every time by:

    $view->set_display($display_id);
    $view->init_handlers();

which in my case it doesn't make much sense, because the display and the view is exactly the same, so why set the display and initialize it on each run?

I have changed the key to

$key = $name . ':' . $display_id;

And the performance boost is incredible, it was reduced to 88ms.

So, although I have worked a little bit through views entrails and also with some external modules, I am not sure of the implications of this, but for the logic that's around the loaded views on this specific function, I think a wider key makes a lot of sense.

Even the following key would do it:

$key = $name;

Because the set_display() will change it, but I think it's worth to cache each display on it's own.

EDIT: I even don't think this wider key would affect the cache in the way you were thinking it at the first point.

Thoughts?

I don't mind rolling a patch but it's really one line change, and while we are there, can we please commit this into 2.x as well, this is huge!

Thanks,
a.=

I guess I leave the discussion for 3.x first, but really, it's cheap to commit to 2.x at the same time.

Can you please provide a real patch?
This is not really usuable here.

Ok, sorry, being a one line I though it was easy to review. Attached is the patch for 3.x, applies the same to 2.x. I didn't realize the first patch was actually committed to the 2.x,

EDIT: Also bear with me if this is not an appropriate patch. Kind of new to git, only followed the instructions on the create patch page.

You are right, but then we are in the same scenario, I didn't realize that I was doing something wrong, I don't really want to cache the argument value, what I want to cache is the views already initialized and with the display set, actually you can cache both things really. Will submit and improved patch in a short while.

Ok, attached is the improved patch. both things are cached.

With this approach, I realize I had to remove $views->destroy() in order for the cached initialized view makes sense. I wonder if there are implications with that but I really think not.

Of course I lost a few milliseconds as the previous patch was super fast because it was wrong, but still in the order of the 150ms, which is still a lot of performance gain.

Probably an easier patch to review, just with git diff, I realized git format-patch includes every single commit as separate diffs, is that the proper way of submitting patches now?

I lost the changes I did on the first time and I left those there the second time by accident, of course, you are right.

I understand what you are saying, we could always make this optional by a configuration, but probably not worth adding complexity into it.

Not sure what to say, it definitely made an impact on my use case, if I were to chose between memory consumption and page load, I would go with the latter, although I know eventually one will hit the other in the but I still think this might be useful to have it.

I give this a thought while I was out, and if this becomes and issue or a stopper to maybe accepting this, there might be a way in hook_menu_alter() to decide whether to cache the views or not, depending on how many times an argument for a specific view/display will be loaded, I believe this might be known there, but not 100% sure.

+++ b/views.moduleundefined
@@ -340,18 +340,31 @@ function views_menu_alter(&$callbacks) {
+  $key_views = $name . ':' . $display_id;

Short question, why do we store the same view, but for different displays?

The rest looks fine from my perspective! Great patch

Powered by Dreditor.

dereine: I think becuase args vary based upon things like the path in the display, so each view + display creates a unique combination.

Oh yes i was wrong, but i'm still wondering whether it would be possible to store just a single view for multiple displays.

[slightly off track]
In an ideal world, we would not load the view, but just do what is minimally required to do the access check, or to load the argument.
This could either be via existing loaders like user_load() or taxonomy_term_load(), or new dedicated loaders like views_taxonomy_term_load(), OR
by loading just a small part of the view - not as much as would be necessary to actually render it.

This would be a lot more work, I'm afraid..

There might still be cases where it is required to load the complete view, if we want to show a "page not found" if the view is empty.
We can still load the view in views_access(), if it turns out to be necessary.
But otherwise, we could probably get away cheaper than that.
[/slightly off track]