When clearing the cache on a high traffic site, multiple requests will attempt to rebuild the default views cache at the same time. This happens in _views_discover_default_views(). Ordinarily, this stampede is not critical except for performance, but when combined with the extra views cache in views_attach.module, or various other caches, it becomes a problem.

We've seen the following behaviour:

Request A & B are both executing _views_discover_default_views(), line 99 of cache.inc.

- A is currently building the cache; calling views_cache_set('views_default:' . $name, $view, TRUE); on line 148, for each view.

- B checks to see if it also needs to rebuild the cache, by checking if the 'views_default_views_index' cache item is set (line 106), which it is, because A has already set it right before A began setting each individual view's cache item. So B proceeds as if the cache is ready, even though A hasn't finished setting each view cache item yet.

- B collects each view, by calling views_cache_get('views_default:' . $view_name, TRUE), and some of these calls fail, as they aren't set yet by A.

B's page request will probably fail if it is trying to use the views for anything. In fact, any page requests that happen in the time between A setting the index cache item and A setting each view cache item, may fail.

As long as A completes it's build, subsequent requests will succeed as A will have finished building the cache properly. So some users may notice some strange behaviour, but nothing serious.

The problem occurs when the result of the failed request B is cached somewhere else. When combined with views_attach this happens on line 206 - the broken result is cached, meaning that any attached views will be broken until the cache is explicitly cleared again.

Proposed solution

By setting the index cache item after the individual view items, we ensure that subsequent requests only attempt to use the cache when it's completely built.

CommentFileSizeAuthor
#129 853864-memory.patch4.59 KBcatch
#127 views-853864-127-fix-race-condition-6x-2x-do-not-test.patch3.46 KBmikeytown2
#127 views-853864-127-fix-race-condition-6x-3x.patch5.05 KBmikeytown2
#126 views-853864-126-fix-race-condition.patch3.5 KBmikeytown2
#113 interdiff.txt1.77 KBdstol
#111 views_race_condition_853864.patch15 KBcatch
#110 views_race_condition_853864.patch14.92 KBcatch
#106 views-default-853864-106.patch14.96 KBcatch
#99 views_default_853864_6x2x_99.patch15.13 KBdstol
#98 views-default-853864-98.patch14.95 KBJoeMcGuire
#97 views-default-853864-97.patch14.94 KBJoeMcGuire
#96 views_default_853864-6x3x.patch14.41 KBcatch
#91 default_views_minimum_version-853864-91.patch14.29 KBwebflo
#91 default_views_minimum_version-inner_diff-853864-91.patch2.28 KBwebflo
#87 default_views.patch14.24 KBcatch
#84 views_default_853864-84-6x3x.patch17.01 KBcatch
#83 views_default_853864-83-6x3x.patch17.01 KBcatch
#80 views_default_853864-80-6x3x.patch16.96 KBcatch
#79 views_default_853864-79-6x3x.patch16.63 KBcatch
#74 default_views63.patch16.61 KBcatch
#74 default_views62.patch16.63 KBcatch
#69 default_views63.patch16.6 KBcatch
#69 default_views62.patch16.62 KBcatch
#68 default_views62.patch16.62 KBcatch
#68 default_views63.patch4.59 KBcatch
#66 default_views.patch16.27 KBcatch
#66 deafult_views63.patch16.27 KBcatch
#64 default_views.patch16.27 KBcatch
#63 default_views.patch0 bytescatch
#63 deafult_views63.patch16.27 KBcatch
#60 views_default.patch15.46 KBcatch
#59 views_default.patch16.55 KBcatch
#54 default_views_63.patch9.69 KBcatch
#54 default_views_62.patch9.67 KBcatch
#53 default_views.patch16.53 KBcatch
#51 default_views.patch14.56 KBcatch
#50 default_views.patch3.1 KBcatch
#49 default_views.patch13.82 KBcatch
#40 views-853864-6x-3x.patch5.05 KBcatch
#34 views-853864.patch3.65 KBcatch
#31 views-853864.patch3.1 KBmikeytown2
#30 views-853864.patch3.09 KBmikeytown2
#29 views-853864.patch3.29 KBmikeytown2
#27 views-1003716-27.patch3.27 KBmikeytown2
#26 views-1003716.patch2.36 KBmikeytown2
#22 views_race_condition.patch2.98 KBcatch
#21 views-1003716.patch2.19 KBmikeytown2
#20 views-1003716.patch2.12 KBmikeytown2
#12 Desk 1_022.png228.05 KBcatch
#12 Desk 1_023.png225.94 KBcatch
#11 views_race_condition_853864_0-3.x.patch5.33 KBdawehner
#10 views_race_condition_853864.patch5.6 KBcatch
#9 views_race_condition_853864.patch3.38 KBcatch
#8 default_views_race_condition_853864.patch1.24 KBcatch
#7 views-prevent_race_condition-853864.patch3.1 KBktha
#2 views-cache_stampede-853864-1.patch766 bytesMark Theunissen
#1 views-cache_stampede-853864-0.patch721 bytesMark Theunissen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mark Theunissen’s picture

Mark Theunissen’s picture

Forgot to use the -p switch.

Mark Theunissen’s picture

Now that the locking framework is in Drupal 6 core, has anyone talked about adding it to the views cache rebuild?

moshe weitzman’s picture

I agree that a quick lock.inc implementation is in order. We have been adding it to D7 in various places so its a good path going forward. For an example, see #802446: Cache stampede protection: drupal_render() and page cache

Mark Theunissen’s picture

Status: Needs review » Needs work

I noticed another issue that could arise when using Views in conjunction with a memcache cluster. If the Views bin is distributed across multiple memcache instances, and one of them goes down (one that doesn't contain the index cache item), the function _views_discover_default_views() will not fallback to rebuilding the cache.

Ideally, it should assume a missing view (despite the presence of the index) means a problem, and that the cache needs regenerating.

I want to work on this and the locking but am on vacation for 2 weeks from tomorrow, so will have to be after that.

Mark Theunissen’s picture

Title: Cache 'views_default_views_index' after caching each individual 'views_default:$name' in _views_discover_default_views() » Overhaul views caching logic to rebuild item on cache miss.
ktha’s picture

Status: Needs work » Needs review
FileSize
3.1 KB

In my opinion, there are multiple race conditions that could occur when splitting the list of views items among multiple entries (an index and individual elements).
I've attached a patch that will make sure that _views_discover_default_views() will cache the entire list of views as one element. The individual caching of the views entries has also been removed since "per-view-name" caching is triggered by views_get_default_view().

catch’s picture

Title: Overhaul views caching logic to rebuild item on cache miss. » _views_discover_default_views() is prone to race conditions
Priority: Normal » Major
FileSize
1.24 KB

I just spent most of today debugging missing content (from views that had returned false in views_get_view() while the cache was being rebuilt) which eventually led me to this bug. Came here to post the same patch as Mark Theunissen, did a search for the function name and arrived at this issue.

While #7 looks sane in principle, it has a bug where each module overwrites the cache entry with a partial list as far as I can see. I'm also concerned that caching such a large array may hit mysql_allowed_packet limits on some configurations - that's a lot of data for a single cache entry.

This is the patch I arrived at separately, it's exactly the same as Mark's plus some additional comments and a @todo. Fully agreed that there should be some logic if an individual view is a cache miss - memcache server going down, eviction - there are valid reasons for that to happen. Also that this whole process could do with a lock around it.

Will be testing this patch on a live site over the next few hours. May well work on a lock version later in the week. I'm also bumping this to major.

catch’s picture

Had the same symptoms with this patch applied, which I believe is due to a different race condition again.

Here's how I think that race could happen, this isn't 100% confirmed since I had trouble forcing it to be reproduced:

Process 1 gets a cache hit for the list of views.
Process 2 clears the views cache
Process 1 Then gets a cache miss for each view, until they are cached again by another process.

This is an even smaller window than the original race condition here, but comes down to the same bug of assuming that if one cache entry exists, all the others do.

To account for this, the original race, and the potential for cache items to go missing individually (in memcache and etc.), I'm uploading ktha's patch which builds and caches all the default views in one entry.

Going to do some testing with this now.

catch’s picture

There's a fatal flaw with the #7/#10 approach, and this also highlighted another issue with the original code too.

In views_get_default_view(), it does the following:

1. Try to get the cache entry for an individual default view, return it if available.
2. Load up all default views, return if available.
3. If not return null.

views_get_default_view() also doesn't have any code for when no default view is found, because that's a valid condition if the view lives only in the database.

This means that with #7/#10 applied, views_get_default_view() would always get a cache miss, so always cause the cache entry of all default views to be loaded into memory, when profiling this was taking up up to 20mb for a single request.

As much of a concern, with the current code in 6.x-2.x, if a view lives only in the database, every single time it is loaded, views will load every default view into memory, just to find out that there isn't one for this view - on a site where custom views aren't exported to code, but which has a lot of modules installed that provide views like nodequeue, this could be a very large memory hit.

With the various issues here - the possibility for a view to have a default or not, the two potential race conditions, I can't see many options apart from maintaining two caches, attached patch does this. The same logic in #7/10 is retained, with a single large cache entry in _views_discover_default_views() - when this function is called, all default views are loaded into memory anyway, and that still remains the only way to fix the various possible race conditions.

However it also adds an internal cache to views_get_default_view() - this requests from cache, gets the output of views_get_default_views() if there's a miss, writes an individual cache entry for the view if one is found. If there's no default view at all, then it writes a cache entry with NULL instead - so that on the next request, we won't load every default view again just to check.

This could present an issue with the following, however thinking about it I don't think there's any regression:

1. View is in database only.
2. View is exported to code.
3. We now have a cache entry that says there's no default view, even though there is one.

However, the caching in _views_discover_default_view() doesn't get rebuilt until the $index changes, and that won't happen just because a default view gets added, and when it does get rebuilt, it'll update the cache entries for individual views as well. So functionally this ought to still be the same as now. However I'd really appreciate some reviews of this. Going to do some additional profiling myself to try to validate some of the above.

dawehner’s picture

So here is a rerole against views3.

The changed code looks fine. It makes sense to avoid the discovering of default views as much as possible.

For database-only views it would make sense in views_get_view to not load the default view at all. So what about adding a parameter to views_get_view whether the default view should be loaded too.

catch’s picture

FileSize
225.94 KB
228.05 KB

dereine pointed me to the issue where this was introduced: #570558: Split up views_default_views cache to improve memory usage.

Note this patch removes double static caching of the default views array, I tried xhprof profiling with just that change, and couldn't see a measurable memory difference, so it may be that at least in PHP5, the static cache just holds the pointer to the object anyway.

However I created a very basic view in the UI, the profiled the page it was on, and confirmed that this reduces memory usage on the site I'm profiling for that page, from 85mb down to 73mb - you can see the 13mb used by views_discover_default_views() in the screenshot, the function isn't called at all with the patch applied.

bendiy’s picture

: 0

catch’s picture

Title: _views_discover_default_views() is prone to race conditions » views_get_default_view() - race conditions and memory usage

Retitling since the memory issue, while not a functional bug, is going to affect a large number of sites.

merlinofchaos’s picture

Assigned: Unassigned » bojanz

Assigning to bojanz for review and testing.

catch’s picture

Version: 6.x-2.11 » 6.x-2.x-dev

Moving this into the 2.x-dev queue, not sure why it was posted against the point release, there are patches here for both 2.x and 3.x though now.

mikeytown2’s picture

subscribe from #1003716: views_menu_alter fails sometimes; views_cache_get() issue in _views_discover_default_views()

Might want to check out my patch; it's simple and avoids the MySQL max packet size issue. Doesn't take into account multiple writes to the cache; seems like the locking framework would solve that. Also doesn't take into account the memory leak; but that seems to be independent of the issue I address.

catch’s picture

That patch has a typo, $cache_bad vs. $views_cache_bad

I'd considered doing this a few times but convinced myself that it would cause constant cache rebuilding for views that legitimately don't have default views, however since it's only looking for views that are in the index list, that shouldn't be an issue. Trying to remember how I ended up at that now...

So yeah the main issue then would be turning the race condition into a cache stampede, and a lock should fix that.

On the memory leak, at least some of that could be dealt with by #1000898: Don't load default view by default in views_get_view(), in fact that might be enough for the worst cases.

I'll have a look at re-rolling your patch with the locking framework addition. Would definitely be nice not to maintain that huge array.

mikeytown2’s picture

FileSize
2.12 KB

typo fixed with locking in place

mikeytown2’s picture

FileSize
2.19 KB

last patch introduces a 1 second delay; this patch should take care of that.

catch’s picture

FileSize
2.98 KB

OK here's a re-roll of mikeytown2's patch with the following changes:

I changed $views_bad_cache to $rebuild_cache.

Also added locking - if we get a miss for either the index or an individual view it tries to acquire a lock. If that succeeds, the cache rebuilt, if another process has the lock, it calls lock_wait(), tries to get the cache item again, and if that fails, goes ahead and rebuilds the cache. This is the same basic approach as #802446: Cache stampede protection: drupal_render() and page cache.

catch’s picture

Crossposted with #20 and #21, looking at #21 I'm not sure why we need to check the lock every time there's a miss on the static cache as well as the cache_get(), also there's no lock_release(). I'm split at the moment whether the extra complexity of trying to avoid a stampede when individual items are missing from the cache is worth it - that stampede would happen under the same circumstances as the original bug here.

mikeytown2’s picture

It seems like the only time the 'views_default_views_index' cache will be a miss is when it is being rebuilt; so might as well wait for it to be rebuilt. Or was my assumption wrong in this case?

catch’s picture

I meant this:

   if (!isset($cache)) {
+    // Wait for the cache to be rebuilt.
+    if (!lock_may_be_available('views_default_cache')) {
+      lock_wait('views_default_cache');
+    }
+

$cache is only a static, and http://api.drupal.org/api/drupal/includes--lock.inc/function/lock_may_be... queries the db, so calling this during the first call to _views_discover_default_views(), as opposed to if $index is a cache miss is going to add extra work even though we might not eventually need to rebuild the cache at all. At the moment this happens for every view that's in the database whenever it's viewed.

mikeytown2’s picture

FileSize
2.36 KB

Good point; not the most elegant way to accomplish this but it should make the 99% of the time use case be faster.

mikeytown2’s picture

FileSize
3.27 KB

used your patch as a base and added in the check for a views_default_views_index miss due to it getting rebuilt.

Added back in the isset check; won't that throw a notice if you try to access $index->data on a bool?

mikeytown2’s picture

Status: Needs review » Needs work

Patch #27 gets an extra element in the array at the end. Working on a solution.

mikeytown2’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

Found the issue
$data->data;
vs
$cached->data;

mikeytown2’s picture

FileSize
3.09 KB

fixed the cache miss with a rebuild in progress case;

$cache = _views_discover_default_views();
$rebuild_cache = FALSE;

is the answer.

mikeytown2’s picture

FileSize
3.1 KB

lock_wait(); was missing $lock_name in the last patch.

Anonymous’s picture

i got here from #402896: Introduce DrupalCacheArray and use it for drupal_get_schema().

this locking code looks weird, because the cache-miss scope is way, way finer than the scope of the lock. someone hit me with a cluebat if this is off base, but why do we rebuild the entire views cache if we get a miss for a single view?

also, the code seems to err on the side of performance over data integrity - is this intentional? (we set the cache even if we didn't acquire the lock.)

catch’s picture

If there's a cache miss for an individual view we need to load and alter every view to be able to cache the individual items. Also there's currently no code path via which an individual view should be a cache miss - that only happens with the race condition that prompted this issue, or if a cache item goes missing (like a memcache eviction or similar), so trying to save resources in that case didn't seem very useful.

I haven't reviewed the latest patch here yet but I'm sure it's not intentional to set the cache even if we didn't acquire the lock.

catch’s picture

FileSize
3.65 KB

Added a check to see if we acquired the lock before setting the cache.

mikeytown2’s picture

been using this patch in production for the last 3 weeks. works quite well.

catch’s picture

Issue tags: +memory

Tagging. I have this running in production on two large sites now - both of which were experiencing 'missing' views prior to the patch.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

based on last 2 posts, rtbc.

EvanDonovan’s picture

Subscribing.

merlinofchaos’s picture

Status: Reviewed & tested by the community » Needs work

Applies to 2.x but not 3.x -- need patches for both.

Right now 7.x is using the CTools export, which means none of this applies. Though it should -- we're going to have to do something with CTools to bring back default view caching. It doesn't actually have it.

catch’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

Is there an issue for ctools? Loading all the default views is very, very expensive (i.e. takes 12mb of memory on the same site I originally found this bug on and iirc), more in #1000898: Don't load default view by default in views_get_view().

Here's an (untested) patch for 6.x-3.x, the reason it didn't apply was the $reset argument added to the function, I don't have time to figure out what's going on with git blame (all lines are from the migration in the 3.x branch) to figure out where that commit came from.

merlinofchaos’s picture

There isn't one; it may be worth opening one so we can get something like this in before we roll another alpha.

catch’s picture

OK, opened a ctools issue at #1102252: Caching of defaults.

Renee S’s picture

catch, I tried this with V3 and it appears to be doing its job. Thanks =)

mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community

I think we can move this back to RTBC

merlinofchaos’s picture

I feel like the lock/rebuild code is kind of spaghettified, that's a lot of if/else conditions and it's difficult for me to follow. We could do this a LOT more cleanly if we made that a function.

See http://drupalcode.org/project/ctools.git/commitdiff/88edd92cb7da955191ec... for how I did it in CTools. The logic feels MUCH cleaner to me.

merlinofchaos’s picture

Status: Reviewed & tested by the community » Needs work
crea’s picture

Subscribing

catch’s picture

Assigned: bojanz » catch

I'm going to revisit this soon, based on #402896: Introduce DrupalCacheArray and use it for drupal_get_schema() (although with different code paths for PHP5 vs. 4), assigning to me for a while.

catch’s picture

FileSize
13.82 KB

OK here's initial work, it's not ready yet but good enough for profiling.

I profiled a real Drupal 6 site (with over 200 default views - some custom, some provided from modules). Ran drush cc all, then refreshed the front page. All caches are being rebuilt but the overall numbers seem accurate. I'll try to run this again to verify.

Without the patch:

Total Incl. Wall Time (microsec):	8,632,614 microsecs
Total Incl. CPU (microsecs):	4,560,000 microsecs
Total Incl. MemUse (bytes):	107,473,224 bytes
Total Incl. PeakMemUse (bytes):	108,029,128 bytes
Number of Function Calls:	473,512

With the patch:

Overall Summary
Total Incl. Wall Time (microsec):	6,113,706 microsecs
Total Incl. CPU (microsecs):	5,600,000 microsecs
Total Incl. MemUse (bytes):	92,942,928 bytes
Total Incl. PeakMemUse (bytes):	95,516,384 bytes
Number of Function Calls:	668,537

Here's a high level comparison:

The current code loads all views into memory at the same time, runs hook_views_default_views_alter() on them, then cache_set() for each view + cache_set() for the index of all views.

The patch keeps the index, however it builds both indexes and the individual views caches themselves, by looping over the modules that provide default views.

The view objects get built for each module, processed, then go out of scope again. This should mean that peak memory is kept under control. I am not seeing this in xhprof, but xhprof doesn't handle memory that is used then freed up well, and the summary + devel query log suggest that what I'd expect to happen is actually happening, but would be good to confirm.

In terms of performance, note this only works when not every view is actually requested during the same request, some of these assumptions are unverified - although they should hopefully be true by the time this is committed.

- memory usage on cache misses (for all views or just individual ones) is kept to the maximum from one module's default views, not all combined.

- There should be much less than 220 cache_set() when discovering default views - it'd just be whichever views are needed on the page.

- There is a CPU cost - for example if 5 default views are a cache miss, it will keep rebuilding the views over and over again to cache each one - since only the view actually being requested gets cached. This is a direct trade for memory + database inserts though - and doesn't look too bad in the profiling summary.

- it will take several requests for the views cache to build - however there is more or less a choice between 1. stampedes/race conditions as there are now 2. locking as with the older patch on here (which just means other requests hang), or 3. building over time.

The patch could completely drop the index of default views, which would allow individual views to be cached a lot quicker and make complete cold starts a lot less expensive. So more to do here but posting progress. Will post new profiling details when the patch is a bit more firmed up (probably next week).

catch’s picture

FileSize
3.1 KB

Got a bit further with this, but I had not really counted on http://drupalcontrib.org/api/drupal/contributions--views--views.module/f...

The two places that really concerns us are views_get_applicable_views() (called via hook_menu_alter()), and hook_block().

With the current state of the patch, the CacheArrayObject doesn't handle iterating over all views - just the lazy loading bit.

So that means a couple of things:

There are two different use cases.
- The first is requesting a specific default view, and getting the cached object back - patch handles this.
- The second is iterating over every view on the site to look for specific properties (mainly for menu and block at least in core views code).

We could have two different code paths for this (have views_get_default_view() not use views_discover_default_view() at all) - that would make the patch workable for this case.

However I'm pretty sure views_block('list'); is going to run on pretty much any Drupal 6 site after a cache clear, and that needs to go through and have all loaded views objects.

What I'm thinking, is define an iterator in the CacheArrayObject implementation. And this iterator would get the default view and return it, but ensure it's not static cached anywhere. If we did that, the memory from the default views that are being iterated over should be freed up after each individual one is checked. While individual views requested directly can be built and cached separately same as in the current patch.

This is tricky though so need to give it some more thought.

catch’s picture

FileSize
14.56 KB

Helps to upload the complete patch.

catch’s picture

New patch adding an iterator class.

Here's how the xhprof summary looks now:

Patch from #34 (same rough profile as now for one request on a cold start):

Total Incl. Wall Time (microsec):	6,730,274 microsecs
Total Incl. CPU (microsecs):	4,570,000 microsecs
Total Incl. MemUse (bytes):	114,028,216 bytes
Total Incl. PeakMemUse (bytes):	114,573,696 bytes
Number of Function Calls:	479,340

With the patch:

Total Incl. Wall Time (microsec):	7,961,507 microsecs
Total Incl. CPU (microsecs):	5,430,000 microsecs
Total Incl. MemUse (bytes):	110,691,872 bytes
Total Incl. PeakMemUse (bytes):	113,097,216 bytes
Number of Function Calls:	620,417

So at the moment this is saving around 3mb of memory, 1mb peak memory. This is in exchange for quite a bit of CPU cost, since we're no longer static caching all the default views (on this request five different default views were a cache miss).

It may well be possible to improve on this, for example storing an array of module and view names somewhere so we don't have to foreach() over them the second or third time into the function to pick out views when we already know where they live. Potentially that could reduce the CPU cost by half or more.

However this version also successfully gets the number of cache_set() (on a Drupal 6 site fromviews_cache_set() to 5, instead of 206 currently.

So to summarize the trade-offs at the moment:

CPU/walltime usage on cache misses is higher (may be fixable).

Memory usage on cache misses is lower by a mb or more (possibly improvable upon, this is disappointing compared to what I'd hoped).

Database inserts are limited to views actually requested, not all of them - on the site I'm testing this is 5 instead of 206. On a 'real' site this may be more of an issue than the extra CPU cost, I'm profiling on my old laptop with memcache cache backend for these tests.

There is still no single large cache entry to worry about.

There's no locking at all - with the current patch from #34, every single request is blocked on the one that acquires the lock, until it has finished loading and caching every view - there was no way around this without hitting the race condition. With the patch there is no locking, so if you have several pages that are requesting different views, they can all build their own caches in parallel.

I will see if I can improve the CPU/Wall time, and try to sanity check the memory usage a bit more once that's done, so leaving at CNW for that.

catch’s picture

FileSize
16.53 KB
catch’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
9.69 KB

OK this fixes the @todo in #52.

Additionally I realised it was overriding every single method in CacheArrayObject, so it now extends ArrayObject directly. That cuts about 100 lines off the patch size.

With that dealt with, the xhprof summary now looks like this:

Current code:

Total Incl. Wall Time (microsec):	5,066,427 microsecs
Total Incl. CPU (microsecs):	4,250,000 microsecs
Total Incl. MemUse (bytes):	115,687,248 bytes
Total Incl. PeakMemUse (bytes):	116,232,880 bytes
Number of Function Calls:	480,851

Devel says:
Memory used at: devel_init()=6.24 MB, devel_shutdown()=112.03 MB.

Latest patch:

Total Incl. Wall Time (microsec):	5,366,566 microsecs
Total Incl. CPU (microsecs):	4,330,000 microsecs
Total Incl. MemUse (bytes):	91,610,496 bytes
Total Incl. PeakMemUse (bytes):	92,175,448 bytes
Number of Function Calls:	495,196

Devel says: Memory used at: devel_init()=4.48 MB, devel_shutdown()=88.92 MB.

So a high level view of the benefits of this approach both compared to the original patches on this issue and the current code.

- over 20mb of peak memory saved on cache misses.
- no significant increase in PHP execution time (may vary a bit up and down depending on which specific views are requested on cache misses).
- it is not necessary to cache_set() every default view before one can be used, this saves 200 calls to cache_set() on the client site I'm working on this for when there's a miss on the default views cache.
- Due to this there's no need for a global read lock around the default views generation code, and there's no race condition where views can go missing from not having the lock.
- this works both for cache misses on default views, and also for iterating over all default views such as views_block('list') does.

Most of the extra code weight now (it is 150+ lines of code added) is due to the two code paths, I'm hoping it'll be half that amount of less for the ctools D7 port where we can have PHP5.

I didn't attempt to solve the problem for PHP4 at all, that logic is directly copied from the current includes/cache.inc

There is probably still some cleanup to do here, but I'm going to mark it as needs review, since from a performance perspective this is as good as I think I can get it.

Anonymous’s picture

will review this tomorrow.

catch’s picture

This is working well for the default views cache, but the iterator isn't happening properly yet:

- views_load_all_views() - this takes all the default views, iterates over them, then static caches the result. The iterator isn't working with that, but when it does, it's just going to return views that then get statically cached and not fix the memory issue.

This means to have the memory savings for that as well (mainly for views_block() in terms of normal usage), there's a couple of options:

- have views_block() do some of the logic of views_load_all_views() so it doesn't actually need access to them all in memory. That won't fix the other places that use views_load_all_views()

- have views_load_all_views() also return an ArrayAccess object, that starts with the views in the db and moves onto the default ones, not sure how feasible that is.

We can scale this back to only handling missing items in the default views cache, for which it is working fine (and that'd be a simpler patch).

Having the iterator work so you can foreach() over all views without eating the memory all at one point I'm not sure if that's doable yet given the end user facing API merges views from the database with ones from default storage. Will need to give it some more thought again.

Leaving at needs review since I could use the feedback at this point.

Anonymous’s picture

Status: Needs review » Needs work

maybe i'm missing something, but this part of the default_views_63.patch looks likes its treating $index in funny ways?

like, checking isset($this->index[$offset]), but then referencing $index[$offset]?

also, if ($index == $offset) { looks like that should be if ($key == $offset) { or something?

+    if (isset($this->index[$offset])) {
+      $views = views_initialize($index[$offset]);
+      $this->offsetSet($offset, $view);
+      $view_clone = clone $view;
+      $view_clone->destroy();
+      views_cache_set('views_default:' . $offset, $view_clone, TRUE);
+      return $view;
+    }
+    else {
+      foreach (module_implements('views_default_views') as $module) {
+        $views = views_initialize($module);
+        foreach ($views as $key => $view) {
+          $index[$key] = $module;
+          if ($index == $offset) {
+            $this->offsetSet($offset, $view);
+            $return = $view;
+            $view_clone = clone $view;
+            $view_clone->destroy();
+            views_cache_set('views_default:' . $offset, $view_clone, TRUE);
+          }
+        }
+        // If one of the views provided by the module is the one we're looking
+        // for, cache it then return.
+        if (isset($index[$offset])) {
+          return $return;
+        }
+      }
+    }
catch’s picture

Status: Needs work » Needs review
Issue tags: -memory

Did not deal with beejeebus' feedback yet and there's a lot more cleanup to do here. However this one gets the iterator working in views_get_all_views(). Did not performance test that compared to current code yet.

catch’s picture

Issue tags: +memory
FileSize
16.55 KB

Did not deal with beejeebus' feedback yet and there's a lot more cleanup to do here. However this one gets the iterator working in views_get_all_views(). Did not performance test that compared to current code yet.

catch’s picture

FileSize
15.46 KB

Here's xhprof output from admin/build/block:

Before:

Total Incl. Wall Time (microsec):	8,918,566 microsecs
Total Incl. CPU (microsecs):	7,000,000 microsecs
Total Incl. MemUse (bytes):	110,493,056 bytes
Total Incl. PeakMemUse (bytes):	142,347,776 bytes
Number of Function Calls:	1,236,732

After:

Total Incl. Wall Time (microsec):	10,045,530 microsecs
Total Incl. CPU (microsecs):	8,040,000 microsecs
Total Incl. MemUse (bytes):	95,833,992 bytes
Total Incl. PeakMemUse (bytes):	127,017,792 bytes
Number of Function Calls:	1,327,169

So again higher CPU usage, but saving ~15mb of memory.

Also posting a cleaned up version.

The ViewsDefaultViewsCache class now just implements ArrayAccess - does not have an iterator (this can be added if required).

The ViewsAllViews class implements iterator, does not implement ArrayAccess (this could also be added).

Didn't mean to set status back to needs review yesterday, but that's the right status now.

I added some clarifying comments for the $index stuff, those could probably be improved. It /might/ be worth caching the $index persistently (but not requiring it) to save some CPU foreaching over the views on misses.

dawehner’s picture

Status: Needs review » Needs work

Didn't reviewed the php4 part as it doesn't seem to make sense here.

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+  private $storage = array();
...
+  private $index;

Would it be possible to make them protected so a sub-class could perhaps reuse it?

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+   * An array of view_name => module pairs to locate views quicker when requested. ¶

Really minor: space at the end.

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+    // the view as value. There is no way to find out which view belongs to
+    // which module without iterating over all views until we find it. However

Wouldn't it perhaps be possible to store this index in a cache as well? No idea whether this would actually improve the performance.

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+      $views = views_initialize($index[$offset]);
+      $this->storage[$offset] = $view;
+      $view_clone = clone $view;
+      $view_clone->destroy();

I'm a bit confused about this lines as $view isn't created yet in this context. Seems to a somehow a mistake

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+  return new ViewsAllViews;

Just for Consistency it would make to use ()

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+    $default_views = views_initialize($module); ¶

Really minor: space

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+function views_initialize($module) {

This name could confuse some people, what about views_initialize_module_views

corbacho’s picture

subs

catch’s picture

FileSize
16.27 KB
0 bytes

New patch, I think this addresses the review.

Didn't reviewed the php4 part as it doesn't seem to make sense here.

PHP4 is just the existing code moved around.

Would it be possible to make them protected so a sub-class could perhaps reuse it?

Done.

Wouldn't it perhaps be possible to store this index in a cache as well? No idea whether this would actually improve the performance.

Yeah I had @todo for this in my head and on balance I think it's better to cache the index yeah. If you have five pages with different views on each, it will reduces the chances that pages 2-5 need to do the foreach() to locate the default when building the cache.

I went for caching after the processing of each module. This gives other processes a better chance of a cache hit during cold starts. There's also a check to ensure we're actually caching more than has already cached when multiple processes are all working at the same time, otherwise there's a risk of undoing work.

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+ $views = views_initialize($index[$offset]);
+ $this->storage[$offset] = $view;
+ $view_clone = clone $view;
+ $view_clone->destroy();
I'm a bit confused about this lines as $view isn't created yet in this context. Seems to a somehow a mistake

Yeah that's a bug, I think I reproduced and fixed but did not try to force it deliberately yet.

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+ return new ViewsAllViews;
Just for Consistency it would make to use ()

Fixed.

+++ b/includes/php5.incundefined
@@ -0,0 +1,189 @@
+function views_initialize($module) {
This name could confuse some people, what about views_initialize_module_views

Agreed.

catch’s picture

Status: Needs work » Needs review
FileSize
16.27 KB

0 byte patch. Also back to CNR.

dawehner’s picture

In general i quite like the patch from a code perspective but it makes things more complicated then it used to be.

So i'm not sure whether this patch will be applied to views 2.x, because we already declined a lot of other patches, but they weren't major.

Additional as the patch is, it can't be ported directly, because in d7 ctools is used. This kind of stuff would probably have to be moved to ctools, which would be a great overall memory improvement.

catch’s picture

FileSize
16.27 KB
16.27 KB

Here's an updated 6.2 patch, just fixes a typo.

There's a 6.x-3.x version of the patch, I'm maintaining both mainly because the site I'm working on is using 6.x-3.x

I don't have any 7.x sites using Views at the moment, but when I do I'd likely end up porting this to 7.x ctools for one of those pretty fast though.

I'd be up for working on this for ctools before that happens too, but probably not without an indication it's got a very good chance of getting in.

bojanz’s picture

I think the improvement is big enough for it to break the Views 2.x rule of "no more big patches".

In any case, looking forward to seeing this land in Views 3.x. Great job, catch!

catch’s picture

Found another bug ironically introduced by fixing the previous typo.

Uploading new patches for both versions.

catch’s picture

Not quite.

hefox’s picture

(Subscribe. Since it's a race condition it's sorta hard to test, but will see if views go missing again -- resulted in an 404 for some menu items :(.)

hefox’s picture

Before if a implement of hook_views_default_views_alter didn't return an array, it didn't matter cause module_invoke_all don't care; now it runs drupal_alter on the direct return value, which may or may not be an array (aka trust the module to return correctly or not?).

So, should the patch be updated, or the offending module? #1284556: flag_views_default_views returns wrong value when bookmark flag doesn't exist is an update for flag.

catch’s picture

Do you mean hook_views_default_views() returning NULL? There's a similar issue for signup here: #1266764: php: Notice: Undefined variable: views in signup_views_default_views().

I've been treating those as bugs in the modules so it's good if there's issues, but for BC we could check that condition and not run the alter on it.

hefox’s picture

Status: Needs review » Needs work

Yep; co-worker is updating the patch for above (checking hook_views_default_views returns something) and for line 38, it returns $cache object instead of an $cache->data array resulting in fatal errors when things try to access this object like an array.

catch’s picture

FileSize
16.63 KB
16.61 KB

Here's a new patch that should deal with both the issues hefox brought up.

catch’s picture

Status: Needs work » Needs review
InternetDevels’s picture

Status: Needs review » Needs work

After applying patch default_views62.patch from previous comment some views on site dissapeared. I added reset($this->dbViews); on line 138 in __construct() method of ViewsAllViews class in includes/php5.inc file and it fixed problem. It looks like foreach() moved internal array pointer to last element and next() method worked incorrectly. Please look into this, maybe other patch should be also changed.

Souvent22’s picture

Subscribing.

hefox’s picture

Some things I've noticed

1) Fatal error due to removing a module and not clearing cache -- not unreasonable I guess, but a function_exists might be nice
2) Error due to defining a view in a hook_views_default_view_alter-- er, yea, not sure how to handle it. In this case the view was overriding an existing view without checking that view was defined, so added that check to fix it. Looking at what the patch does, not sure how it'd handle that case. It expects every view to have a defining module.

catch’s picture

Status: Needs work » Needs review
FileSize
16.63 KB

#76 looked right to me, added the reset().

#78 - I found one place where the $reset argument wasn't being passed to clear caches, that might have fixed this (unless you were just removing modules from the filesystem without disabling them).

Defining a new view in hook_views_default_views_alter() isn't supported by this patch, or at least not the same way - the only way to get the full memory savings is to prevent every single default view from being loaded into memory at the same time, which means alter has to run per-module instead of all at once.

Uploading new 6.x-3.x patch, not updated the 6.x-2.x patch yet though. Back to needs review, thanks for the reviews and testing so far!

catch’s picture

bdurbin found another bug with this - default views weren't appearing in the views admin ui list (and other stuff related to $view->type for default views returned by ViewsAllViews. that's now fixed in this patch.

Dmitriy.trt’s picture

Status: Needs review » Needs work

PHP4 implementation still have problems with partially broken cache (it happens sometimes when memcache is experiencing low memory and removing some cache entries). Try to manually remove any of cache entries named views_default:$view_name:$language and view will be broken until the next cache flush.

Is there any reason why default views are stored in cache and not in views_view and views_display tables? We could add one more column to indicate storage type (normal, default or overridden). Benefits from this solution are:

  • Simplified logic when loading single view and list of views. Now when loading single view we search for it in both database and cache, so it wouldn't add DB query. For "views cache disabled" mode we could still use expensive default views discovery, but when it is disabled - simply fetch view from database.
  • Independence from cache integrity, because views_view and views_display are not cache tables.
  • Easy way to use Drupal lock framework: just protect default views rebuilding function and then get views from database. Now cache can be broken again right after rebuild (with memcache) and attempt to use lock framework adds danger to end up with infinite recursion (in the worst case, of course, but there is a chance).

I'd like to implement this solution. So, is there any reason to not do it?

catch’s picture

Version: 6.x-2.x-dev » 6.x-3.x-dev
Status: Needs work » Needs review

The patch doesn't attempt to fix anything for php4. Storing in the db should help with the cache integrity issue but is not going to help with the memory issues or the expensive of rebuilding caches.

catch’s picture

Fixes a couple of notices, should not be any functional changes.

catch’s picture

And minor cleanup for that previous patch - just moved the initialization of $this->status to the top of __construct()

hefox’s picture

I was actually just removing a module completely, which generally is fine as much places do function-exists (it wasn't a module that had a hook_disable/hook_uninstall).

So the two things that are deprecated with this patch

  1. Defining a view in the alter
  2. Completely overriding a view with another in the alter (Not sure why this breaks, but it's totally does). Ie, going $views['my_view'] = new view; ...;
Dmitriy.trt’s picture

Status: Needs review » Needs work

Sorry, but PHP 4 version is producing parse error: code is repeated, including <?php, and curly brace is missing for last function.

catch’s picture

FileSize
14.24 KB

hmm looks like a patch applied twice. Try this one.

Dmitriy.trt’s picture

Status: Needs work » Needs review

What about requiring PHP 5 for Views 6.x-3.x? I have some progress on views stored in database for 6.x-2.x and even implemented most of functions with Catch's great idea to retrieve and process default views module-by-module instead of collecting them all at once, but views_get_all_views() is still consuming too much memory (without API changes and PHP 4/5 versions). With both solutions implemented we could fix both problems: storing default views in database (with lock-protected rebuild and caching individual views without index) fixes race condition and implementation of views_get_all_views() using Iterator could fix high memory usage. But maintaining both PHP 4 and PHP 5 versions is too much I think, life could be easier with PHP 5 requirement, at least for 6.x-3.x.

catch’s picture

Currently the PHP4 code is just the original code copied to an include. It makes the patch considerably more messy but I'm not prepared to spend time trying to fix this bug for PHP4 - if it's no worse than it is now that is plenty for me.

I know there's been some discussion of dropping PHP4 support in 6.x-3.x but that's really up to Earl. Would certainly make the patch a lot smaller, but with the conditional include it doesn't really add too much extra effort.

danmuzyka’s picture

We have a site that is affected by this issue, but the issue only occurs sporadically and unpredictably. Does anyone have a way of deliberately reproducing this issue in order to test the patches? Is anyone using load testing software to reproduce the race condition, or some other method?

Thanks catch and others for your work on this issue, and thanks in advance to anyone who has recommendations about how to test the patches!

webflo’s picture

Looks good. There is an api function for minimum version.

+++ b/includes/php4.incundefined
@@ -0,0 +1,93 @@
+            if (!empty($view->api_version) && $view->api_version >= 2) {

+++ b/includes/php5.incundefined
@@ -0,0 +1,214 @@
+      if (!empty($view->api_version) && $view->api_version >= 2) {

Replaced 2 by views_api_minimum_version(). New patch is attached.

webflo’s picture

Status: Needs work » Needs review
catch’s picture

To reproduce randomly you can delete some of the individual cache entries for default views without the patch applied. That should have the effect of making the default views disappear.

I've seen this issue on about five different sites now (often blamed on memcache but it is easy to recognise once you've seen it once). Was not able to reproduce with load tests though.

dstol’s picture

The version numbering has dropped off the patches, is it fair to assume that the last 6.x-2.x patch was #74?

danmuzyka’s picture

Thanks catch, I'll go ahead and try doing that to reproduce the issue.

catch’s picture

Added some defensiveness to cover hefox's case of removing a module, better not to fatal there. Drupal 7 is less forgiving if you remove modules from the file system but D6 mostly lets you get away with it. This includes the cleanup from #91 as well.

JoeMcGuire’s picture

Patch was breaking the current version of Features showing the error "Cannot use object of type ViewsAllViews as array".

This was because Features was expecting an array interface. Luck would have it I've been able to commit a patch to Features also which solves this:
#1346188: Complex syntax after views_get_all_views()

That said I think it's necessary to add the ArrayAccess interface here for backward compatibility.

JoeMcGuire’s picture

Missed of a line from the previous patch.

dstol’s picture

Here is an equivalent of #98 for those of us in the dark ages of 6.x-2.x.

mikeytown2’s picture

#99 throws a warning for me in the context module Warning: ksort() expects parameter 1 to be array, object given when I do a cache flush.

Code in question in short:

  $views = views_get_all_views();
  ksort($views);

One more notice as well on a normal page view
Notice: Undefined variable: status in ViewsAllViews->next() (line 175 of sites/all/modules/views/includes/php5.inc).
For this notice the view is enabled; but the view returns nothing; its a views for a block.

dstol’s picture

Status: Needs review » Needs work

Seems there are bigger problems with #96, #98 and #99.

All default views are missing for both 6.x-2.x and 6.x-3.x.

thebuckst0p’s picture

Subscribe. I've been dealing with an issue that is probably the same as the one described here on a large site for the last year. The obvious symptom is, a certain view disappears from the homepage. In some cases the page title disappears too, not sure why that's related. So the klugy workaround we came up with was a script that runs on cron every 5 minutes, checks if the block is missing (via curl|grep), and if it is, clear the menu cache. That would fix it, sometimes for weeks, sometimes for only 15 minutes. Why the menu cache solved the problem wasn't clear until I read this thread -- but as catch noted in #50, this process can be triggered by hook_menu_alter -- so now that part makes sense.

We're running D6, (a fairly recent) Views 2.x-dev, Apache load-balanced over 3 servers, MySql and memcache on one server. Most of the Views are in Features, and the issue seems to happen more frequently when the view is "updated" in the feature, i.e. running as a default view rather than from the DB (which makes sense given this thread).

It looks like there's not yet a stable patch for 6.x-2.x, I can try to help test patches if that would be useful.

Other than my 5-minute cache-clear kluge, what other workarounds have people come up with for this (seeing as it's an unresolved issue on many production sites)?

Thanks for all the work everyone has put into this.

mikeytown2’s picture

#34 is considered good. The patches after the refactoring (like #99) result in faster code, but #34 fixes the disappearing view. We use #34 on production.

thebuckst0p’s picture

Thanks mikeytown2. It's unclear from the thread what version #34 is for - is it for 2.x?

mikeytown2’s picture

That is for 2.x and can be adapted to 3.x if needed.

catch’s picture

Status: Needs work » Needs review
FileSize
14.96 KB
ksort($views);

That's just not going to be supported if this patch lands, can't do anything about it here but it'll need a followup for context module once we know this is getting in.

The status notice is a typo in the patch - needs to be $this->status. Uploading a new patch to fix that, which I think puts this back at CNR.

dstol’s picture

Status: Needs review » Needs work

I'm seeing the same problems with #106 that I was in #101. Check out http://skitch.com/unncola/gscx6/views-6testbed

dawehner’s picture

I'm not 100% sure but maybe implementing IteratorAggregator would help to solve the ksort() Problem.

mcarbone’s picture

[removed, posted before thinking]

catch’s picture

Status: Needs work » Needs review
FileSize
14.92 KB

iteratorAggregrator won't help here, PHP's array functions just don't work with SPL types, although I think there's been some discussion of trying to fix this in later PHP versions.

The ArrayAccess implementation in ViewsAllViews wasn't correct - there's no guarantee in that class that the storage contains any particular view, so instead I've made it just call out to views_get_view() for offsetGet() and offsetExists().

I can't reproduce the problem that dstol is having, so this doesn't try to address that, but marking back to CNR in the hope of more testing - I'll update a client site with this patch soonish too.

catch’s picture

Slight update - previous patch didn't include some fixes I had locally.

dstol’s picture

Good news, while you say that the patch didn't address the issues I was having earlier, it seems to do just that.

In a quick smoke test, I'm not seeing any of the issues I described in #107, with the patch in #111. I'll need to test it out more throughly.

dstol’s picture

FileSize
1.77 KB

I was curious about what changed between #106 and #111. Not all that much, interdiff attached. I'm curious if I just can't reproduce my issues. I'll need to give it a try on the same site I was looking at originally.

m2jalali’s picture

hi
when i used this patch #111
Fatal error: Call to undefined function views_api_minimum_version() in C:\xampp\htdocs\pressflow\sites\all\modules\views\includes\php5.inc on line 235
please guide me

catch’s picture

dstol’s picture

Status: Needs review » Reviewed & tested by the community

Using #111 I'm unable to reproduce my earlier issues. I gave this a good testing and couldn't find issues. RTBC.

Just an aside for the maintainers, I'd love to see this in 2.x. I'm not exactly sure of the maintenance status of 2.x, considering there haven't been commits in 2 months. Would you consider a commit for 2.x pending a RTBC backport?

catch’s picture

fwiw while I'd love to see #111 in 6.x-3.x, I think #34 is safer for 6.x-2.x since there's absolutely no API change whatsoever with that patch - there's no performance/memory improvement at all but it stops the race condition (which I've seen on every Drupal site I've worked on in the past year more or less).

hefox’s picture

I've been using one of the versions of the patch with memory improvements with views 6.x-2.x without much issue (the memory improvements are nice). The only problems I've had it is when a module was doing $views = views_get_all_views(); foreach ($views as $view) { $views[$view->name]}, which was totally silly and since been fixed in the dev version. 'Twas features, so if views released without before features did, ouch.

catch’s picture

That should actually be doable with the latest version of the patch, since it's also implementing ArrayAccess now. I haven't done anything with features for a while, but the original site I wrote this patch for is, and no issues have been reported.

dstol’s picture

Just to be clear...

6.x-2.x #34 is RTBC per #37. I'll also add a +1 for it as I've been running it all this time.
6.x-3.x #111 is RTBC per #116.

yched’s picture

Status: Reviewed & tested by the community » Needs work

Got a couple remarks looking at patch #111 - coming late to the party, sorry if those have been addressed before.

+++ b/includes/php5.incundefined
@@ -0,0 +1,242 @@
+    $this->views = reset($this->dbViews);

Looks odd, reset($this->dbViews) will return the first db view.
Shouldn't that be reset($this->dbViews); $this->views = $this->dbViews; ?

Also, is it alright that rewind() populates $this->views with db views + default views of the 1st module, while a fresh new object out of __construct() will only contain db views ?

+++ b/includes/php5.incundefined
@@ -0,0 +1,242 @@
+function _views_get_all_views($reset = FALSE) {
+  return new ViewsAllViews();
+}

No static ? We really create a new object on each call ?

catch’s picture

Looks odd, reset($this->dbViews) will return the first db view.
Shouldn't that be reset($this->dbViews); $this->views = $this->dbViews; ?

Yes it should.

Also, is it alright that rewind() populates $this->views with db views + default views of the 1st module, while a fresh new object out of __construct() will only contain db views ?

That's fine I think no-one can or should access the data in the class directly.

No static ? We really create a new object on each call ?

Yes that's actually intentional. If we static cache then we're keeping at least the database views in memory all the time, whereas the function is (usually) only called very rarely - admin pages and cache misses and I've never seen it called more than twice on a page. So I went for allowing that memory to be freed up at the cost of potentially more CPU usage.

fenda’s picture

Perhaps my situation is related.

On occasion (cache clearing, module page form save, add a field) my database is returning this error in the mysql log:

120606 5:32:10 [ERROR] Got error -1 when reading table './cyac/cache_views'

I then must run /etc/init.d/mysqld restart for my site to become responsive.

Did a successful cache clear 10 minutes and checked cache_views. views_data:ja and views_data:en are showing 500kb+.

Any 'quick fix' for this? Happening on a production server fairly often.

geerlingguy’s picture

Is this still an issue with Drupal 7? (Issue set to 6.x-3.x). Only reason I ask is because I have encountered some strange behaviors on a D7 site while clearing caches, and it seems like it could be related to this issue.

catch’s picture

I believe Views is using ctools for this in Drupal 7, however I haven't reviewed the equivalent ctools code for some time.

mikeytown2’s picture

In regards to #34 I discovered an issue with it and the usage of lock_wait(). I have a cache grind of the server waiting 30 seconds (on pressflow). I think it should wait a lot less time (like 2 seconds).

Attached patch is based off of #34 and is thus for views 6.x-2.x

Looks like I messed this one up; patch is from #34 but was against views 6.x-3.x... going to re-roll some things here.

mikeytown2’s picture

This fixes the race condition. Improving the memory usage can be done with a followup.
6.x-2.x patch is based off of #34.
6.x-3.x patch is based off of #40.

The only difference between the 2 patches mentioned above and the 2 attached below is lock_wait() is set to 2 seconds.

Status: Needs review » Needs work

The last submitted patch, views-853864-127-fix-race-condition-6x-3x.patch, failed testing.

catch’s picture

FileSize
4.59 KB

Found an issue with the lower-memory version, includes yched's feedback from #121 also (which was related).

Jody Lynn’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev

There's some bad logic in the patch that was committed to 7.x: http://cgit.drupalcode.org/views/commit/?id=0ba1a86 that causes it to not be very effective.

After caches are cleared, tables will get cache misses in _views_fetch_data, resulting in (for the first table) the full views_data cache being loaded and kept in the static variable, and the missing table cache item saved. The next table will be found (in the static, not the real cache) and not get saved to the cache (despite that it too was a cache miss).

Say you have 200 tables that run through this function and are all cache misses. You will have to load the page 200 times before each one of them has been saved to the cache, and finally then you will not trigger the very memory expensive full views_data cache load. (Hopefully your cache has not been cleared again by that time!)

Either we need to be more careful with what we're doing with the static $cache variable, or we need _views_fetch_data_build to also build all the table cache entries.

cassio’s picture

Hey guys,

I really appreciate everyone's work on this and my ability to learn more about caching. What is the best way to mitigate this in a drupal 6 installation? Limit views caching? beef up server resources? downgrade views (which looks tenuous.)?

dawehner’s picture

Version: 7.x-3.x-dev » 6.x-3.x-dev

There's some bad logic in the patch that was committed to 7.x: http://cgit.drupalcode.org/views/commit/?id=0ba1a86 that causes it to not be very effective.

Note: This issue is not about views data but about default views, but we recently committed a fix for that views data, ... see last SA of views. Does that problem still exist in the wild? In case yes,
it would be great to open its own dedicated issue!

Let's move back to 6.x as it seems to be the version of Drupal where this problem actually appears.

Can someone quickly describe whether we need both the approach from @catch, see #111 and the approach from @mikeytown2 or whether one of them is enough?
In general I'd be fine with getting #111 but simply without the PHP 4 BC layer, we don't support it anymore anyway.

Chris Matthews’s picture

Status: Needs work » Closed (outdated)

The Drupal 6 branch is no longer supported, please check with the D6LTS project if you need further support. For more information as to why this issue was closed, please see issue #3030347: Plan to clean process issue queue