[Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645

nicolas-grekas · 2018-06-19T20:10:26Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#19895
License	MIT
Doc PR	-

With this PR, when igbinary is available, it is automatically used to serialize values.
This provides faster and smaller cache payloads.
The unserializing logic is autoadaptative:

when an igbinary-serialized value is unserialized but the extension is missing, a cache miss is triggered
when a natively-serialized value is unserialized and the extension is available, the native unserialize() is used

Ping @palex-fpt since you provided very useful comments on the topic and might be interested in reviewing here also.

palex-fpt · 2018-06-20T04:03:08Z

src/Symfony/Component/Cache/Traits/AbstractTrait.php

+     * Serializes a list of values using igbinary_serialize() if available, serialize() otherwise.
+     */
+    protected static function serialize(array $values, ?array &$failed): array
+    {


Why is it placed into AbstractTrait? It didn't used not by AbstractTrait nor by AbstractCache. It would be surprise to override serialize method and got nothing. Only 3 of 7 AbstractCache subclasses requires serialization: FilesystemCache, PdoCache and RedisCache.

palex-fpt · 2018-06-20T04:03:52Z

src/Symfony/Component/Cache/Traits/MemcachedTrait.php

@@ -194,6 +194,10 @@ public static function createConnection($servers, array $options = array())
     */
    protected function doSave(array $values, $lifetime)
    {
+        if (!$values = parent::serialize($values, $failed)) {


memcache API allows to store plain PHP vars. And its serialization method can be configured in connection strings. Do we need double serialization here?

palex-fpt · 2018-06-20T04:46:52Z

src/Symfony/Component/Cache/Traits/AbstractTrait.php

                return $value;
            }
-            throw new \DomainException('Failed to unserialize cached value');
+
+            throw new \DomainException(error_get_last() ? error_get_last()['message'] : 'Failed to unserialize cached value');
        } catch (\Error $e) {


Do we need that complex logic here? Why simple call to unserialization function and result check would not be enough?

palex-fpt · 2018-06-20T05:11:19Z

src/Symfony/Component/Cache/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 4.2.0
 -----

+ * automatically use igbinary when available, with graceful error handling


IMHO it is OK to have meaningful defaults for framework. But it is not OK to force settings for components.
[edit: see answer below]

palex-fpt · 2018-06-20T07:18:28Z

Opps. I'm late with review.

Just checked our issue board: there was tickets when we was forced to revert igbinary serialization for some components. Having ability to select serialization method without rebuilding operation environment is critical.

nicolas-grekas · 2018-06-20T07:56:50Z

Having ability to select serialization method without rebuilding operation environment is critical

💯 that's exactly what is provided here: when data is read from the backend, we adaptatively unserialize it using igbinary or native-serialize depending on the payload. Of course, if igbinary is not loaded but the data had been serializing with it, we report a cache miss - there is no other way around.
This allows seamless migration from native-serialize to igbinary-serialize, with data preservation in this direction and without in the other direction. Do you see something else?

memcache API allows to store plain PHP vars. Do we need double serialization here?

That's actually required to provide the auto-adaptative behavior just described above. I'd recommend to not use igbinary at the extension level for this reason also. Maybe even deprecate having it enabled?

not OK to force settings for components.

What's the downside? If there is none, better keep this as an internal detail. If you have igbinary, there is no downside to using it, and since this extension is opt-in, you can always remove it.

nicolas-grekas · 2018-06-20T11:26:41Z

src/Symfony/Component/Cache/Traits/AbstractTrait.php

@@ -110,13 +110,13 @@ public function clear()
            if ($cleared = $this->versioningIsEnabled) {
                $namespaceVersion = 2;
                try {
-                    foreach ($this->doFetch(array('@'.$this->namespace)) as $v) {
+                    foreach ($this->doFetch(array('/'.$this->namespace)) as $v) {


Changing this creates a separate set of keys when using 3.4 to 4.1 vs 4.2 & up. This is required for Memcached adapters because of the added serialization that was delegated to the extension before. Not doing so breaks loading values using 3.4 with a cache that has been populated with 4.2 (can happen during migrations.) Let's be safe. Since versioning is enabled only for Memcached by default, this is nicely selective.

palex-fpt · 2018-06-20T23:46:20Z

This allows seamless migration from native-serialize to igbinary-serialize, with data preservation in this direction and without in the other direction.

This fallback is required due fact that from application point 'nothing' changed. When I reconfigure application cache (change file to memcache, change directory or change serialization mode) I'm pretty OK with repopulating cache. And I prefer fast fail when igbinary is configured for application but is not available on deployment node, than slow running application with hard to detect reasons.

What's the downside? If there is none, better keep this as an internal detail. If you have igbinary, there is no downside to using it, and since this extension is opt-in, you can always remove it.

It is now igbinary is faster. It can be changed. When some other component requires igbinary you would not be able to disable it for cache. Moreover - it can not be easy tested cause enabling/disabling extension is hard.

Well, can we have selectable modes: adaptive, igbinary_only, php_only?

nicolas-grekas · 2018-06-21T12:14:41Z

I prefer fast fail when igbinary is configured for application but is not available

No way: that explicitly conflicts with PSR-6 spirit and wordings (for good reasons IMHO.)

Well, can we have selectable modes?

Done with a new constructor argument: $allowIgbinarySerialize, set to true by default, and forcing native-serialize() when set to false.

nicolas-grekas · 2018-06-22T10:08:09Z

Now with marshaller injection instead of boolean option.

Toflar · 2018-06-25T08:33:51Z

Just a general question: I haven't checked out the code in depth but it seems to me like the Marshaller could be used outside of the Cache component to dump stuff in a performant way, no? Wouldn't it make more sense to have that somewhere else?

…lon" controller service refs (nicolas-grekas) This PR was merged into the 4.1 branch. Discussion ---------- [TwigBundle] bump lowest deps to fix issue with "double-colon" controller service refs | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27645 | License | MIT | Doc PR | - Bumping http-kernel to ~4.1 and conflicting with framework-bundle <4.1. The rest is transitivity. Commits ------- f75723f [TwigBundle] bump lowest deps to fix issue with "double-colon" controller service refs

nicolas-grekas · 2018-06-28T05:16:02Z

ping @symfony/deciders

javiereguiluz · 2018-06-28T08:16:37Z

src/Symfony/Component/Cache/Marshaller/DefaultMarshaller.php

+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class DefaultMarshaller implements MarshallerInterface


Maybe this has been asked, but why do we have a DefaultMarshaller with some internal logic in all methods to differentiate between PHP and IgBinary ... instead of defining a PhpMarshaller and IgBinaryMarshaller (moreover given that we define a MarshallerInterface).

Because there is only one implementation of MarshallerInterface: DefaultMarshaller, which auto-adaptatively uses serialize() or igbinary_serialize().

But that looks to me like creating a DefaultMailer which makes if($use_smtp) { ... } elseif ($use_gmail) { ...} inside its methods. In other parts of the framework we define 1 interface and multiple different implementation classes. Here it looks like we have multiple implementations inside a single class. It looks confusing to me, sorry.

@javiereguiluz it's not the same. Here, by dealing with both serialization methods (and there are none others for PHP objects), we provide an important feature: auto-adaptative unserialization, providing safe forward and backward compatibility at the data level. This is not possible without having something that knows about both formats.

IgBinaryMarshaller could take PhpMarshaller as a fallback marshaller (which is optional), no?

What is the purpose of the MarshallerInterface then if there are no other reasonable implementations?

Thanks to the interface, the serialized string itself can still be processed, e.g compressed and/or encrypted.
One could also build a serializer that would be dedicated to and optimized for very specific data structures (eg only numbers, etc.), etc.

Hmm... But actually it's not true that igbinary is the only serialization format that might benefit from a fallback to PHP serialization. There are / were other contenders like msgpack (MessagePack) and Protobuf, both of which have PHP extensions.

Nice, another reason to have the interface. Let's keep the topic: should we split the default marshaller in two. I think we would do a misfeature by doing so: things would be more complex to understand, more complex to wire, less capable, and less performant while in a critical code path. Would the individual envisioned marshallers empower our users in comparison to these downsides? Not at all in my opinion.

I think you're right, and those other serializer formats are not PHP-specific, so it wouldn't make sense for them to fallback to PHP serialization, actually. 😆

joelwurtz · 2018-06-28T09:20:24Z

What will happen if someone install the igbinary extension on their server but already have a lot of cache ?

nicolas-grekas · 2018-06-28T09:21:21Z

@joelwurtz see test cases: native unserialize() will be used.

teohhanhui · 2018-06-28T15:52:28Z

src/Symfony/Component/Cache/Marshaller/DefaultMarshaller.php

+{
+    private $allowIgbinarySerialize = true;
+
+    public function __construct(bool $allowIgbinarySerialize = true)


A way to force only igbinary (disable the PHP serialization fallback)?

@palex-fpt said:

I prefer fast fail when igbinary is configured for application but is not available on deployment node, than slow running application with hard to detect reasons.

To which @nicolas-grekas you replied:

No way: that explicitly conflicts with PSR-6 spirit and wordings (for good reasons IMHO.)

~~But if the fail-fast behavior means a cache miss, that's not violating PSR-6 in any way.~~ (Bad idea, as it'll result in exactly "slow running application with hard to detect reasons".) It could fail-fast by throwing an exception in the constructor when igbinary is forced but the extension is not available. That would not violate PSR-6.

Correct. Not convinced yet. Really worth it?

TBH, I prefer fail-fast feature over auto-adaptive feature. Reconfigured application cache can and should be warmed up. But having spoiled node that writes to shared cache in php format and forces all other properly configured nodes to use slow method is not good. If someone concerning about preserving old cache data he can use workaround with distinct namespaces and chain cache instances with igbinary an php-serialize.

Here we are: the default mode is auto-adaptative, but if you pass true, you'll get an exception when igbinary is not loaded.

nicolas-grekas · 2018-07-01T06:54:58Z

Any other comment here?
Time to vote @symfony/deciders :)

…providing a default one that automatically uses igbinary when available

fabpot · 2018-07-09T14:39:47Z

Thank you @nicolas-grekas.

@palex-fpt

…he serializer, providing a default one that automatically uses igbinary when available (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #19895 | License | MIT | Doc PR | - With this PR, when igbinary is available, it is automatically used to serialize values. This provides faster and smaller cache payloads. The unserializing logic is autoadaptative: - when an igbinary-serialized value is unserialized but the extension is missing, a cache miss is triggered - when a natively-serialized value is unserialized and the extension is available, the native `unserialize()` is used Ping @palex-fpt since you provided very useful comments on the topic and might be interested in reviewing here also. Commits ------- 9c328c4 [Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available

Koc · 2018-07-15T13:37:52Z

Is it possible decoration for serialization? Create MarshallerCacheItemPool which will decorate any other pool.

nicolas-grekas added this to the next milestone Jun 19, 2018

carsonbot added Status: Needs Review Cache Feature labels Jun 19, 2018

nicolas-grekas force-pushed the cache-igbinary branch 4 times, most recently from d7d91b3 to cfd7492 Compare June 20, 2018 06:22

nicolas-grekas added the Deprecation label Jun 20, 2018

nicolas-grekas force-pushed the cache-igbinary branch 2 times, most recently from 3cc3d37 to 6622803 Compare June 20, 2018 06:34

nicolas-grekas mentioned this pull request Jun 20, 2018

[Cache] Allow to configure serializator for cache instances. #27484

Closed

palex-fpt reviewed Jun 20, 2018

View reviewed changes

nicolas-grekas force-pushed the cache-igbinary branch 2 times, most recently from 36820be to 9a5c2ff Compare June 20, 2018 11:20

nicolas-grekas commented Jun 20, 2018

View reviewed changes

nicolas-grekas force-pushed the cache-igbinary branch from 9a5c2ff to cfc9cd0 Compare June 20, 2018 12:14

nicolas-grekas force-pushed the cache-igbinary branch from cfc9cd0 to c62ca03 Compare June 21, 2018 12:11

nicolas-grekas force-pushed the cache-igbinary branch from c62ca03 to bed9c4b Compare June 22, 2018 06:37

nicolas-grekas changed the title ~~[Cache] automatically use igbinary when available, with graceful error handling~~ [Cache] Add MarshallerInterface allowing to change the serializer, providing a default one that automatically uses igbinary when available Jun 22, 2018

nicolas-grekas force-pushed the cache-igbinary branch from bed9c4b to 523556c Compare June 22, 2018 10:00

nicolas-grekas force-pushed the cache-igbinary branch 2 times, most recently from 99f0534 to fd8d65c Compare June 22, 2018 13:15

nicolas-grekas mentioned this pull request Jun 25, 2018

[TwigBundle] bump lowest deps to fix issue with "double-colon" controller service refs #27702

Merged

nicolas-grekas force-pushed the cache-igbinary branch from fd8d65c to 67e0dca Compare June 25, 2018 12:59

nicolas-grekas mentioned this pull request Jun 25, 2018

[Cache] Added support for Redis igbinary serializer #27709

Closed

javiereguiluz reviewed Jun 28, 2018

View reviewed changes

carsonbot removed the Status: Needs Review label Jun 28, 2018

chalasr approved these changes Jun 28, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Jun 28, 2018

teohhanhui reviewed Jun 28, 2018

View reviewed changes

nicolas-grekas force-pushed the cache-igbinary branch 2 times, most recently from 73ea8dd to 3a7517d Compare June 29, 2018 11:19

nicolas-grekas force-pushed the cache-igbinary branch 2 times, most recently from 3cff315 to 9c328c4 Compare July 8, 2018 08:55

[Cache] Add MarshallerInterface allowing to change the serializer, …

ae4319a

…providing a default one that automatically uses igbinary when available

nicolas-grekas force-pushed the cache-igbinary branch from 9c328c4 to ae4319a Compare July 9, 2018 14:39

fabpot approved these changes Jul 9, 2018

View reviewed changes

nicolas-grekas closed this Jul 9, 2018

nicolas-grekas deleted the cache-igbinary branch July 9, 2018 15:49

javiereguiluz mentioned this pull request Sep 7, 2018

[Cache] Add MarshallerInterface to auto-select the serializer symfony/symfony-docs#10296

Closed

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

derrabus mentioned this pull request Sep 10, 2023

[FrameworkBundle] Remove obsolete feature detection #51620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645

[Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645

[Cache] Add MarshallerInterface allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645

[Cache] Add MarshallerInterface allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645

[Cache] Add `MarshallerInterface` allowing to change the serializer, providing a default one that automatically uses igbinary when available #27645