Currently dmemcache.inc collects data in $_memcache_statistics however this data is only used if the admin module is enabled and a user has the ability to view the data. As a result this means that most of the time this data will simply be ignored but we are still collecting it. I came onto issue when cache_clear a large number of nodes.

Hopefully I will be able to create a patch over the next few days.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Status: Active » Needs review
FileSize
4.52 KB
slashrsm’s picture

Title: Memcache collections stastics data for usage by the admin module unconditional » Memcache collections statistics data for usage by the admin module unconditional
slashrsm’s picture

Status: Needs review » Needs work

This patch seems to save some memory, specially in the case when site executes a lot of cache operations in a single request. Since this module does not have ANY affect to existing users I think it could be committed.

+++ b/dmemcache.incundefined
@@ -35,7 +35,9 @@ $_memcache_statistics = array();
+  if (variable_get('show_memcache_statistics', TRUE) && function_exists('user_access') && user_access('access memcache statistics')) {
+    $_memcache_statistics[] = array('set', $bin, $full_key, '');

I was thinking about this condition. Would it be reasonable to calculate this condition in global scope just once per request? Something like this:

$collect_stats = variable_get('show_memcache_statistics', TRUE) && function_exists('user_access') && user_access('access memcache statistics');

function ...{
  ....
  global $collect_stats;
  if ($collect_stats) {
    ....
  }
}

Why did you check for existence of user_access()?

marcingy’s picture

Because the function may not exist depending on where we are in the bootstrap, note this is the exact same conditional as in the close down function. And global scope feels wrong as that becomes an uncoditional operation on every page load as the cache,inc will be included - now if this was d8 and lazy loaded....I did consider a

if (variable_get('show_memcache_statistics', TRUE) && memache_access()) {}......

function memache_access() {
  static $access = NULL;

  if (!isset($access )) {
    $access = function_exists('user_access') && user_access('access memcache statistics');
  }

return $access;
}

But user_access already has a static cache for each user and there permissions which put me off the idea.

slashrsm’s picture

I get it... in that case this patch disables stats collection entirely until user_access() is not available (condition will be always false because of the conjunction in it). I think we have to collect all stats when user enables that, so we should find some way around it. This also changes current behaviour of the module, which is probably not acceptable.

markpavlitski’s picture

Title: Memcache collections statistics data for usage by the admin module unconditional » Memcache unconditionally collects statistics data for usage by the memcache_admin module
Component: Code » memcache_admin
Status: Needs work » Needs review
FileSize
5.72 KB

The attached patch achieves the following changes:

Firstly page stats are not collected unless the memcache_admin module is enabled.

Secondly collection is disabled as soon as practical (hook_init) if the user doesn't have permission to access them.

Because collection is enabled by a module rather than cache.inc, any cache calls prior to hook_boot are not registered. This is normally limited to get/set of 'cache_bootstrap-variables' and 'cache_bootstrap-bootstrap_modules'.

Kind regards,

Mark

slashrsm’s picture

Looks OK to me. Another option would be to enable in cache.inc instead of hook_boot() and still disable in hook_init(). This way we'd also collect missing gets and keep module's behaviour exactly the same even after this patch gets in.

What do you think about this?

markpavlitski’s picture

I did look at that originally but there's a significant amount of cache activity before hook_init() which gets collected and then thrown away.

I'll post a patch shortly for comparison.

markpavlitski’s picture

I forgot to mention that without hook_boot(), if the memcache_admin module is turned off then all stats get recorded.

slashrsm’s picture

Huh... yes... That's true and definitely not OK, since then we achieve no improvement.

What about this?

Attached patch is based on #1, except that only calculates condition until it realizes that stats should not be collected. After that uses saved value.

slashrsm’s picture

Discussed this with marcingy and he proposed a bit modified solution that should be faster.

markpavlitski’s picture

I like that approach better, though there is a potential issue with the patch.

The `if ($collect)` statement will always succeed when statistics do need to be collected, so the variable_get and user_access tests will be re-run for every cache operation.

Also if the memcache_admin module is disabled (but not uninstalled) the variable_get and user_access checks can still succeed.

The attached patch is a bit longer but should be faster as each condition is tested once as soon as it can be. It also checks that memcache_admin is enabled.

What do you think?

slashrsm’s picture

Looks nice. Just a few comments.

+++ b/dmemcache.incundefined
@@ -458,3 +471,26 @@ function dmemcache_key($key, $bin = 'cache') {
+  // variable_get() is available very early so check it first.
+  if (!isset($collect['variable_get']) && function_exists('variable_get')) {
+    $collect['variable_get'] = variable_get('show_memcache_statistics', TRUE);
+  }

If this gets called before DB bootstraps (and it will) you automatically set this to TRUE. And since you skip this condition after that you basically override this setting.

I think that this will fix it:

$collect['variable_get'] = variable_get('show_memcache_statistics', NULL);
+++ b/dmemcache.incundefined
@@ -458,3 +471,26 @@ function dmemcache_key($key, $bin = 'cache') {
+  if (!isset($collect['variable_get']) && function_exists('variable_get')) {

variable_get() is available almost from the very beginning of request (index.php requires bootstrap.inc where we have this function) so function_exists() check if probably unnecessary here.

markpavlitski’s picture

Thanks for the feedback. I've made a few changes in this patch.

- Removed the function_exists('variable_get') check.
- Split into three static variables for readability since it doesn't use drupal_static() any more.
- Added a check to confirm that variable_initialize() has run before calling variable_get(). I've left the default parameter intact for consistency in case show_memcache_statistics hasn't been set.
- Added a check so that user_access() and module_exists() won't get called if it has already failed the variable_get() test.
- Swapped the order of user_access() and module_exists() since the user_access() check may be unnecessary.
- Added a hook_uninstall() to the module which removes the show_memcache_statistics variable.

slashrsm’s picture

Status: Needs review » Needs work
+++ b/dmemcache.incundefined
@@ -458,3 +470,28 @@ function dmemcache_key($key, $bin = 'cache') {
+  if (!isset($variable_get) && function_exists('module_list')) {
+    $variable_get = variable_get('show_memcache_statistics', TRUE);
+  }

Variable's value can be also set in settings.php. This part of configuration is loaded at very first stage of bootstrap (DRUPAL_BOOTSTRAP_CONFIGURATION), which means it is no need to wait until DRUPAL_BOOTSTRAP_VARIABLES. We could also add README.txt entry that would suggest this approach.

+++ b/dmemcache.incundefined
@@ -458,3 +470,28 @@ function dmemcache_key($key, $bin = 'cache') {
+  elseif (!empty($variable_get) && !isset($module_exists) && !empty($GLOBALS['theme'])) {
+    $module_exists = module_exists('memcache_admin');
+    // If the module doesn't exist then the user can't have access.
+    $user_access = $module_exists && user_access('access memcache statistics');
+  }

I was thinking about "!empty($GLOBALS['theme'])" in the condition. It would probably make more sense to check for "function_exists('user_access')" instead and it should still work OK. What do you think?

+++ b/memcache_admin/memcache_admin.moduleundefined
@@ -106,7 +106,7 @@ function memcache_admin_menu() {
 function memcache_admin_admin_settings() {
   $form['show_memcache_statistics'] = array('#type' => 'checkbox',
     '#title' => t('Show memcache statistics at the bottom of each page'),
-    '#default_value' => variable_get('show_memcache_statistics', 1),
+    '#default_value' => variable_get('show_memcache_statistics', TRUE),
     '#description' => t("These statistics will be visible to users with the 'access memcache statistics' permission."),
   );

#type should go in newline.

I am not sure if we need this configuration at all. Since it is the best to set this in settings.php I'd only document this and propose that approach.

markpavlitski’s picture

Agree that the $conf['show_memcache_statistics'] in settings.php would be nice, but I guess we still need to support existing instances where this is configured on the memcache config page?

I didn't mean for the #default_value to change, so have re-rolled the patch to exclude that. I might add another issue for correcting the #type newline and making the #default_value consistent.

user_access() and module_exists() are both defined too early by _drupal_bootstrap_full().

module_exists() can result in an infinite loop when called during bootstrap, as module_exists() calls module_list() which calls system_list() which calls cache_get().

drupal_get_bootstrap_phase() can break bootstrap and drupal_bootstrap() reports that it has finished bootstrapping while the final phase is occurring - see http://drupal.org/node/1806992 - so can again cause infinite loops.

The global $theme variable is set as the penultimate bootstrap step, just before hook_init(), so seemed like a good thing to test for, but if you can think of an alternative that's a bit cleaner let me know?

slashrsm’s picture

Status: Needs work » Needs review
FileSize
6.69 KB

Agree with you. What about this solution? It will disable stats very early if configured in settings.php or it waits until variables from DB are available.

I think that there is no need to open new issue just to fix a coding standard issue. Since it is kinda related to the thing we're working on here I believe it's OK.

slashrsm’s picture

@marcingy pointed out that we have too complex logic when checking variable. He proposed to use new var, which would be designed to be used in settings.php only. Attached patch does that. Also documented that change in README.txt.

marcingy’s picture

Status: Needs review » Needs work

I have found cases when ESI is thrown into the mix where user_access will produce a notice because we don't have a user object yet it seems like the access check needs to be

$user_access = $module_exists && isset($GLOBALS['user']->uid) && user_access('access memcache statistics');

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
6.45 KB

If we're adding a settings.php variable, how about the following approach which does away the various permission checks.

It also adds a message on the memcache config page to let the user know that stats won't be shown if collection is turned off.

It's a bit faster than doing the module_enabled and user_access checks and won't be affected by the ESI issue marcingy mentioned but will still let users turn off stats in production.

What do you think?

sd46’s picture

Status: Needs review » Needs work

The patch in #20 looks good, but no longer applies properly because of changes to the README.txt

slashrsm’s picture

Status: Needs work » Needs review
FileSize
6.41 KB

Reroll.....

Jeremy’s picture

Not quite ready, I'd change the following:

  1. Don't introduce a new variable, instead tie this to the existing show_memcache_statistics variable
  2. Set the default to FALSE -- so, if memcache_admin isn't enabled there's no overhead from stats

With those changes made, I'm not sure you even need to touch the README.

markpavlitski’s picture

I've re-rolled and simplified the patch from #16, which already takes this approach, and I've reversed the default value for show_memcache_statistics as per your recommendation.

Because the variable is set through the UI, we need to be careful when checking it that DRUPAL_BOOTSTRAP_VARIABLES has been reached otherwise some stats will be lost.

slashrsm’s picture

Status: Needs review » Needs work

I think we need to leave it enabled by default. There are two reasons for that:
1. This is current behaviour of this module and we should not change this within a major version.
2. this patch defaults to FALSE, while memcache_admin defaults to TRUE. This will produce a mess if variable never actually get's set (not in db or set in settings.php), as we'll use different default value on two places within the same module.

That also implicates that we need to provide info about that in README.txt.

Otherwise looks OK.

markpavlitski’s picture

1. True, this will have an impact on existing users who have enabled and use the memcache_admin module, but have never saved the memcache_admin config page.
This could be mentioned in the release notes of the next stable release. @Jeremy, what do you think?

2. The patch in #24 changes the default value in both modules, so remains consistent.

slashrsm’s picture

Changing default value of a variable is not acceptable within a stable major release/branch IMO. I would call this a backward compatibility breakage.

Jeremy’s picture

Status: Needs work » Needs review

I disagree; this is a performance and memory improvement to the code. We're removing unnecessary overhead on most page loads, except when it's explicitly desired. We'll document this change in the change notes for the next release, and it doesn't hurt to document it in the README too. This is not an API, we're not breaking anything, we're optimizing.

I'm offline for a couple of weeks; I'm hoping this will get some additional testing in that time. In particular, make sure we're not losing any statistics in the bootstrap.

markpavlitski’s picture

Status: Needs review » Postponed
FileSize
14.16 KB

I've done some testing against this patch on an existing site using XHProf, results attached.

The stats recorded are identical, however it looks as if the performance gains are questionable at best for normal site usage.

Given that the memory allocated to $_memcache_statistics is only in the region of 10's of KiB, I'd suggest we postpone this issue for now.

marcingy’s picture

Status: Postponed » Needs review

How many items was this with? There are cases where doing a lot manipulations that collecting stats can bring sites down. This is where the patch original came out of - note this is a site with millions of nodes and taxonomy terms where bulk updates can touch many 10,000s of nodes in terms of cache clears.

Rok Žlender’s picture

Collecting stats can blow through multiple hundred MB of memory when clearing a lot of nodes. Even small gains for a module that is used on basically every high volume Drupal site is worth it imo.

markpavlitski’s picture

This was on a site with about 1.5m nodes, flushing all caches then performing a solr (search api) search.

I've not seen stats size go above 100kib during normal usage, though I haven't traced heavy cron runs etc.

I'll do some more testing based on your feedback.

markpavlitski’s picture

@Rok Žlender note that in some cases the patch causes a higher memory footprint and has a higher function call count.

marcingy’s picture

Flushing an entire cache bin will not cause any issues with memory as that logs a single stat in memcache, but doing large numbers of targeted clears will because the operation multiplies by N. We are also using the entity cache and performance hacks module, so to clear a node a cache clear gets issued for entity itself, and then for each view mode, theme and user combination.

Prior to creating this patch the solution we had was too in certain larger scripts to reset the global after N nodes had been updated in the calling script. So we have real world experience that collecting stats can and will bring a site down.

markpavlitski’s picture

@marcingy thanks, I'll put some test scripts together to confirm that #24 solves that use case.

markpavlitski’s picture

FileSize
1.85 KB

A few stats generated with the attached script:

Without the patch:

Initial variable size: 0.00MB
Initial memory usage: 12.68MB

Variable size after 25000 sets: 4.63MB
Memory increase after 25000 sets: 28.87MB

Variable size after 25000 gets: 9.20MB
Memory increase after 25000 gets: 28.85MB

Variable size after 25000 clears: 13.91MB
Memory increase after 25000 clears: 29.11MB

Total memory increase: 86.84MB

With patch applied (without permission to view stats):

Initial variable size: 0.00MB
Initial memory usage: 12.68MB

Variable size after 25000 sets: 0.00MB
Memory increase after 25000 sets: 0.02MB

Variable size after 25000 gets: 0.00MB
Memory increase after 25000 gets: 0.00MB

Variable size after 25000 clears: 0.00MB
Memory increase after 25000 clears: 0.01MB

Total memory increase: 0.03MB
markpavlitski’s picture

This patch adds tests to #24 to check that early bootstrap variables are still recorded and to check that statistics are not collected when they are turned off or the user doesn't have access.

It also includes a change to the debug_backtrace() call in memcache.inc:164 which will reduce the memory footprint of the call in all php versions, and increase function speed in php5.4.

SocialNicheGuru’s picture

after installing I ran the following command

drush cc --debug

I got the following error numerous times:
debug_backtrace() expects at most 1 parameter, 2 given memcache.inc:164

markpavlitski’s picture

@SocialNicheGuru Please can you confirm which error level was produced? e.g. E_WARNING/E_NOTICE. Can you also confirm which version of PHP you are using?

markpavlitski’s picture

This is due to a difference in the way debug_backtrace() expects to be called prior to PHP 5.4.

This patch suppresses the warning, but is otherwise the same as #37.

@SocialNicheGuru Can you confirm if you still receive the warning using this patch?

SocialNicheGuru’s picture

it fixed it

Rok Žlender’s picture

+++ b/memcache.incundefined
@@ -161,7 +161,7 @@ class MemCacheDrupal implements DrupalCacheInterface {
+    $backtrace = @debug_backtrace(FALSE, 3);

I don't think changing this call should be part of this issue if at all. Drupal 7 requires 5.2.5 and recommends 5.3. I don't think memcache should use 5.4 changes.

Rok Žlender’s picture

Status: Needs review » Needs work
markpavlitski’s picture

Ok I'll remove the change and open another issue. Note that although it provides a performance enhancement for php 5.4, this change is backward compatible with earlier versions too. Php 5.2 will still work as-is.

Rok Žlender’s picture

Yes it will work however there is a performance decrease anytime PHP throws a notice even if suppressed.

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
11.74 KB

True. This patch does not include the debug_backtrace() change.

marcingy’s picture

Some variable renaming in collection function and also some tweaks to the access checking logic to ensure that we do not check user access on every memache call.

markpavlitski’s picture

@marcingy I prefer your variable naming, and good spot on the repeated user_access() checks.

This version just replaces the === NULL test with isset() and moves it before !empty().

Jeremy’s picture

Status: Needs review » Fixed

Thanks for all the effort on this! I made some tweaks to dmemcache_collect_stats -- I improved the documentation, modified the logic slightly, and fixed a typo in the $user_access_checked variable. Committed!
http://drupalcode.org/project/memcache.git/commitdiff/cf758f7

Statistics are now disabled by default -- and when disabled they no longer have overhead.

markpavlitski’s picture

@Jeremy Great to see this finally get in!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

DamienMcKenna’s picture

Status: Closed (fixed) » Patch (to be ported)

Seems like this would be worth backporting.

DamienMcKenna’s picture

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

Correct branch.

davidwhthomas’s picture

A note, Got this error testing 7.x-1.1-beta4

Error: Call to undefined function dmemcache_collect_stats() in .../memcache/dmemcache.inc, line 240

It appears that function isn't actually defined anywhere.

Looks like it just need to be removed, as the stats collection is later.

I went to make a patch but looks like it's been patched already in the 1.x-dev branch.

Jeremy’s picture

  • Jeremy committed b560d76 on 6.x-1.x
    Issue #1733674, only collect statistics if enabled; improved stats
    
Jeremy’s picture

Status: Patch (to be ported) » Fixed

Backported along with other relevant cleanup:
http://cgit.drupalcode.org/memcache/commit/?id=b560d76

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.