Make serializer customizable for Cache\DatabaseBackend

This is D8 material.

subscribe

I lhad no idea json_encode() was that much faster. Still, json_encode's inability to restore objects as anything but stdClass makes this less worthwhile IMO, especially now that we have the class registry. Maybe it's worth looking into alternative serialization for specific places we store large simple arrays though. I'm looking at drupal_get_schema() and _theme_load_registry(). We can always json_encode() (or whatever function) the data before passing it into cache_set() as long as we know how to decode it on the way back out.

I have done some limited benchmark with serialize / igserialize / json_encode on PHP 5.3, and it seems that serialize is not slow anymore.

please download the benchmark at
https://github.com/jbergstroem/instabench

please download the php version PHP 5.4.0alpha1 (latest version) in PHP

The latter handles most of the PHP data structures, while the former only handles Javascript arrays (a type of data that doesn't exist in PHP) and Javascript plain objects (that are relatively similar to PHP arrays).
> i think there are two PHP data structures, arrays and objects which json handles better

The latter is binary safe, while the former only handles valid Unicode strings (try json_encode(chr(255)), for example
> if there will be error in the code, i will later patch all invalid unicode strings to json friendly version

please do see the benchmark as serialize are intensively used by drupal, if this benchmark is correct, drupal will be 2.7x faster

D:\php-5.4.0alpha1-Win32-VC9-x86>php.exe ..\nginx-1.0.4\html\jbergstroem-instabe
nch-5f828f9\test.php
InstaBench 0.1 (PHP 5.4.0alpha1)

Benchmarking results (10000 iterations)
=======================================
   serialize: 7507ms (baseline)
  var_export: 19096ms (0.4x slower)
 json_encode: 2794ms (2.7x faster)

i definitely want to choose 2.7x faster drupal and later problem unicode strings like chr(255)
how about others?

please check attached patch if will succeed, many thanks

others please post your benchmark, and let this patch succeed!
imagine, almost 3x speed than the latter. wow!

more benchmarks, raw screenshots attached

json_encode is 
2.3x
3.5x
2.0x
3.1x

faster than serialize 

imagine up to 4x speed of drupal than the latter, wow!
please test my patch, many thanks!

This is just a stupid benchmark. Take anything non trivial (like the attached - we serialize a node generated by Devel Generate), and you will get:

InstaBench 0.1 (PHP 5.3.4)

Benchmarking results (10000 iterations)
=======================================
   serialize: 781ms (baseline)
  var_export: 2259ms (0.3x slower)
 json_encode: 1354ms (0.6x slower)

You can even better results when testing the unserialize . serialize roundtrip.

can you review my patch if it will return success?
my goal is to change serialize/unserialize to drupal_json_encode/ drupal_json_decode

to make encoded versions in blobs in database more readable and more html5 and javascript friendly
against serialize versions.

there are no native serialize out there, but json can be directly readable to javascript.

added some tests in simpletest so it will return success.

#921300: Use json_encode/decode() instead of un/serialize() has been marked as duplicate.

Lessons learned from that issue: The problem with JSON is that it doesn't allow for mixed non-scalar data types. It also does not support 1) serialized class instances and 2) PHP references.

But Drupal stores a lot of serialized data containing mixed data types. The most obvious use-case is:

$form_state = array();
$form_state['values'] = array(...);
$form_state['node'] = stdClass $node;

And that's just one of many examples throughout Drupal core. That's why I stopped working on aforementioned issue.

The only viable option I could see would be to introduce an optional flag, in particular for cache_set(), that would allow the caller to say "I absolutely and definitely know that this data is an array and doesn't contain any objects or references, so you may use JSON for serialization." -- combined with a new or revised "serialized" schema column that tells how to unserialize for each record.

Closed #1340260: Support for igbinary as a duplicate of this issue.

The topic of this issue doesn't really match the patch in #11, so I'm moving this back to active. I also get the feeling that the performance of unserialize() may depend heavily on which version of PHP you're running. In 2010, PHP apparently made an adjustment to HEAD that sped up unserialize signficantly. In that issue at least they say it couldn't be included in PHP 5.3 for compatibility reasons, so I'm guessing it's only in PHP 5.4.

I was doing research into the igbinary serializer today and found this issue. Against PHP 5.3.22 I didn't find igbinary serialize to be that much faster. According to XHProf, igbinary was only marginally faster (with its "compact_strings" option turned on).

However, the *size* of the serialized data was significantly reduced. Most entries in the cache_* tables were reduced by about half, and entries that had a lot of redundant data (such as cache_form) reduced entries to a 1/3 - 1/4 the original size. Larger reductions are seen the larger the form being serialized.

So overall, yay, I think I like igbinary_serialize() and igbinary_unserialize(). Combined with the reduction in overhead of transferring this data to the database or memcache, you could get some modest improvements.

However my opinion on this is that making the serializer customizable (per this issue title) is unnecessary. unserialize() is already getting faster in newer versions of PHP (apparently, I don't have numbers on that), but more over, if you want to replace the largest uses of serialize/unserialize, you can do so already by providing a custom cache back-end. Alternatively I think it would be a great idea for existing cache backends (Redis, Memcache, etc.) to support using igbinary_serialize() automatically if it is available. The largest unserialization strings we have (and most of them) are coming from cache_get(). Since this is already swappable, I don't think any additional effort is required on the part of core.

Hey, sorry for digging this up but has anyone had solution for igbinary support? I'm thinking about swappable handler also, just like quicksketch mentioned above but still don't know how to implement it. Because when the cache is built with native serializer, and right after that we get and set new cache with igbinary, everything will be broken

I already made a copy here https://github.com/in2pire/drupal-igbinary just for research. Replaced all serialize() and unserialize() with igbinary functions.

Hope that we can find some ways to add the support for igbinary soon.

I was talking to Pierre Joye at DrupalSouth and he mentioned we should not be using serialize due to a) it can break between PHP versions. b) its not secure. After this discussion it should perhaps be raised to higher priority.

igbinary_cache is a step in the right direction but there's a lot of other contributed modules that use serialize/unserialize function calls directly that could benefit. Truthfully it's unfortunate that there's not a direct drop-in replacement on the PHP level to handle this. Seems like it would make the most sense to do it there so that the syntax for serialize and unserialize could be independant of the actual library doing the serialization. They already do this with the session storage option would be nice to expand it into the functions themselves too.

I created https://www.drupal.org/project/igbinary
It enables igbinary for Drupal's key value stores.

The next step would be to create a set of core patches to delegate serialization to services implementing Drupal\Component\Serialization\SerializationInterface instead of calling serialize() and unserialize() directly. In core they should leverage the PHPSerialize by default, just like the key value stores.

Drupal-like hard-coded dependency injection :-(

Sorry, #24 was incomplete.

I think this looks great. Does it make sense to introduce a CacheBackendBase and CacheBackendFactoryBase for this?

Does it make sense to introduce a CacheBackendBase and CacheBackendFactoryBase for this

I don't think so. Other Backends don't need that serialization. For example APCu handles serialization by itself (and could be configured in php.ini to use igbinary).

nitpicks

+++ b/core/tests/Drupal/KernelTests/Core/Cache/BackendChainTest.php
@@ -17,9 +17,9 @@ protected function createCacheBackend($bin) {
+      ->appendBackend(new MemoryBackend(\Drupal::service('serialization.phpserialize')))
+      ->prependBackend(new MemoryBackend(\Drupal::service('serialization.phpserialize')))
+      ->appendBackend(new MemoryBackend(\Drupal::service('serialization.phpserialize')));

+++ b/core/tests/Drupal/KernelTests/Core/Cache/ChainedFastBackendTest.php
@@ -20,7 +20,7 @@ class ChainedFastBackendTest extends GenericCacheBackendUnitTestBase {
+    $consistent_backend = new DatabaseBackend(\Drupal::service('serialization.phpserialize'), \Drupal::service('database'), \Drupal::service('cache_tags.invalidator.checksum'), $bin);

+++ b/core/tests/Drupal/KernelTests/Core/Cache/MemoryBackendTest.php
@@ -18,7 +18,7 @@ class MemoryBackendTest extends GenericCacheBackendUnitTestBase {
+    $backend = new MemoryBackend(\Drupal::service('serialization.phpserialize'));

Kernel tests have access to the container so instead retrieving the php serializer through \Drupal:service you should use $this->container->get.

Beside that great job!

@hchonov: your comment is correct. But all the code nearby in the tests uses \Drupal::service(), for example:

  protected function createCacheBackend($bin) {
    $consistent_backend = new DatabaseBackend(\Drupal::service('serialization.phpserialize'), \Drupal::service('database'), \Drupal::service('cache_tags.invalidator.checksum'), $bin);
    $fast_backend = new PhpBackend($bin, \Drupal::service('cache_tags.invalidator.checksum'));
    $backend = new ChainedFastBackend($consistent_backend, $fast_backend, $bin);
    // Explicitly register the cache bin as it can not work through the
    // cache bin list in the container.
    \Drupal::service('cache_tags.invalidator')->addInvalidator($backend);
    return $backend;
  }

For me it looks inconsistent to use $this->container-get() just for the serializaation service there. Maybe all of these service() calls have to be replaced, but that seems to be out of scope for this issue.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -200,7 +224,9 @@ protected function getRequestTime() {
+    return array_filter($this->traitSleep(), function($var) {
+      return '_serviceIds' == $var;
+    });

This could be done without array_filter :
$this->traitSleep();
return ['_serviceIds'];

Also we need a comment here why simply returning $this->traitSleep(); is not sufficient.

This could be done without array_filter :
$this->traitSleep();
return ['_serviceIds'];

That's not true. This code always returns _serviceIds even if $this->traitSleep() didn't add that property!

But it will add it always.. and thinking about it is correct for the method to add the property always as otherwise you do not have to use the trait. Take a look at DependencySerializationTrait::__wakeup it does not check if the property is set but accesses it directly.

I reviewed the code. You're right the property _serviceIds is always set.

re-roll against 8.3.x

That's funny. I had to fix all new usages of the MemoryBackend in the tests. But these were erroneous anyway because they use the obsolete parameter 'bin' which was removed in 8.3.x.

I think the comment in the overridden sleep method should be something like this :

In case the serializer has been injected as a service then we have to ensure it will be properly recovered on wake up, otherwise we do not take care of it in order to prevent data stored in memory backends from being serialized.

What do you think?

I randomly picked the key "locale.translation_status" from the key_value table from an existing installation. The size of the php serialized string is 27k the igbinary serialized is 7k :-)
I'll go on to get some more numbers and update the issue description.

new in 8.3.x setiings:

/**
 * Disable caching for migrations.
 *
 * Uncomment the code below to only store migrations in memory and not in the
 * database. This makes it easier to develop custom migrations.
 */
# $settings['cache']['bins']['discovery_migration'] = 'cache.backend.memory';

For sure this feature will benefit from using igbinary if this core patch is committed because of the much smaller memory footprint.

After a discussion with @catch in IRC I came up with the idea to define an interface that ensures that only serializers that are able to handle PHP objects could be injected.

OK, the tests pass, except the ones that currently broken on the 8.3.x regardless of this patch here.

Seems like this patch could be simplified greatly if PhpSerialize was set as a default argument. At least for the MemoryBackend.

It's also a BC break, but not sure how those are counted.

Seems like this patch could be simplified greatly if PhpSerialize was set as a default argument. At least for the MemoryBackend.

Do you have an idea how to do this?

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -14,12 +17,33 @@
+  public function __construct(ObjectSerializationInterface $serializer = NULL) {
+    $this->serializer = $serializer ?: new PhpSerialize();
+  }

@twistor: Sure, that works. But my quick comment wasn't precise enough, sorry :-(

I agree that this will reduce the size of the patch. But that only affects test code.

But such a constructor will open the door for bad usages of the MemoryBackend if it could be instantiated without thinking about the serializer.
Especially for the MemoryBackend I want to inject a compressing serializer.

And the serializer service is a singleton. There's no need to create multiple instances of it - beside in test code.
So the correct default would be to get the serialization.phpserialize service. But this will not work in the test code when the service is not available.

Conclusion:
- Having this default will not simplify the code base, just the test code.
- Having this default allows developers to accidentally bypass the configured serializer.

What we can do is providing a derived TestMemoryBackend within the test code.

I adjusted the patch according to a discussion with @catch in IRC.

catch: I think for database it makes sense - if we add a new interface to identify serializers that can handle php objects. For memory I'd just do a new cache backend that uses igbinary rather than add the dependency there.
mkalkbrenner: are you fine with the interface name?
catch: hmm, it suggests it's only for serializing objects, but I don't immediately have a better suggestion.
mkalkbrenner: Something like ObjectAware?
catch: yeah that's better :)
catch: and we should open a follow-up postponed to use that for k/v

I renamed the interface to ObjectAwareSerializationInterfacea and reverted the changes to the MemoryBackend.
A new MemoryBackend will then be provided by the igbinary module.

Just opened the follow-up like @catch suggested.
#2839862: Ensure that serializers injectable into Drupal\Core\KeyValueStore are aware of PHP objects

Just the annoying standard 8.3 CI fails. The patch itself passes as the retest proves

1. +++ b/core/lib/Drupal/Component/Serialization/ObjectAwareSerializationInterface.php
   @@ -0,0 +1,9 @@
   +/**
   + * Ensures that a serializer is usable for serializing PHP objects.
   + */
   +interface ObjectAwareSerializationInterface extends SerializationInterface {
   

   Coming from #2143149: PHPRedis and igbinary support for read performance and memory reductions..

   what about defining a new service that's specifically for this purpose? Something like serialization.object_serializer or some other generic service name. Then your igbinary module could just override that service and anything that uses it will automatically benefit from it and it won't need specific redis and $other_module integration?

   Finding a good name for that might be a challenge.

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -46,11 +55,12 @@ class DatabaseBackend implements CacheBackendInterface {
       * @param string $bin
       *   The cache bin for which the object is created.
       */
   -  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, $bin) {
   +  public function __construct(ObjectAwareSerializationInterface $serializer, Connection $connection, CacheTagsChecksumInterface $checksum_provider, $bin) {
        // All cache tables should be prefixed with 'cache_'.
        $bin = 'cache_' . $bin;
   

   I could imagine this is a class that has some custom subclasses in contrib and changing the constructor could break them.

   what we did recently is to add the new argument last and fall back to \Drupal::service() if not provided.

what about defining a new service that's specifically for this purpose?

That sounds like a simple solution but I don't think that it would be a good solution. With the igbinary module installed you have 4 object aware serializers:

• PHP
• PHP compressed
• igbinary
• igbinary compressed

For redis it is very important to select a compressing serializer. For the default database cache backend and the key value stores you might want to choose compression as well. But I think you'll sooner or later face a situation where you don't want to have compression for a particular sub-system.
Or you use igbinary and compression for redis but you have another component that somehow shares serialized objects with a different system or exports serialized objects. In this case you should use the default PHP serializer without compression for the maximum compatibility.

I could imagine this is a class that has some custom subclasses in contrib and changing the constructor could break them.
what we did recently is to add the new argument last and fall back to \Drupal::service() if not provided.

I thought about that as well. I did it the current way to follow the given pattern of the key avlue store implementation that already exists in core. But if the maintainers prefer this solution I'm fine to change it.

Or you use igbinary and compression for redis but you have another component that somehow shares serialized objects with a different system or exports serialized objects. In this case you should use the default PHP serializer without compression for the maximum compatibility.

I think we might be able to "solve" that with a service name that explains what it is. serializer.optimized_internal sounds weird, but something in that direction.

I assume the developer that defines a service knows whether he can use that or needs to use phpserialize. And if we have a name that explains thoes developers when to use it and when not, then I don't see a problem with your use case?

I think we might be able to "solve" that with a service name that explains what it is. serializer.optimized_internal sounds weird, but something in that direction.

But if you introduce that service at least the key value store and the database cache backend will use it in core. And redis in contrib :-)
In this case - just like you proposed - it would be easy for the igbinary module to swap that service and to enable the "igbinary compressed" serializer for all of them.
But if you use mongodb with it's internal compression for the key value store and redis for the cache you need to different flavors of that service.
I know that these are edge cases ;-)
Finally that additionally service won't hurt and if others agree, I'm fine to introduce it.

I prepared a version of the patch that reflects @berdir's suggestions.

+++ a/core/tests/Drupal/KernelTests/Core/Cache/ChainedFastBackendTest.php
@@ -20,7 +20,7 @@
    */
   protected function createCacheBackend($bin) {
+    $consistent_backend = new DatabaseBackend(\Drupal::service('database'), \Drupal::service('cache_tags.invalidator.checksum'), $bin);
-    $consistent_backend = new DatabaseBackend(\Drupal::service('serialization.phpserialize'), \Drupal::service('database'), \Drupal::service('cache_tags.invalidator.checksum'), $bin);
     $fast_backend = new PhpBackend($bin, \Drupal::service('cache_tags.invalidator.checksum'));

+++ a/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
@@ -25,7 +25,7 @@
   protected function createCacheBackend($bin) {
+    return new DatabaseBackend($this->container->get('database'), $this->container->get('cache_tags.invalidator.checksum'), $bin);
-    return new DatabaseBackend($this->container->get('serialization.phpserialize'), $this->container->get('database'), $this->container->get('cache_tags.invalidator.checksum'), $bin);
   }
 

+++ a/core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php
@@ -69,7 +69,6 @@
     $container->register('cache_factory', 'Drupal\Core\Cache\DatabaseBackendFactory')
-      ->addArgument(new Reference('serialization.phpserialize'))
       ->addArgument(new Reference('database'))
       ->addArgument(new Reference('cache_tags.invalidator.checksum'));

+++ a/core/tests/Drupal/KernelTests/Core/Config/Storage/CachedStorageTest.php
@@ -87,7 +87,6 @@
-      ->addArgument(new Reference('serialization.phpserialize'))
       ->addArgument(new Reference('database'))
       ->addArgument(new Reference('cache_tags.invalidator.checksum'));

As commented in IRC, lets keep the explicit argument here, at the new position. The fallback is BC, and we don't want to rely on our own BC, so it will be easy to remove in 9.x

Come on CI ...

How was I now aware of this issue all this time?! :O

For example igbinary shrinks the size of larger serialized PHP objects to one third without losing any performance.

Awesome, how have I never heard of igbinary?!

1. +++ b/core/core.services.yml
   @@ -421,6 +421,8 @@ services:
   +  serialization.objectaware.default:
   +    alias: serialization.phpserialize
   

   Let's add a comment to this to explain the purpose of this alias.

2. +++ b/core/lib/Drupal/Component/Serialization/ObjectAwareSerializationInterface.php
   @@ -0,0 +1,9 @@
   +interface ObjectAwareSerializationInterface extends SerializationInterface {
   

   This name sounds a bit strange to me. What's "object-aware" about this? It seems like it's just a serializer capable of handling any PHP data type, there's nothing object-specific about it?

   Also, what does this add on top of \Drupal\Component\Serialization\SerializationInterface?

   The comment does not make this clear at all for me.

3. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -21,6 +22,12 @@ class DatabaseBackend implements CacheBackendInterface {
   +   * The serialization class to use.
   ...
   +   * @var \Drupal\Component\Serialization\ObjectAwareSerializationInterface
   ...
   +  protected $serializer;
   

   "serialization class" vs "serialization interface" vs "serializer"

   Let's please be consistent.

4. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -45,14 +52,17 @@ class DatabaseBackend implements CacheBackendInterface {
   +  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, $bin, ObjectAwareSerializationInterface $serializer = NULL) {
   ...
   +    $this->serializer = $serializer ?: \Drupal::service('serialization.objectaware.default');
   
   +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -27,10 +35,13 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
   -  function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider) {
   +  function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, ObjectAwareSerializationInterface $serializer = NULL) {
   ...
   +    $this->serializer = $serializer ?: \Drupal::service('serialization.objectaware.default');
   

   Why make this optional? For BC? https://www.drupal.org/core/d8-bc-policy says constructors do not need to maintain BC.

4. We technically don't, but this is becoming a standard pattern, specifically in classes that are likely to be overriden by contrib. I know there is at least one customized cache backend in contrib that implements a max ttl, which likely overrides this. It might or might not have additional constructor arguments, not sure.

@Wim Leers:

2. @catch and myself agreed on the requirement for such an interface and that Interface name. Have a look at #71. JSON and YAML are suitable for other PHP types except objects. @catch also requested a follow-up to leverage the new interface for the key value store. See #2839862: Ensure that serializers injectable into Drupal\Core\KeyValueStore are aware of PHP objects

3. This inconsistency already exists in core. Have a look at the key value stores which are the blue print for the patch here. If we decide to clean that up, it should be a larger follow-up.

I agree on 1 but there should be a decision on 4 before a final patch.

#94: Yep, for a cache back end, that's fair.

#95.2: that sounds like a great reason… but can you then just add that reasoning to the interface itself as well? Then this would've been instantly clear :)

#95.3: Ok, but then let's at least be locally consistent.

As discussed with @berdir in IRC there's no need for an object aware serialization default service in core and for example the redis module. Each component should simply use the serialization.phpserialize as it is. That's the defacto default.

I think I now addressed all concerns.

Just one incredibly stupid nit. I'm fine with this now. I'll leave it to @berdir to RTBC.

+++ b/core/lib/Drupal/Component/Serialization/ObjectAwareSerializationInterface.php
@@ -0,0 +1,16 @@
+ * suitable for PHP objects, for example using the PHP string serilization

s/serilization/serialization/

Fixed the nitpick :-)

Ok, lets do this. Will have to try out the igbinary module on our sites :)

That said, I think this belongs in the cache component.

Also not sure if this needs a change notice or not, e.g. for the new interface. It is a new thing, but is also something that very, very few people will need to worry about I think, so might be more noise than necessary?

This failed on the new test introduced in #2793849: Handle offcanvas differently at lower widths. The question is: Is that a new random failure or is that related in some super bizarre way? Sending for a re-test anyway...

Ahh, that was reverted now, so assuming this will come back green then.

Retest suceeded.

Bot vs. human: Episode III

I'm still not 100% sure about this, for a couple of reasons, didn't really think of the second one during the irc conversation referenced above:

- This adds an additional dependency for the cache subsystem, which in the default case is required entirely to call serialize(). That's loading an interface and a class to call a PHP native function in 99.9% of cases. If we ever do a subtree split, that'll be even more obvious.

- I'm not sure how many classes we have to load to serve a cached page with the database backend, but as above this adds at least one interface and one class to that number. On a site not using the APCu class loader (and using the database cache, so not that unlikely) this might even be measurable.

Seems like we could refactor the serialization logic in the database cache backend to make it easier for the igbinary contrib module to extend from as an alternative.

Seems like we could refactor the serialization logic in the database cache backend to make it easier for the igbinary contrib module to extend from as an alternative.

We use Redis for most of the caches.

The igbinary module simply swaps the serialization.phpserialize service to do the job for the database and the redis backend at once. The same would happen for any other backend that uses the serialization sevice.

This adds an additional dependency for the cache subsystem, which in the default case is required entirely to call serialize()

From my point of view this default isn't a good choice at all. For a lot of use cases it makes a lot of sense to at least compress the serialized data. That hard-coded call to serialize() prevents us from optimizing the caching.

Re-roll only.

From my point of view this default isn't a good choice at all. For a lot of use cases it makes a lot of sense to at least compress the serialized data. That hard-coded call to serialize() prevents us from optimizing the caching.

It doesn't prevent it though. We could refactor the database cache backend so the serialize() call is in a method, then igbinary module could subclass and change that one method. Even as it is now it'd be possible to subclass, just not very efficient due to amount of duplicated code necessary.

Simple re-roll of #113

Fixing the failing test.

This looks very similar to #2886405: Cache backend should use the serializer service., do we close one of them as duplicate?

Some related work is being done over at #1281408: Add a compressing serializer decorator as well.

Adding related issues per #120 + #121.

This is just an idea, but perhaps to ensure something like #1281408: Add a compressing serializer decorator is possible, the serializer should potentially have control of the 'serialize' data field of the calling code.

So that several chained serializers can store their formats and don't need to create new objects, which need to be serialized again.

I just want to mention that https://www.drupal.org/project/igbinary is prepared for this patch here since 18 months.

I think it's really bad idea to add this extra overhead (function calls + loading service) to every cache operation
PHP has common way to define serialization and I remember a lot of issues when "milti-head" was configured differently for php serializer

Making another wrapper for build-in php stuff makes it hard to understand/learn for other newcomers

If you wanna make serializer configurable - it require to replace all places where serialize() or unserialize() used + all contrib which rely on it

I think it is fine to allow contrib to override this easily, but what about we add an:

$function = isset($this->serializer) ? $this->serializer : 'serialize';

instead of injecting in core.

And yes switching serializer on the fly is always asking for trouble.

PHP has common way to define serialization and I remember a lot of issues when "milti-head" was configured differently for php serializer

You can swap the serializer for igbinary this way, but I don't think that you can replace the php serializer by a compressing one this way.

And yes switching serializer on the fly is always asking for trouble.

There're multiple serializers with fallback strategies within https://www.drupal.org/project/igbinary
Using them you don't crash your site by swapping the serializer!
I guess, Drupal\igbinary\Component\Serialization\PhpCompressSerialize will be sufficient for most people.

I think it's really bad idea to add this extra overhead (function calls + loading service) to every cache operation

I don't agree. Having compression enabled gave us a performance boost regarding the complete stack (not just Drupal, but redis and DB caches).
As you might have noticed, there already exists such a service in core for a long time! It's simply not used in every place.
As far as I remember one reason for that was, that you want to ensure in some places that nobody injects a serializer that is not suitable for PHP objects. That's why the patch introduced the ObjectAwareSerializationInterface. Just have a look back in the comments.

Re-roll for #2853503: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2.

+1 for me. Great idea to make the backend more flexible. I also agree with @andypost that this issue needs profiling.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -73,8 +80,10 @@ class DatabaseBackend implements CacheBackendInterface {
+   * @param \Drupal\Component\Serialization\ObjectAwareSerializationInterface $serializer
+   *   The serializer to use.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
@@ -37,13 +45,16 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
+   * @param \Drupal\Component\Serialization\ObjectAwareSerializationInterface $serializer
+   *   The serializer to use.

Probably we should make clear here that this param is optional only for BC reasons. Then we need a @todo and D9 followup to make this param mandatory in D9.

I profiled this and the overhead (which is micro optimization) is well worth the win -- as many more sites have igbinary available out of the box and that saves 50% on serialization operations, which is pretty huge.

Therefore and by reviewing this patch:

RTBC

I created #3014514: Make igbinary the default serializer if available, it saves 50% time on unserialize and memory footprint as followup.

I created #3014511: Increase performance and scalability of the cache subsystem as parent issue.

Fixed #130, added #3014548: Make the serializer mandatory param for DatabaseBackend and DatabaseBackendFactory as Drupal 9.x followup.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -82,6 +94,7 @@ public function __construct(Connection $connection, CacheTagsChecksumInterface $
+    $this->serializer = $serializer ?: \Drupal::service('serialization.phpserialize');

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
@@ -37,13 +45,19 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
+    $this->serializer = $serializer ?: \Drupal::service('serialization.phpserialize');

We should be triggering a deprecated error here so that when we come to make it mandatory in D9 we've been warning people for 2+ years. We'll also need a legacy test for that.

Other than that, this looks great

Adding the deprecation stuff.

Implemented #135. Removed the reference to follow-up as is not a standard deprecation policy. We rely on error triggering.

Back to RTBC as #135 is addressed.

Thanks @claudiu.cristea!! Also wrote a change notice pretty much at the same time in order to help get this issue across the finish line... ...so we now have two. 🙈 Feel free to delete mine, whoever was the powers to do so.

EDIT: Well, technically, we now have three, because I also wrote a change notice for ObjectAwareSerializationInterface but I think that one has merit on its own, so I think we should keep that one.

Adding credit for those who reviewed to shape the patch, profiled, wrote change records

there was no answer to #114 here

Also the issues I raised in #3014514: Make igbinary the default serializer if available, it saves 50% time on unserialize and memory footprint could be solved by having distinct database tables, which lends towards a subclass too.

@Fabianx did you profile page cache hits without igbinary?

@catch: I suppose an implementation that *dynamically* picks the better serializer has a much bigger issue with having to deal with it changing on the fly. If you do it based on manual configuration, it's not that crazy to expect to properly clear caches.

But if we want to, we could also make the table name based on the serializer class dynamically, e.g. check if it's not the default and in that case, use a few characters of the hash of the fully qualified service name or so?

Subclassing wouldn't just require a different class but also a different factory class. The advantage of using the serializer service is that the same can be used for different cache backends, redis already supports this as mentioned above.

#142 I did and the overhead for the new service injection is neglectable (compared to what Symfony base has as overhead already).

For #114:

As that is static configuration you should be matching that across environments. Also as we live in the "age of docker" it is much more likely to have local dev envs match production.

In the worst case an automatic igbinary service could add the format as a one number thing at the start.

For a cache it's not a big problem if it's invalid information as it can just return FALSE.

Though that is the problem, okay as FALSE is still valid for $item->data.

Hmm, so what we would need is that we can return an invalid value from ObjectSerializationInterface or throw an Exception that we can catch?

e.g. like a class ObjectSerializationInvalidValue that could be used to check that the result is valid?

And as we are introducing a new interface it would be indeed good to get that right.

Because the compressor also has the same problem that it might not be able to decompress the data, if e.g. bzip2 extension is not available, etc. and then still needs to return something sane.

For the cache we can rebuild, but what about other uses where we store permanent non-recreatable data in serialized format?

I think if the object serialization interface says that you need to match it OR ensure you can handle decode failures, then that is okay.

I am setting to "Needs work" for potential interface changes.

We use igbinary serialization in production for two years now using this core patch and a special branch of the drupal igbinary module: https://cgit.drupalcode.org/igbinary?h=serialization.objectaware.default

We use it for every kind of serialization in Drupal, like caches and the key value stores! If you have a closer look at https://cgit.drupalcode.org/igbinary/tree/src/Component/Serialization?h=... you'll see that I implemented a simple Fallback mechanism to deal with "old" entries that have been serialized earlier in a different format. If required, we can move these Serializers into core, too.

In the current mix of patches, some (small) parts are tricky to use igbinary everywhere. Maybe that could be solved in an easier way if we have everything in core. Meanwhile we use these settings to leverage igbinary early ;-)

$class_loader->addPsr4('Drupal\\redis\\', 'modules/contrib/redis/src');
$class_loader->addPsr4('Drupal\\igbinary\\', 'modules/contrib/igbinary/src');

$settings['bootstrap_container_definition'] = [
  'parameters' => [],
  'services' => [
    'redis.factory' => [
      'class' => 'Drupal\redis\ClientFactory',
    ],
    'cache.backend.redis' => [
      'class' => 'Drupal\redis\Cache\CacheBackendFactory',
      'arguments' => ['@redis.factory', '@cache_tags_provider.container', '@serialization.phpserialize'],
    ],
    'cache.container' => [
      'class' => '\Drupal\redis\Cache\PhpRedis',
      'factory' => ['@cache.backend.redis', 'get'],
      'arguments' => ['container'],
    ],
    'cache_tags_provider.container' => [
      'class' => 'Drupal\redis\Cache\RedisCacheTagsChecksum',
      'arguments' => ['@redis.factory'],
    ],
    'serialization.phpserialize' => [
      'class' => 'Drupal\igbinary\Component\Serialization\IgbinaryCompressSerialize',
    ],
  ],
];

Re-roll and clean-up

subscribe

Fixed deprecation

Fix broken tests and paraphrased messages to fit new coder's standards

Fix message for factory

Simple reroll against 8.9.x.

I think once this passes tests again, it can go back to RTBC. This only adds the serializer for the cache, where re-creating values is not an issue.

Test fixed by updating cache_factory service registration for kernel test.

Back to RTBC

Only thing that might warrant a fallback thingy is that this breaks BC for anyone overriding the bootstrap container as there the PHP serialization service obviously does not exist.

Do maybe hardcoding the class to use is better instead of \Drupal::service() as it was hardcoded before?

#160 maybe this should be coordinated with the modules which (might) do this: memcache, redis..?

I had to update the deprecation messages as this might not get committed in 8.8.0, therefore I changed the version from 8.8.0 to 8.8.x.

#160 maybe this should be coordinated with the modules which (might) do this: memcache, redis..?

I am not sure whether the modules override the bootstrap container themselves or the site owners in their settings.php files.

Only thing that might warrant a fallback thingy is that this breaks BC for anyone overriding the bootstrap container as there the PHP serialization service obviously does not exist.

Do maybe hardcoding the class to use is better instead of \Drupal::service() as it was hardcoded before?

I would rather add the PHP serialization service to the retrieved bootstrap container definition. We could achieve this either by adding it directly or by merging the default bootstrap container definition into the retrieved one. I would prefer merging the bootstrap container definitions, as this is a more general solution and will allow us further adding new services to the bootstrap container definition without worrying about overridden bootstrap container definitions.

Here I upload 3 patches:

1. 839444-165.patch - #158 with updated version reference.
2. 839444-165--bc-bootstrap-service.patch - BC layer for adding the PHP serialization service to the retrieved bootstrap container definition.
3. 839444-165--merge-bootstrap-definitions.patch - merging the retrieved bootstrap container definition with the default one.

My feeling is the explicit declaration is better. Leaving decision to framework managers.

Back to RTBC

I've re-rolled the patches for Drupal 9 and 8.9.
The interdiff contains the resolved conflict in DeprecationListenerTrait
It needs review, please

Back to RTBC as those are simply re-rolls without any functional changes. Thank you, @gnunes!

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -73,8 +80,10 @@ class DatabaseBackend implements CacheBackendInterface {
-  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, $bin, $max_rows = NULL) {
+  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, $bin, $max_rows = NULL, ObjectAwareSerializationInterface $serializer = NULL) {

@@ -82,6 +91,11 @@ public function __construct(Connection $connection, CacheTagsChecksumInterface $
+    if (!$serializer) {
+      @trigger_error('Calling DatabaseBackend::__construct() without the $serializer argument is deprecated in drupal:8.8.x and the $serializer argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3014688', E_USER_DEPRECATED);
+      $serializer = \Drupal::service('serialization.phpserialize');
+    }
+    $this->serializer = $serializer;

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
@@ -37,13 +45,20 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
-  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, Settings $settings = NULL) {
+  public function __construct(Connection $connection, CacheTagsChecksumInterface $checksum_provider, Settings $settings = NULL, ObjectAwareSerializationInterface $serializer = NULL) {
...
+    if (!$serializer) {
+      @trigger_error('Calling DatabaseBackendFactory::__construct() without the $serializer argument is deprecated in drupal:8.8.x and the $serializer argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3014688', E_USER_DEPRECATED);
+      $serializer = \Drupal::service('serialization.phpserialize');
+    }
+    $this->serializer = $serializer;

This is a bit tricky. In the Drupal 9 patch we shouldn't have the deprecation... but making the serializer argument not null is not possible because there are nullable arguments before them. In order to make it required we're going to need to swap arguments around which is possible by removing argument types and doing deprecations but it is not simple.

This needs thinking about.

Also the whole bc-bootstrap-service vs merge-bootstrap-definitions vs doing nothing needs to be chosen before this is RTBC. And if we decide to do merge-bootstrap-definitions then I think we should handle that separately in a blocking issue so we can add explicit test coverage. Putting my framework manager hat on I think ensuring all the services we think should be there are there is important. The bootstrap container is supposed to provide stable place from which to work and this has uncovered the unsurprising that replace-ability means that we need to do more work to make this so.

Also for now I'd target 9.0.x and not worry about 8.9.x and we should handle a backport if and when we get to that.

I've removed the deprecation warning for 9.0.x
It needs review, please.

I've removed the tests for the deprecating strings.
It needs review, please.