Goal

  • Leverage APC when it is available.

Use-cases

  • Symfony\Component\ClassLoader\ApcUniversalClassLoader

  • Drupal\Core\Cache\ApcBackend

    • Allow to use it for assorted cache bins.
  • APC user variables? We surely must have use-cases in core that could leverage those?

Related issues/projects

CommentFileSizeAuthor
#123 1807662-122.patch13.36 KBWim Leers
#118 interdiff.txt1.57 KBWim Leers
#118 1807662-116.patch14.24 KBWim Leers
#114 1807662-114.patch13.31 KBAnonymous (not verified)
#109 1807662-109.patch13.51 KBAnonymous (not verified)
#109 interdiff.tagssss.txt2.16 KBAnonymous (not verified)
#104 interdiff.apcu-cleanup.txt2.16 KBAnonymous (not verified)
#104 1807662-104.patch13.28 KBAnonymous (not verified)
#102 interdiff.invalidate.txt942 bytesAnonymous (not verified)
#102 1807662-102.patch13.76 KBAnonymous (not verified)
#96 interdiff-apc-backend.txt6.46 KBAnonymous (not verified)
#96 1807662-95.patch13.76 KBAnonymous (not verified)
#89 drupal-apcu-1807662-02a5887-89.patch14.03 KBMixologic
#79 interdiff-1807662-78.txt1.68 KBdamiankloip
#79 1807662-78.patch13.28 KBdamiankloip
#74 interdiff.txt6.58 KBWim Leers
#74 apc_cache-1807662-74.patch13.3 KBWim Leers
#73 interdiff.txt6.58 KBWim Leers
#73 apc_cache-1807662-73.patch17.12 KBWim Leers
#67 interdiff.txt6.61 KBsun
#67 apc.cache_.67.patch11.02 KBsun
#66 1807662-66.patch10.76 KBAnonymous (not verified)
#56 1807662-56.patch12.93 KBdamiankloip
#49 1807662-49.patch12.58 KBdamiankloip
#47 1807662-47.patch12.57 KBdamiankloip
#47 interdiff-1807662-47.txt677 bytesdamiankloip
#45 1807662-44.patch12.35 KBdamiankloip
#45 interdiff-1807662-44.txt1.3 KBdamiankloip
#43 1807662-43.patch12.2 KBdamiankloip
#43 interdiff-1807662-43.txt9.82 KBdamiankloip
#34 platform.apc_.34.patch17.43 KBsun
#34 interdiff.txt6.67 KBsun
#31 platform.apc_.31.patch15.55 KBsun
#31 interdiff.txt5.38 KBsun
#26 platform.apc_.26.patch14.8 KBsun
#26 interdiff.txt3.79 KBsun
#25 platform.apc_.24.patch14.8 KBsun
#25 interdiff.txt8.66 KBsun
#16 platform.apc_.16.patch14.51 KBsun
#16 interdiff.txt3.73 KBsun
#13 platform.apc_.13.patch11.85 KBsun
#13 interdiff.txt3.5 KBsun
#12 platform.apc_.12.patch14.22 KBsun
#12 interdiff.txt3.24 KBsun
#10 platform.apc_.10.patch14.22 KBsun
#7 platform.apc_.3.patch28.44 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

msonnabaum’s picture

Not sure I care if we have cache backends for it, but I do like the idea of using apc when it's available. For example, this patch is quite helpful when your files directory is on a distributed file system:

https://github.com/pressflow/7/pull/16/files

cweagans’s picture

I would argue that we should automatically use ApcUniversalClassloader if APC is available. Don't make me edit a configuration variable when APC is clearly the better option. I would also argue that we should automatically use ApcBackend when APC is available and that both of those options should be able to be overridden in settings.php.

Crell’s picture

Except that APC class loader would need a cache flush every time you touch a file or class name in dev. Ideally if we got a dev/prod toggle enabled we could auto-select the APC class loader if available in prod mode only. Related: #1537198: Add a Production/Development Toggle To Core

msonnabaum’s picture

@cweagans - Using APC as a cache backend by default is insane. No way we're doing that.

Crell is right about the autoloader. It's something you have to be aware of, so it shouldnt be the default.

andypost’s picture

Why APC? xcache2 more powerful
How it could work on shared hosts with different config options?

podarok’s picture

agree with #5
we should not integrate one op-code cacher feature - as framework has to be compatible and able to connect to any of them via common api layer

sun’s picture

Title: [meta] Built-in APC support in core » Built-in APC support in core
Assigned: Unassigned » sun
Status: Active » Needs review
FileSize
28.44 KB

Added Cache\ApcBackend from APC module.
Cleaned up apc stub functions.
Removed APC statistics support.
Fixed phpDoc, relocated methods.
Added visibility to methods.
Fixed missing method visibility in Cache classes.
Other Cache classes clean-ups.
Basic upgrade of getMultiple() and set() methods.
Added ApcBackendUnitTest.
Fixed bogus @file declaration.
Fixed bogus phpDoc in DatabaseBackend.
Upgraded ApcBackend for cache tags.
ApcBackendUnitTest passes, but UnitTestBase is broken; added lots of todos and debugging code. :(

We should probably back out the unrelated fixes into a separate issue.

Status: Needs review » Needs work

The last submitted patch, platform.apc_.3.patch, failed testing.

sun’s picture

sun’s picture

FileSize
14.22 KB

Remove unrelated changes, and Merged in HEAD.

Status: Needs review » Needs work

The last submitted patch, platform.apc_.10.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
14.22 KB

Removed Simpletest debugging code. To be fixed over in #1436684: Remove static cache in drupal_valid_test_ua()

sun’s picture

FileSize
3.5 KB
11.85 KB

Sorry, that was the wrong patch. :-/

Status: Needs review » Needs work

The last submitted patch, platform.apc_.13.patch, failed testing.

brianV’s picture

We would need to be cautious about how this is done in order to not be detrimental in shared hosting situations where the default apc.shm_size (if enabled at all) may not be sufficient. Excessive cache empties and reloads don't help anyone.

I would argue that we don't want full caching in APC by default; perhaps limit it to Symfony\Component\ClassLoader\ApcUniversalClassLoader by default so as to not eat up too much of the segment size while still providing a good performance boost.

Then make the cache backend for cache bins optional, for the user to configure as they see fit (and have apc segment memory available.

On a more philosophical note - why should the cache backend be in core? Anyone savvy enough to configure APC (admittedly simple) and configure the bins in settings.php is savvy enough to download it from contrib. What do we gain by providing the backend in core? And why just APC and not Xcode2, memcache, eaccellerator etc. etc.?

sun’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
14.51 KB

APC is maintained by the PHP core team. It is very actively maintained. It is available for all platforms and often comes pre-bundled/pre-installed even. Most of Drupal's competitors have built-in APC support since ages. Enough said.

Fixed unit tests are not able to detect PHP CLI, since run-tests.sh masquerades as Apache.
Fixed run-tests.sh bogusly detects unmet requirements as fatal error.

cweagans’s picture

Status: Needs review » Needs work

This probably needs a comment in default.settings.php describing how to configure different cache bins to use APC with some examples (like the database settings). If the documentation isn't included in default.settings.php, then at least a URL for the documentation page on drupal.org should be added.

Crell’s picture

+++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
@@ -0,0 +1,379 @@
+    // First we determine the prefix from a setting.
+    $prefix = self::getPrefixSettingForBin($this->bin);

Just to be on the safe side, this should probably be static::, not self::, so that we get late static binding. There's very few reasons to still use self:: in PHP 5.3.

+++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
@@ -0,0 +1,379 @@
+    // When we are in testing mode we add the test prefix.
+    // @todo drupal_valid_test_ua() should *always* contain the correct prefix,
+    //   but does not due to Simpletest bugs.
+    // @see http://drupal.org/node/1436684
+    if ($test_prefix = drupal_valid_test_ua()) {
+      $prefix = $test_prefix . '::' . $prefix;
+    }
+    elseif (isset($GLOBALS['drupal_test_info'])) {
+      $prefix = $GLOBALS['drupal_test_info']['test_run_id'] . '::' . $prefix;
+    }

Can't we externalize this logic? Two global calls here make me cringe.

+++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
@@ -0,0 +1,379 @@
+  protected static function getPrefixSettingForBin($bin) {
+    $prefixes = variable_get('cache_prefix', '');
+
+    if (is_string($prefixes)) {
+      // Variable can be a string, which then considered as a default behavior.
+      return $prefixes;
+    }
+
+    if (isset($prefixes[$bin])) {
+      if (FALSE !== $prefixes[$bin]) {
+        // If entry is set and not FALSE, an explicit prefix is set for the bin.
+        return $prefixes[$bin];
+      }
+      else {
+        // If we have an explicit false, it means no prefix whatever is the
+        // default configuration.
+        return '';
+      }
+    }
+    else {
+      // Key is not set, we can safely rely on default behavior.
+      if (isset($prefixes['default']) && FALSE !== $prefixes['default']) {
+        return $prefixes['default'];
+      }
+      else {
+        // When default is not set or an explicit FALSE, this means no prefix.
+        return '';
+      }
+    }
+  }

Again, can't the prefix be externalized? It's basically the same thing as passing in a DB connection to the database-backed cache driver.

+++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
@@ -0,0 +1,379 @@
+  protected function binKey() {
+    return $this->prefix . $this->bin . '::';
+  }

I hereby decree this method shall be pronounced "Binky". :-)

+++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
@@ -0,0 +1,379 @@
+  public function deleteMultiple(array $cids) {
+    apc_delete(array_map(array($this, 'key'), $cids));
+  }
+
+  /**
+   * Implements Drupal\Core\Cache\CacheBackendInterface::flush().
+   */
+  public function flush() {
+    $iterator = $this->getAll();
+    foreach ($iterator as $key => $data) {
+      apc_delete($key);
+    }
+  }

I'm curious why you're doing the fancy functional approach for deleteMultiple() but not for flush()? It's not a blocker I guess, I'm just curious why you wouldn't do both the same way.

+++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
@@ -0,0 +1,379 @@
+// PHP CLI uses a different php.ini in which APC may not be enabled.
+// Create stub functions to prevent fatal errors.
+if (drupal_is_cli() && (!extension_loaded('apc') || !ini_get('apc.enable_cli'))) {
+  function apc_fetch($key, &$success) {
+    $success = TRUE;
+    return FALSE;
+  }
+  function apc_store($key, $var) {
+    return FALSE;
+  }
+  function apc_delete($key) {
+    return FALSE;
+  }
+  function apc_inc($key, $step, &$success) {
+    $success = TRUE;
+    return FALSE;
+  }
+}

Why is this code outside of the class? That's a no-no.

brianV’s picture

Most of Drupal's competitors have built-in APC support since ages. Enough said.

Most of Drupal's competitors are inferior to Drupal. That's hardly a justification for extra core bloat.

Don't get me wrong - I like APC a lot. I use it on all my projects. I don't, however, see any good justification for adding more core bloat... Given that the cache backend is intended to be pluggable, I ask again - why are we putting this backend in core, and not contrib? Alternatively, what justification allows this one in core, but not other stable caching backends?

alexrayu’s picture

Don't bloat the core up people!

cweagans’s picture

I don't, however, see any good justification for adding more core bloat.

First, you need to define bloat. Disk space is cheap and I could care less about the few extra kilobytes that this would add. This code won't get loaded if it's not used, so if you don't want it, don't turn it on.

Second, having this available in core (especially if/when the config option is added to default.settings.php) ensures that people actually know about it. One of the biggest complaints that I hear about Drupal is that it's slow. I ask them "Well, have you turned on all the caching options built into core? Memcache? Boost? APC?" They usually answer, "Well, no...I haven't even seen that stuff."

Third, APC is the most common caching mechanism available, so this makes more sense for core than, say, the memcache backend.

Personally, I think it would be great if Drupal could cache things somewhere other than the database without having to download another contrib module. A module being enabled seems like more resource usage on every page request than simply setting a flag in settings.php

brianV’s picture

First, you need to define bloat. Disk space is cheap and I could care less about the few extra kilobytes that this would add. This code won't get loaded if it's not used, so if you don't want it, don't turn it on.

Probably a good idea. I would consider this bloat because it's code that is in core, and bound by core policies, that doesn't need to be. It places a burden on the core devs and those who help maintain core...

Second, having this available in core (especially if/when the config option is added to default.settings.php) ensures that people actually know about it. One of the biggest complaints that I hear about Drupal is that it's slow. I ask them "Well, have you turned on all the caching options built into core? Memcache? Boost? APC?" They usually answer, "Well, no...I haven't even seen that stuff."

There is definitely a problem here, but I see the solution in better documentation and training, personally.

Anyways, I will admit, we are off into the weeds from my initial main concern - I suggest we don't enable the APC cache bins by default, apart from perhaps class loading. Lots of shared hosting has 32M (default) or smaller apc.shm_size, and it's not hard to overrun that especially if the user runs several sites on the same account. The problem I see is that when APC is full, it flushes itself *completely*.

So if you end up filling the cache every few requests, the users could end up with a cold cache as often as not. It might actually be detrimental to performance in these situations.

sun’s picture

@Crell:
Yeah, most of that prefix/bin/key code originates from the APC module and I didn't really touch it yet, since the logic going on there is quite advanced and debugging showed that some parts of it are indeed necessary.

The static stuff is definitely not required though - I'll convert that into a simple class property that's set up only once in __construct().

Regarding "externalizing" (I guess you mean injecting?) the prefix, I guess that's what #1764474: Make Cache interface and backends use the DIC is about?

The binKey() method should go away entirely IMHO - it just prefixes the $cid with a string - I don't see why we'd need a function call for that.

The key() method should be renamed to addPrefix(), I think.

The difference between deleteMultiple($cids) and flush() is that the former gets a known list of $cids passed, so those keys can be simply prefixed. flush(), however, needs to find all cache items in the store that belong to the prefix.

Lastly, the stub functions are a very interesting detail in this. (Apparently, a stub for ApcIterator is missing.) As the comment clarifies, APC might be turned off for PHP CLI but may be set as cache class via $conf['cache_classes'] in settings.php, in which case stuff like Drush and other CLI scripts would attempt to use it and break with fatal errors. Do you have any ideas for how we could resolve that in a better way?

cweagans’s picture

Anyways, I will admit, we are off into the weeds from my initial main concern - I suggest we don't enable the APC cache bins by default, apart from perhaps class loading. Lots of shared hosting has 32M (default) or smaller apc.shm_size, and it's not hard to overrun that especially if the user runs several sites on the same account. The problem I see is that when APC is full, it flushes itself *completely*.

Read the patch. This is not enabled by default. You have to put stuff in settings.php to turn it on. Also? Shared hosting typically does not have APC enabled.

sun’s picture

Status: Needs work » Needs review
FileSize
8.66 KB
14.8 KB

Cleaned up APC prefix determination, variables, and methods.

sun’s picture

FileSize
3.79 KB
14.8 KB

Renamed all 'Apc' to 'APC', since it's an acronym and PHP itself uses 'APC', so 'Apc' would be inconsistent.

sun’s picture

andypost’s picture

APC is a trademark :)

Status: Needs review » Needs work

The last submitted patch, platform.apc_.26.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/Cache/APCBackend.php
@@ -0,0 +1,384 @@
+    $prefix = self::getPrefix($this->bin);

static:: please. :-)

+++ b/core/lib/Drupal/Core/Cache/APCBackend.php
@@ -0,0 +1,384 @@
+// PHP CLI uses a different php.ini in which APC may not be enabled.
+// Create stub functions to prevent fatal errors.
+if (drupal_is_cli() && (!extension_loaded('apc') || !ini_get('apc.enable_cli'))) {
+  function apc_fetch($key, &$success) {
+    $success = TRUE;
+    return FALSE;
+  }
+  function apc_store($key, $var) {
+    return FALSE;
+  }
+  function apc_delete($key) {
+    return FALSE;
+  }
+  function apc_inc($key, $step, &$success) {
+    $success = TRUE;
+    return FALSE;
+  }
+}

Hm. OK, I see what is going on here, now. I'm not a fan of the approach, but I don't have a better idea yet. At minimum, this sort of hackery should go in common.inc or something rather than in a class file.

And yes, by "externalize" I mean inject, vis, move that logic external to this class. If we can't do that yet due to another issue, let's drop a @todo on it with an issue link so we know to fix it later.

sun’s picture

Status: Needs work » Needs review
FileSize
5.38 KB
15.55 KB

I'm not a fan of the approach, but I don't have a better idea yet. At minimum, this sort of hackery should go in common.inc or something rather than in a class file.

Me too. This code also originates from APC module. (It's really helpful to move something into core that was battle-tested already.)

The downside of moving this code elsewhere, however, is that we'd non-trivial code for checking whether the stubs are/will be needed. I'm perfectly aware that this completely violates PSR-*, but keeping the stubs in there seems to be the most simple and most elegant way to solve the problem, because the code is only loaded when actually needed and doesn't introduce complexity nor code debt elsewhere.

...but apparently, an APCIterator stub is still missing, and trying to add that in the same way yields:

Fatal error: Cannot declare class Drupal\Core\Cache\APCIterator because the name is already in use in Drupal\Core\Cache\APCBackend

So here's a different proposal that hopefully works better:

1) Check availability in the constructor, set a Boolean property, and implant a condition into the public cache API methods. If FALSE, do nothing.

2) Optional, later: If FALSE, turn the APCBackend silently into a MemoryBackend (i.e., built-in), so as to prevent data from being rebuilt multiple times on CLI.

Replaced self:: with static::.
Replaced stub functions with APCBackend::$available property.

Status: Needs review » Needs work

The last submitted patch, platform.apc_.31.patch, failed testing.

Crell’s picture

Ah, I like that a lot better. Agreed that a memory backend fallback would be even better. Could we do that by just making this class subclass off of the memory backend, and then if (!static::$available) { return parent::whatever(); } ?

sun’s picture

Status: Needs work » Needs review
FileSize
6.67 KB
17.43 KB

Added fallback to MemoryBackend if APC is not available.
Added test for APCBackend fallback to MemoryBackend. (YAY!)

Status: Needs review » Needs work

The last submitted patch, platform.apc_.34.patch, failed testing.

pounard’s picture

  • What! Please don't merge the kind of isAvailable() checks in the backend itself, it taints it.
  • Re WHAT! There is no way the ApcBackend actually extends the MemoryBackend, both are very different for different needs: this is a very ugly way of fixing the CLI problem.
  • The APC module does it in a more elegant manner I think: it creates stub implementations when apc_*() functions are missing, which makes it being a null implementation without tainting it with runtime availability checks
  • Agree with Crell, the getPrefix() helper must get out of this specific backend class, it's something than a lot of contrib backends will need, and this should be in a utility class instead or live as a DIC parameter instead to avoid global state
  • All those static:: calls everywhere in all the methods are actually very frightening (and quite ugly)
  • The getAPCKey() and APCBackend sounds like we're yelling I'd personally follow SF for naming and switch to getApcKey() and ApcBackend instead.
  • You forgot the most important point: if I have a multiple frontend installation (multiple boxes with their own webserver for each, but the same site) you can't flush your caches on all frontends at once. I contributed to the APC module, and a colleague of mine and a few other people are actually trying to solve this problem: we cannot seriously support this Apc backend in core without first:
    1. Get in touch with them
    2. Find a generic solution for this problem; because some other backends will need it
  • This patch is clearly NOT YAY at all IMO.
Damien Tournoud’s picture

You forgot the most important point: if I have a multiple frontend installation (multiple boxes with their own webserver for each, but the same site) you can't flush your caches on all frontends at once.

You don't even need multiple frontend. The same problem occurs between the command line and the web server.

sun’s picture

@pounard:

1) You obviously did not read any issue comments, nor earlier patches, so there's no point in responding to your available/MemoryBackend fallback criticism.

2) Regardless of being injected or not, an externally callable getPrefix() method will be required to figure out the prefix. Even more so, when the prefix needs to be injected. Moving it into a completely separate function outside of the class would only make sense if the identical logic can be applied for other k/v cache backends, which I'm entirely not sure about.

3) PHP uses APCIterator, this is the APCBackend. There's no Symfony involved here. Consistency is thus to be achieved with PHP. In any case, if there's disagreement on that detail » separate issue, thanks.

4) Challenges involving multiple web heads are heavily advanced and can be happily follow-ups to this patch, if needed. Much like the challenge of a possible PHP extension on CLI that's required for the backend, these are actually the secondary major reasons for why I want APC support built-in in core. Alternative implementations for other backends (but also APC) can exist in contrib and learn from the best practices we're figuring out in core.

pounard’s picture

You obviously did not read any issue comments

Not all that's true. But I did read some of them. Regarding "...but apparently, an APCIterator stub is still missing, and trying to add that in the same way yields" the contrib backend actually don't suffer from it, and in the continuity of the legacy method, we can stub quite easily: class APCIterator extends EmptyIterator {} and it will work seemlessly.

identical logic can be applied for other k/v cache backends

Memcache and Redis are two examples that jumped out of my head in less than 3 seconds after reading that [EDIT: actually Redis was already in my mind since it's a topic in which I'm trying to get uniformisation accross contrib backends because core don't provide anything for it].

PHP uses APCIterator, this is the APCBackend. There's no Symfony involved here. Consistency is thus to be achieved with PHP

We are using Symfony too, both two are valid proposals, I'm just proposing the other one.

can exist in contrib and learn from the best practices we're figuring out in core.

Calm down and respect multiple years of work and many issues solved by many users out there on the existing backend, and don't try to impose what are the "best practices", especially when you cut/pasted lots of code from them.

Challenges involving multiple web heads are heavily advanced and can be happily follow-ups to this patch

Providing an non working implementation as soon as you try to scale, without any warning about incomplete status of the backend is foolish. I don't want non Drupalistas to see that after running their site into prod, after they actually broke it because of it, just because it's "better to do in a follow up" which has 50% chances of never being finished.

In the end, my opinion about this issue is that it is the right one that triggers all those problems and that needs to fix it. The same way that each new use case of the K/V store leads to a K/V store patch in the specific issue, the new problems we discover here must be taken care of right when we found it. Especially when it's a series of long term problems that a lot of people actually are experiencing!

For the record, here are some issues where those problems are trying to be solved:

sun’s picture

Regarding CLI vs. webserver (or any other SAPI interface):
A common approach to that seems to be to invoke scripts that require to access the same memory through the desired SAPI; e.g.:

file_get_contents('http://example.com/myscript.php');

There are probably other possible solutions. In any case, I don't really see why that should block us from adding APC support? And why that cannot be discussed in a separate follow-up issue? The problem surely applies to pretty much all alternate caches around, so discussing it on the sole ground of APC would be incomplete anyway.

Damien Tournoud’s picture

There are probably other possible solutions. In any case, I don't really see why that should block us from adding APC support? And why that cannot be discussed in a separate follow-up issue? The problem surely applies to pretty much all alternate caches around, so discussing it on the sole ground of APC would be incomplete anyway.

It really only applies to APC (and similar shared memory cache schemes), which is cannot be accessed from outside the web server SAPI. Memcache, Redis, etc. are both cluster-wide and accessible via the CLI.

pounard’s picture

We cross-posted, I edited my post upper.

We can use the state API to store the latest cache clear timestamp and store its equivalent at flush() call in a key in the APC side. When the object is being built at __construct() time we can compare them and run the flush() if the state() version is higher than the APC local version. This is the most simple solution, and this is probably just something like 10 to 20 lines to implement, so let's do it once for all.

The state() key can actually be set either on a per bin basis, either globally, and can be used by other backends. We can even add a LocalCacheBackendInterface which only adds the getter and setter for the local value, which then leads us to be able to either:
* put the backend behing a generic proxy implementation that does the check
* use the factory/manager to do it for us and leave the backend flush itself at __construct() time.

There is also numerous other smart leads proposed in the given links upper.

damiankloip’s picture

Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
9.82 KB
12.2 KB

Okay, Let's try to get this issue moving again! I think I agree that the APCBackend should not care about implementing the fallback logic itself. It should be a dumb cache implementation - this should be handled outside of the class.

I have rerolled the patch to remove all this logic from APCBackend, and also update it to the current CacheBackendInterface, as a few things have changed since the last patch. So atm I have just removed the prefix logic, as I think we need somewhere else for it to live.

Status: Needs review » Needs work

The last submitted patch, 1807662-43.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
12.35 KB

Forgot about $allow_invalid

Status: Needs review » Needs work

The last submitted patch, 1807662-44.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
677 bytes
12.57 KB

Sorry, also needs a deleteTags() method.

Status: Needs review » Needs work

The last submitted patch, 1807662-47.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
12.58 KB

Status: Needs review » Needs work

The last submitted patch, 1807662-49.patch, failed testing.

catch’s picture

APC is less likely to be available now that opcache is in PHP 5.5.

There's http://pecl.php.net/package/APCu which provides the user cache without the opcode caching but not sure how well that's going to be supported.

pounard’s picture

Indeed. If D8 remains PHP 5.3 compatible, a lot of hostings will still ship APC (think of not-so-bleeding edge Linux distros for example) so I guess having this in core for 8 is still a good idea, while it will probably be removed for later versions. The amount of code needed for this remains minimal and encapsulated, and will be quite easy to maintain for 8.

Pancho’s picture

I agree with pounard on that it's worth it and the good maintainability, plus I don't even expect D9 to rightaway require PHP 5.5, so it would stay in core until D10.

damiankloip’s picture

I agree with the above, I think it's a nice little win, and it wont be a big overhead to maintain. I will work on this and get it passing.

cweagans’s picture

damiankloip’s picture

Status: Needs work » Needs review
FileSize
12.93 KB

This will get a bit closer.

andypost’s picture

+++ b/core/lib/Drupal/Core/Cache/APCBackend.phpundefined
@@ -0,0 +1,359 @@
+    apc_store($this->getAPCKey($cid), $cache, $expire);
...
+      apc_inc($this->tagsPrefix . $tag, 1, $success);
+      if (!$success) {
+        apc_store($this->tagsPrefix . $tag, 1);

so result of apc_store() does not matter?

The last submitted patch, 1807662-56.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Title: Built-in APC support in core » Built-in APCu support in core (PHP 5.5 only)
Issue summary: View changes

Given that PHP 5.5 has a built-in opcache now, and any D8 site that seriously cares for performance is going to run on PHP 5.5, I'd suggest to limit the scope of this issue to just a built-in Drupal\Core\Cache\ApcuBackend

https://gist.github.com/msonnabaum/9836899 is a quick & dirty hack based on MemoryBackend, but might help

Anonymous’s picture

i'll roll the work over in the inconsistent backend issue in to this one today.

catch’s picture

Category: Feature request » Task
Anonymous’s picture

Anonymous’s picture

damiankloip’s picture

Re #59, you mean limit the scope (keeping it similar to patches so far) and not include any logic to decide if/when this bin gets used or not?

catch’s picture

Anonymous’s picture

FileSize
10.76 KB

ok, here goes. this is pretty much just the code at #56, with the following changes:

* APCBackend renamed to ApcuBackend

* ApcuBackend->prefix renamed to ApcuBackend->sitePrefix

* ApcuBackend->deleteAll() now uses an APCIterator directly

  /**
   * {@inheritdoc}
   */
  public function deleteAll() {
    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->binPrefix . $prefix, '/') . '/'));
  }
sun’s picture

Status: Needs work » Needs review
FileSize
11.02 KB
6.61 KB

Looks good to me. First wanted to post a review on many minor things, but then figured it's faster if I simply fix them myself.

Various clean-ups + Fixed bogus $prefix variable in deleteAll().

Status: Needs review » Needs work

The last submitted patch, 67: apc.cache_.67.patch, failed testing.

sun’s picture

67: apc.cache_.67.patch queued for re-testing.

The last submitted patch, 67: apc.cache_.67.patch, failed testing.

catch’s picture

apc.enable_cli must be enabled to run this test.

Opened #2250211: Enable apc.enable_cli for core APC cache backend tests.

Anonymous’s picture

assuming we get the bots to the point this can pass, i like changes in #67 - looks pretty ready to me.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Fast By Default, +sprint
FileSize
17.12 KB
6.58 KB

In this reroll:

  1. Introduced a ApcuBackendFactory (with corresponding cache.backend.apcu service), that uses conf_path() as the default site prefix.
  2. Implement the recently added ::setMultiple().
  3. CacheBackendInterface::CACHE_PERMANENT no longer equals zero, which the current code assumes; fixed.
  4. ::removeBin() deleted *everything* in APCu, rather than only its own entries; fixed.
  5. Fixed a lot of nitpicks.

(To test, just change $service_name = 'cache.backend.database'; to $service_name = 'cache.backend.apcu'; in CacheFactory — on a fresh D8 install with no content, the number of DB queries should drop from 150 to 60 with warm caches.)


+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -0,0 +1,362 @@
+    $cids = array_diff($cids, array_keys($cache));

I think the intent here was to do something with those CIDs for which no cache item was to be found? Not sure what the plan was, but right now, this line is rather useless :)

Wim Leers’s picture

FileSize
13.3 KB
6.58 KB

Oops, #73 contains one file of another issue.

sun’s picture

+    $cids = array_diff($cids, array_keys($cache));

I think the intent here was to do something with those CIDs for which no cache item was to be found? Not sure what the plan was, but right now, this line is rather useless :)

$cids is passed by reference.

Status: Needs review » Needs work

The last submitted patch, 74: apc_cache-1807662-74.patch, failed testing.

The last submitted patch, 73: apc_cache-1807662-73.patch, failed testing.

Wim Leers’s picture

Component: base system » cache system
Related issues: +#1809248: Enable apc.enable_cli php.ini setting on testbots

#75: aha, overlooked that :) Alright, I have nothing else then.

This is now just blocked on #1809248: Enable apc.enable_cli php.ini setting on testbots, then it should come back green.

damiankloip’s picture

Status: Needs work » Needs review
Related issues: -#1809248: Enable apc.enable_cli php.ini setting on testbots
FileSize
13.28 KB
1.68 KB

Changes look good. However, just using conf_path() as a prefix is not unique enough as that would be E.g. 'sites/default'. You could have multiple sites on a server with the same conf path. I spoke to catch and alexpott in IRC about this and we agreed that using DRUPAL_ROOT too is a fair approach - then md5()ing this string to give us our site prefix.

Also changed the prefixing slightly as before it would be something like 'sites/defaulttags::', that's a little confusing. Now it would be something like '158a9d58b1d7e7fc53d11eeec068ef25::tags::'.

EDIT: Looks like the $cids = ... comment has been resolved too :)

damiankloip’s picture

Wim Leers’s picture

#79: awesome! I figured conf_path() would be insufficient, but we'd then at least move forward. Thanks for figuring out the best way :)

So now we're *really* down to waiting for #1809248: Enable apc.enable_cli php.ini setting on testbots to be fixed.

Status: Needs review » Needs work

The last submitted patch, 79: 1807662-78.patch, failed testing.

Pancho’s picture

Nice. However we might want to support both APCu and the new, alternative user cache YAC which performs better on reads and is also being packaged now.

catch’s picture

That'd need a different implementation, quick look at the github page says it has a key length restriction, which APC doesn't. So it'd need to be its own backend (either core or contrib - contrib for now since the project page says don't use it in production).

damiankloip’s picture

Agreed, that seems very very much like something for contrib. Let's see where that project ends up further down the line.

Wim Leers’s picture

That and,

Yac is under developing, please DO NOT use in real productions

— even their warning has a typo. Definitely not yet coreworthy.

Anonymous’s picture

+    return new ApcuBackend($bin, 'test');

we need to fix that :-)

each test run should use something that won't collide with other runs as the APC prefx.

Mixologic’s picture

If I understand this correctly, we need to upload multiple copies of #79 in one issue so that we can shotgun the testbots so that we can try and get #1883 to grab it?

Mixologic’s picture

Okay. I rerolled and posted the patch from #78, and shotgunned the bot with a mass of them. (https://drupal.org/node/2255215)

The test still failed, but it definitely ran: https://qa.drupal.org/pifr/test/780063

Was this working locally? Is this still an environment issue? or were we expecting this to fail?

Wim Leers’s picture

#89: it works fine locally, including the tests passing.

Mixologic’s picture

I'm inclined to believe that there is still something not the same environment wise between the testbot and whatever folks are doing locally.

It looks like 3 of the patches all hit the bot that was configured to run with apc, and all three had the same 11 errors. Perhaps we need rfay to let us know which version of APC is configured and what its php.ini settings are.

Berdir’s picture

I can reproduce those test fails on 5.5 APCu, both when running on CLI and in the UI.

Mixologic’s picture

Adding a related critical beta target that will impact this.

damiankloip’s picture

Yeah, this issue is probably closer than that one I would say :)

Anonymous’s picture

Status: Needs work » Needs review

mkay, so tags handling was just kinda borkenated forever with the earlier versions of this patch.

attached patch should fix that.

Anonymous’s picture

with patches this time.

Status: Needs review » Needs work

The last submitted patch, 96: 1807662-95.patch, failed testing.

Wim Leers’s picture

96: 1807662-95.patch queued for re-testing.

The last submitted patch, 89: drupal-apcu-1807662-02a5887-89.patch, failed testing.

Mixologic’s picture

Status: Needs work » Needs review

lets try one more time to test #96 this time.

Status: Needs review » Needs work

The last submitted patch, 96: 1807662-95.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
13.76 KB
942 bytes

yay for this running on the bot now :-)

fixed some dodgy invalidate code.

sun’s picture

I wanted to RTBC, but two tasks are still remaining:

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/ApcuBackendUnitTest.php
+  protected function createCacheBackend($bin) {
+    return new ApcuBackend($bin, 'test');
+  }

'test' should be $this->databasePrefix.

Alternatively, if we want to pass what's actually taken, then $this->siteDirectory (= conf_path() during a test).

Both variables are holding unique test class/site specific strings.

Lastly, this test should have a tearDown() method that force-purges the entire user storage (at minimum for the aforementioned test site prefix).

Anonymous’s picture

new patch addresses #103.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks! :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 104: 1807662-104.patch, failed testing.

Wim Leers’s picture

The move of the cache tag metadata deletion from removeBin() to deleteAll() got me thinking. I started talking to beejeebus in IRC, and we came to the joint conclusion that the cache tag implementation of this cache back-end is subtly broken.

The cache tags are being stored not a per-bin basis, but on a per-site basis:

+    $this->binPrefix = $this->sitePrefix . '::' . $this->bin . '::';
+    $this->invalidationsTagsPrefix = $this->sitePrefix . '::itags::';
+    $this->deletionsTagsPrefix = $this->sitePrefix . '::dtags::';

It must be site-wide, because ::checksumTags() uses static::$tagCache (just like the DatabaseBackend) , which means it's being statically cached for all APCu cache bins within this site. But if that's the case, then doing this in ::removebin is wrong:

+    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->deletionsTagsPrefix, '/') . '/'));
+    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->invalidationsTagsPrefix, '/') . '/'));

It was wrong before #104 (and I introduced it, so I'll take blame), and it's wrong since #104, because both ::deleteAll() and ::removeBin() are bin-specific operations, yet they delete the site-wide (and thus covering N APCu cache bins) cache tag metadata for APCu cache bins.

I think there are three possible solutions:

  1. We could simply never delete the cache tags metadata, this would mean that the cache tag metadata APCu entries would never be deleted. That would be acceptable, considering DatabaseBackend does precisely this also. This is probably the easiest and most correct course of action.
  2. We could revert that particular change in #104. Since a ::removeBin() operation is very rare, we could consider it acceptable that in that case, all cache entries in all cache bins of that type are invalidated because the cache tags checksum will no longer match.
  3. We could track cache tags on a bin-specific basis, but that would require us to remove the static from ::checksumTags(). (Since static implies site-wide, rather than bin-specific.)

All of this, because I introduced this in #73 — sorry!

This explains the test failure of #104.

olli’s picture

#107: would #2250033: Garbage collection for cache tag invalidations solve it if we do 1. here?

  1. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -0,0 +1,379 @@
    +  public function getAll($prefix = '') {
    

    protected?

  2. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -0,0 +1,379 @@
    +    apc_store($this->getApcuKey($cid), $cache, $expire);
    

    Could use a shorter $ttl ($expire is REQUEST_TIME + $ttl).

  3. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -0,0 +1,379 @@
    +    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->binPrefix, '/') . '/'));
    

    Could this use the getAll() like apc_delete($this->getAll())?

  4. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -0,0 +1,379 @@
    +  public function invalidateMultiple(array $cids) {
    +    foreach ($this->getMultiple($cids) as $cache) {
    +      $this->set($cache->cid, $cache, REQUEST_TIME - 1);
    +    }
    +  }
    ...
    +  public function invalidateAll() {
    +    foreach ($this->getAll() as $data) {
    +      $cid = str_replace($this->binPrefix, '', $data['key']);
    +      $this->set($cid, $data['value'], REQUEST_TIME - 1);
    +    }
    +  }
    

    I think these should set $data['value']->expire and use apc_store instead of $this->set().

  5. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -0,0 +1,379 @@
    +  public function deleteTags(array $tags) {
    +    foreach ($this->flattenTags($tags) as $tag) {
    +      apc_inc($this->deletionsTagsPrefix . $tag, 1, $success);
    +      if (!$success) {
    +        apc_store($this->deletionsTagsPrefix . $tag, 1);
    +      }
    +    }
    +  }
    ...
    +  public function invalidateTags(array $tags) {
    +    foreach ($this->flattenTags($tags) as $tag) {
    +      apc_inc($this->invalidationsTagsPrefix . $tag, 1, $success);
    +      if (!$success) {
    +        apc_store($this->invalidationsTagsPrefix . $tag, 1);
    +      }
    +    }
    +  }
    

    These should have to reset static $tagCache...

  6. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -0,0 +1,379 @@
    +        if (isset(static::$tagCache[$type][$tag])) {
    +          $checksum[$type] += static::$tagCache[$type][$tag];
    +        }
    +        else {
    +          $query_tags[$type][] = $this->{$type . 'TagsPrefix'} . $tag;
    ...
    +        $result = apc_fetch($query_tags[$type]);
    +        static::$tagCache[$type] = array_merge(static::$tagCache[$type], $result);
    +        $checksum[$type] += array_sum($result);
    

    This never sets the $tag key but the prefixed $tag key.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
13.51 KB

i went with 1. from #107.

the only thing i agreed with from #108 was 1.

Wim Leers’s picture

#109 actually implements #107.2, not #107.1.

damiankloip’s picture

I think we should go with option 1 and leave the tag data intact - keeping it in line with how we do stuff in the db backend at the moment.

Anonymous’s picture

reroll coming today...

damiankloip’s picture

liar!

Anonymous’s picture

FileSize
13.31 KB

am not! am not!

the diff is:

diff --git a/core/lib/Drupal/Core/Cache/ApcuBackend.php b/core/lib/Drupal/Core/Cache/ApcuBackend.php
index eb500a0..2d9275b 100644
--- a/core/lib/Drupal/Core/Cache/ApcuBackend.php
+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -252,8 +252,6 @@ public function garbageCollection() {
    */
   public function removeBin() {
     apc_delete(new \APCIterator('user', '/^' . preg_quote($this->binPrefix, '/') . '/'));
-    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->deletionsTagsPrefix, '/') . '/'));
-    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->invalidationsTagsPrefix, '/') . '/'));
   }
 
   /**
pwolanin’s picture

Status: Needs review » Needs work

Minor, but md5() and sha1() should not be added to Drupal core in 7.x+

moshe weitzman’s picture

Status: Needs work » Needs review

That policy got reversed with the introduction of Symfony. There isn't much point in preserving it for older releases IMO. Or at least suggest a workaround if you are going to downgrade a serious performance issue.

damiankloip’s picture

Yes. You just did the same in another issue for no apparent reason. Let's not base our code on some static code analyser. I guess that's your reason? For a cache key there are not really security implications. These aren't passwords. Plus using sha1 is perfectly sufficient as far as collisions go and to create a site prefix md5 is not going to hurt us at all.

Wim Leers’s picture

From #107:

I think there are three possible solutions:

  1. We could simply never delete the cache tags metadata, this would mean that the cache tag metadata APCu entries would never be deleted. That would be acceptable, considering DatabaseBackend does precisely this also. This is probably the easiest and most correct course of action.

As of #114, this is the case. Back to RTBC per #105, since the main reason this didn't get committed already is because I marked it NW in #107.

Fixing #115 at the same time, after consulting pwolanin in IRC about what should be used instead. Fixed 3 nitpicky nitpicks at the same time. I agree with #116/#117 that this is pointless, but I don't want to delay this patch any longer, so doing this to avoid further bikeshedding.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 118: 1807662-116.patch, failed testing.

sun’s picture

I agree with @moshe + @damiankloip. The wildcard "Don't use md5()/sha1() anywhere" idea (which never was a formal policy btw) has caused a lot of damage and long-term incompatibilities. It's obsolete.

pwolanin’s picture

@sun. I strongly disagree. I also opened at least one issue to fix symfony. It was certainly a policy or we would not have removed every use from Drupal 7 before release.

The fact that some vendored libraries are not up to this standard does not mean that Drupal core itself should regress.

damiankloip’s picture

What are the implications of not using this 'standard' again? I see no point in #115. This is not a policy so we should not be treating it like one. Doing this in d7 also got a lot of kickback - for good reason.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
13.36 KB

Failed due to a core/lib/Drupal/Core/Test/TestKernel.php hunk I've got NFI of how it got there.

Otherwise identical.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review

Sorry.

Crell’s picture

The md5/sha1 issue is because some government procurement policies forbid them as insecure, and any use of them needs extra special justification to show why they're not a security vulnerability. Avoiding them entirely reduces the paperwork involved in getting governments (or at least the US government, which is a huge user of Drupal) to adopt Drupal.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I think #125 clearly articulates the sole valid reason for not using md5() in this scenario: government bureaucracy. Not everyone may agree that it's worth taking into account, but it's a fact that it's irrelevant for this patch how we create the hash.

Again, let's not have this discussion in this issue. This issue has been delayed enough as is. I've opened a new issue where this can be discussed and formalized in an actual policy: #2268875: [Policy, no patch] Using md5()/sha1()/crc32b in Drupal code.

#123 uses the "safer" hash functionality provided by the Crypt utility, the discussion can move to #2268875, hence this issue is absolved of any further discussion on this matter. Back to RTBC.

damiankloip’s picture

ok, fair enough *grumble grumble* :) The patch is looking just fine.

damiankloip’s picture

Hang on, why do we need to base64_encode() this hash for the site prefix too?

Wim Leers’s picture

#128: we don't need that. But this is a ready-to-use hash function, and there's no harm in base64-encoding it. The size of the resulting hash is similar to when we used md5(). Let's not get stuck in bikeshedding over that too.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great! I'd much prefer this over using PhpBackend so it is a great option to have. Committed!

  • Commit 50cd087 on 8.x by Dries:
    Issue #1807662 by sun, beejeebus, damiankloip, Wim Leers, Mixologic:...
Wim Leers’s picture

Issue tags: -sprint

Hurray! :) Now let's get #2248767: Use fast, local cache back-end (APCu, if available) for low-write caches (bootstrap, discovery, and config) done, so that Drupal will actually use this cache backend automatically if possible!

olli’s picture

+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -0,0 +1,377 @@
+    apc_fetch('');

This gives me a PHP Warning: #2269453: apc_fetch() expects a string or array of strings.

damiankloip’s picture

ready-to-use hash function

I'm sorry, I just don't understand this, what about all hash functions already built in to php? I do no see a problem with just using hash('sha256', $this->sitePrefix). Also, that is not bikeshedding (because you wanted it rtbc?!), it's just a valid point.

olli’s picture

Status: Fixed » Closed (fixed)

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