Coming here from #853864: views_get_default_view() - race conditions and memory usage and #1000898: Don't load default view by default in views_get_view(). merlin said:

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.

This is potentially serious from a memory standpoint - on one D6 site (the source for those two issues) loading all the default views was taking around 12mb of memory.

It's a very long time since I looked at the ctools export code, and I'm not on a normal work or Drupal schedule for the next couple of weeks, so can't promise I can look at this, but opening the issue at least, and will review a patch if I'm beaten to it on writing one.

Comments

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

It's important that this is a setting, but other than that, I think it can work more or less as it did in Views, so it should be relatively easy to port the code over to the CTools function that gets default objects.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Active » Fixed

http://drupalcode.org/project/ctools.git/commitdiff/88edd92cb7da955191ec...

Ok, after studying catch's patch for Views 6.x-3.x I've implemented this as an easily enabled setting for all exportables.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Needs work

lock_wait() doesn't need to be put in a loop, it polls internally and bails as soon as possible. Also the polling is a lot cheaper than lock_acquire(). So to lock for five seconds and bail out, it's better to do lock_wait($name, 5); Also #802856: Make lock_wait() wait less attempts to improve the polling.

It looks like the committed version is using a similar approach to one of my earlier patches on the views issue - caching the absence of defaults - http://drupal.org/node/853864#comment-3838808 - Doesn't this mean you'd need to manually rebuild the cache if a default was added to code?

dereine changed this to simply avoid trying to load the default at all in the case where the object is found in the db. That avoids the memory/cpu issue from always triggering a cache miss, but requires #1000898: Don't load default view by default in views_get_view(). It also means a site running with lots of stuff only in the db has no overhead from trying to load from defaults at all.

Doing it that way, there would be less need to worry about whether there were lots of default objects or not when deciding whether to cache (which is hard to predict for some cases), since that wouldn't make a difference for the vast majority of requests.

Marking this CNW since I think this needs more discussion, and if you want the Views patch to match the pattern here, we should try to agree on what the pattern should look like.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

dereine changed this to simply avoid trying to load the default at all in the case where the object is found in the db.

Don't you have to load the default to know if it's an overridden object? I don't see how we can avoid that.

I had put the loop in because if this in the documentation:

This should not be used with locks that are acquired very frequently, since the lock is likely to be acquired again by a different request during the sleep()

But actually reading the code, I see that comment misled me.

It looks like the committed version is using a similar approach to one of my earlier patches on the views issue - caching the absence of defaults - http://drupal.org/node/853864#comment-3838808 - Doesn't this mean you'd need to manually rebuild the cache if a default was added to code?

Just like you need to manually rebuild the cache if a default is changed in the code. Just like you need to flush caches any time something that is cached is changed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

views_block and page callbacks don't need to know if an object only exists in the database or is an override of a default - they just need the view object, same is going to be true for other exportables. On UI/edit/admin pages there is a need to differentiate, so the argument was made optional. This was dereine's idea but makes sense to me (I was going to cache the absense of a default) - and it will save cycles on those front-facing pages. Currently in views, if you have a view in the database only, but lots of defaults for other views, then they all get loaded into memory just to load that one view (because it's 'missing'). This patch also avoids that condition, but when you have an overridden view, the view object is still instantiated twice.

Caching - yes of course, anywhere that needs to always pick this up will need to skip the caching/do a rebuild so that aspect of it doesn't matter.

edited for Sunday night

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Only comparing that data for context needs is really really dangerous. You run the risk of loading it one way because the first context doesn't need to know if it's overridden, but the second context *does* need to know. I really dislike that idea.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I'm not convinced this is dangerous - if you load something without the information that it's over-ridden or not, then the function that needs to know should just request that (which would load the default and does whatever needs to be done - that could also be encapsulated in a helper). The two situations are very different (needing the object, and needing to compare the administrative state of the object). This requires the calling function knowing what it wants to do, but I think that's fine as a requirement. A spot check on the other issue found that /only/ views and features were actually checking that property. These objects still aren't statically cached afaik - always built from scratch when requested, so it shouldn't be an issue getting a different copy at all.

What would help here would be numbers on exactly how expensive loading the two objects is when only one is needed, I noted this mentally when I first came across the issue, but the race condition (missing content on two high profile sites), and loading all defaults on non-cache misses (extra 11mb memory usage) was more pressing when I updated that issue, so didn't look much further and while I think I remember, I don't want to post up incorrect information. Just need to have a default view, profile that, then profile the same view over-ridden. Not sure when I can get to this but will post results when/if I do. Either way I dislike the idea that we do double work every single time something is used, just to avoid a little bit of extra care on admin pages.

Have you looked at #1000898: Don't load default view by default in views_get_view()?

  • merlinofchaos committed 88edd92 on 8.x-2.x 
    Issue #1102252: Implement and document default object caching, taken...

  • merlinofchaos committed 88edd92 on 8.x-3.x 
    Issue #1102252: Implement and document default object caching, taken...
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher at NorthPoint Digital commented
Issue summary: View changes

It is odd that there is a commit on this issue (88edd92) but the issue is still tagged NW. Is that on purpose or an oversight?

I noticed the lines

    // If we're caching, attempt to get a lock. We will wait a short time
    // on the lock, but not too long, because it's better to just rebuild
    // and throw away results than wait too long on a lock.
    if (!empty($export['cache defaults'])) {
      for ($counter = 0; !($lock = lock_acquire('ctools_export:' . $table)) && $counter > 5; $counter++) {
        lock_wait('ctools_export:' . $table, 1);
        ++$counter;
      }
    }

I defer to @catch on whether a loop is useful at all (see #3 above). But the current code looks wrong to me: unless I am confused, if the lock fails initially, then the loop condition !$lock && $counter > 5 will fail the first time through the loop, so lock_wait() is never executed.

Other points:

  • It looks odd to increment the counter twice.
  • Coding standards prefer "ctools_export:$table" to the explicit concatenation.