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.
Comment | File | Size | Author |
---|---|---|---|
#129 | 853864-memory.patch | 4.59 KB | catch |
#127 | views-853864-127-fix-race-condition-6x-2x-do-not-test.patch | 3.46 KB | mikeytown2 |
#127 | views-853864-127-fix-race-condition-6x-3x.patch | 5.05 KB | mikeytown2 |
#126 | views-853864-126-fix-race-condition.patch | 3.5 KB | mikeytown2 |
#113 | interdiff.txt | 1.77 KB | dstol |
Comments
Comment #1
Mark Theunissen CreditAttribution: Mark Theunissen commentedComment #2
Mark Theunissen CreditAttribution: Mark Theunissen commentedForgot to use the -p switch.
Comment #3
Mark Theunissen CreditAttribution: Mark Theunissen commentedNow that the locking framework is in Drupal 6 core, has anyone talked about adding it to the views cache rebuild?
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedI 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
Comment #5
Mark Theunissen CreditAttribution: Mark Theunissen commentedI 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.
Comment #6
Mark Theunissen CreditAttribution: Mark Theunissen commentedComment #7
ktha CreditAttribution: ktha commentedIn 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().
Comment #8
catchI 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.
Comment #9
catchHad 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.
Comment #10
catchThere'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.
Comment #11
dawehnerSo 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.
Comment #12
catchdereine 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.
Comment #13
bendiy CreditAttribution: bendiy commented: 0
Comment #15
catchRetitling since the memory issue, while not a functional bug, is going to affect a large number of sites.
Comment #16
merlinofchaos CreditAttribution: merlinofchaos commentedAssigning to bojanz for review and testing.
Comment #17
catchMoving 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.
Comment #18
mikeytown2 CreditAttribution: mikeytown2 commentedsubscribe 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.
Comment #19
catchThat 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.
Comment #20
mikeytown2 CreditAttribution: mikeytown2 commentedtypo fixed with locking in place
Comment #21
mikeytown2 CreditAttribution: mikeytown2 commentedlast patch introduces a 1 second delay; this patch should take care of that.
Comment #22
catchOK 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.
Comment #23
catchCrossposted 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.
Comment #24
mikeytown2 CreditAttribution: mikeytown2 commentedIt 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?
Comment #25
catchI meant this:
$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.
Comment #26
mikeytown2 CreditAttribution: mikeytown2 commentedGood point; not the most elegant way to accomplish this but it should make the 99% of the time use case be faster.
Comment #27
mikeytown2 CreditAttribution: mikeytown2 commentedused 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?
Comment #28
mikeytown2 CreditAttribution: mikeytown2 commentedPatch #27 gets an extra element in the array at the end. Working on a solution.
Comment #29
mikeytown2 CreditAttribution: mikeytown2 commentedFound the issue
$data->data;
vs
$cached->data;
Comment #30
mikeytown2 CreditAttribution: mikeytown2 commentedfixed the cache miss with a rebuild in progress case;
is the answer.
Comment #31
mikeytown2 CreditAttribution: mikeytown2 commentedlock_wait(); was missing $lock_name in the last patch.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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.)
Comment #33
catchIf 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.
Comment #34
catchAdded a check to see if we acquired the lock before setting the cache.
Comment #35
mikeytown2 CreditAttribution: mikeytown2 commentedbeen using this patch in production for the last 3 weeks. works quite well.
Comment #36
catchTagging. I have this running in production on two large sites now - both of which were experiencing 'missing' views prior to the patch.
Comment #37
moshe weitzman CreditAttribution: moshe weitzman commentedbased on last 2 posts, rtbc.
Comment #38
EvanDonovan CreditAttribution: EvanDonovan commentedSubscribing.
Comment #39
merlinofchaos CreditAttribution: merlinofchaos commentedApplies 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.
Comment #40
catchIs 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.
Comment #41
merlinofchaos CreditAttribution: merlinofchaos commentedThere isn't one; it may be worth opening one so we can get something like this in before we roll another alpha.
Comment #42
catchOK, opened a ctools issue at #1102252: Caching of defaults.
Comment #43
Renee S CreditAttribution: Renee S commentedcatch, I tried this with V3 and it appears to be doing its job. Thanks =)
Comment #44
mikeytown2 CreditAttribution: mikeytown2 commentedI think we can move this back to RTBC
Comment #45
merlinofchaos CreditAttribution: merlinofchaos commentedI 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.
Comment #46
merlinofchaos CreditAttribution: merlinofchaos commentedComment #47
crea CreditAttribution: crea commentedSubscribing
Comment #48
catchI'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.
Comment #49
catchOK 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:
With the patch:
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).
Comment #50
catchGot 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.
Comment #51
catchHelps to upload the complete patch.
Comment #52
catchNew 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):
With the patch:
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.
Comment #53
catchComment #54
catchOK 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:
Devel says:
Memory used at: devel_init()=6.24 MB, devel_shutdown()=112.03 MB.
Latest patch:
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.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedwill review this tomorrow.
Comment #56
catchThis 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.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedmaybe 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 beif ($key == $offset) {
or something?Comment #58
catchDid 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.
Comment #59
catchDid 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.
Comment #60
catchHere's xhprof output from admin/build/block:
Before:
After:
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.
Comment #61
dawehnerDidn't reviewed the php4 part as it doesn't seem to make sense here.
Would it be possible to make them protected so a sub-class could perhaps reuse it?
Really minor: space at the end.
Wouldn't it perhaps be possible to store this index in a cache as well? No idea whether this would actually improve the performance.
I'm a bit confused about this lines as $view isn't created yet in this context. Seems to a somehow a mistake
Just for Consistency it would make to use ()
Really minor: space
This name could confuse some people, what about views_initialize_module_views
Comment #62
corbacho CreditAttribution: corbacho commentedsubs
Comment #63
catchNew patch, I think this addresses the review.
PHP4 is just the existing code moved around.
Done.
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.
Yeah that's a bug, I think I reproduced and fixed but did not try to force it deliberately yet.
Fixed.
Agreed.
Comment #64
catch0 byte patch. Also back to CNR.
Comment #65
dawehnerIn 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.
Comment #66
catchHere'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.
Comment #67
bojanz CreditAttribution: bojanz commentedI 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!
Comment #68
catchFound another bug ironically introduced by fixing the previous typo.
Uploading new patches for both versions.
Comment #69
catchNot quite.
Comment #70
hefox CreditAttribution: hefox commented(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 :(.)
Comment #71
hefox CreditAttribution: hefox commentedBefore 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.
Comment #72
catchDo 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.
Comment #73
hefox CreditAttribution: hefox commentedYep; 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.
Comment #74
catchHere's a new patch that should deal with both the issues hefox brought up.
Comment #75
catchComment #76
InternetDevels CreditAttribution: InternetDevels commentedAfter 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.
Comment #77
Souvent22 CreditAttribution: Souvent22 commentedSubscribing.
Comment #78
hefox CreditAttribution: hefox commentedSome 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.
Comment #79
catch#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!
Comment #80
catchbdurbin 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.
Comment #81
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedPHP4 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:
I'd like to implement this solution. So, is there any reason to not do it?
Comment #82
catchThe 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.
Comment #83
catchFixes a couple of notices, should not be any functional changes.
Comment #84
catchAnd minor cleanup for that previous patch - just moved the initialization of $this->status to the top of __construct()
Comment #85
hefox CreditAttribution: hefox commentedI 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
Comment #86
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedSorry, but PHP 4 version is producing parse error: code is repeated, including
<?php
, and curly brace is missing for last function.Comment #87
catchhmm looks like a patch applied twice. Try this one.
Comment #88
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedWhat 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.Comment #89
catchCurrently 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.
Comment #90
danmuzyka CreditAttribution: danmuzyka commentedWe 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!
Comment #91
webflo CreditAttribution: webflo commentedLooks good. There is an api function for minimum version.
Replaced 2 by
views_api_minimum_version()
. New patch is attached.Comment #92
webflo CreditAttribution: webflo commentedComment #93
catchTo 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.
Comment #94
dstolThe version numbering has dropped off the patches, is it fair to assume that the last 6.x-2.x patch was #74?
Comment #95
danmuzyka CreditAttribution: danmuzyka commentedThanks catch, I'll go ahead and try doing that to reproduce the issue.
Comment #96
catchAdded 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.
Comment #97
JoeMcGuire CreditAttribution: JoeMcGuire commentedPatch 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.
Comment #98
JoeMcGuire CreditAttribution: JoeMcGuire commentedMissed of a line from the previous patch.
Comment #99
dstolHere is an equivalent of #98 for those of us in the dark ages of 6.x-2.x.
Comment #100
mikeytown2 CreditAttribution: mikeytown2 commented#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:
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.
Comment #101
dstolSeems 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.
Comment #102
thebuckst0p CreditAttribution: thebuckst0p commentedSubscribe. 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.
Comment #103
mikeytown2 CreditAttribution: mikeytown2 commented#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.
Comment #104
thebuckst0p CreditAttribution: thebuckst0p commentedThanks mikeytown2. It's unclear from the thread what version #34 is for - is it for 2.x?
Comment #105
mikeytown2 CreditAttribution: mikeytown2 commentedThat is for 2.x and can be adapted to 3.x if needed.
Comment #106
catchThat'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.
Comment #107
dstolI'm seeing the same problems with #106 that I was in #101. Check out http://skitch.com/unncola/gscx6/views-6testbed
Comment #108
dawehnerI'm not 100% sure but maybe implementing IteratorAggregator would help to solve the ksort() Problem.
Comment #109
mcarbone CreditAttribution: mcarbone commented[removed, posted before thinking]
Comment #110
catchiteratorAggregrator 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.
Comment #111
catchSlight update - previous patch didn't include some fixes I had locally.
Comment #112
dstolGood 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.
Comment #113
dstolI 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.
Comment #114
m2jalali CreditAttribution: m2jalali commentedhi
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
Comment #115
catchhmm, that function definitely exists: http://api.drupal.org/api/views/views.module/function/views_api_minimum_...
Comment #116
dstolUsing #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?
Comment #117
catchfwiw 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).
Comment #118
hefox CreditAttribution: hefox commentedI'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.
Comment #119
catchThat 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.
Comment #120
dstolJust 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.
Comment #121
yched CreditAttribution: yched commentedGot a couple remarks looking at patch #111 - coming late to the party, sorry if those have been addressed before.
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 ?
No static ? We really create a new object on each call ?
Comment #122
catchYes it should.
That's fine I think no-one can or should access the data in the class directly.
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.
Comment #123
fenda CreditAttribution: fenda commentedPerhaps 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.
Comment #124
geerlingguy CreditAttribution: geerlingguy commentedIs 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.
Comment #125
catchI believe Views is using ctools for this in Drupal 7, however I haven't reviewed the equivalent ctools code for some time.
Comment #126
mikeytown2 CreditAttribution: mikeytown2 commentedIn 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.xLooks 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.
Comment #127
mikeytown2 CreditAttribution: mikeytown2 commentedThis 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.
Comment #129
catchFound an issue with the lower-memory version, includes yched's feedback from #121 also (which was related).
Comment #130
Jody LynnThere'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.
Comment #131
cassio CreditAttribution: cassio commentedHey 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.)?
Comment #132
dawehnerNote: 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.
Comment #133
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commentedThe 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