[Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter #33939
Conversation
|
What does this actually do (is there now double serialization of value for instance?), and why does it do it? |
This allows unserializing only the tags when only them are needed, in |
b87f933 to
9d760fb
Compare
9d760fb to
dbda21b
Compare
ok, that's what I thought. Anyway we can avoid the double serialization? |
I suppose yes, I'll have a look. |
dbda21b to
bbf540b
Compare
|
@andrerom updated, please have a look at |
a60d466 to
b2269ca
Compare
src/Symfony/Component/Cache/Adapter/AbstractTagAwareAdapter.php
Outdated
Show resolved
Hide resolved
b2269ca to
0cc9ff5
Compare
There was a problem hiding this comment.
Looks to add quite a bit of complexity. Sure you don't want to solve this by just storing tags separately in adapters where that makes sense and for now treat indeed serialization on delete as know issue for file system? (until we have a reliable way to do parallel and/or async file reading)
Anyway, added several review comments here, some might be missing the point though.
src/Symfony/Component/Cache/Adapter/FilesystemTagAwareAdapter.php
Outdated
Show resolved
Hide resolved
0cc9ff5 to
cc35a60
Compare
|
@andrerom comments addressed hopefully. |
cc35a60 to
2f7f91e
Compare
|
Looks to add quite a bit of complexity, sure you don't want to solve this
by just storing tags separately in adapters where that makes sense
That would be more complex to implement to me. E.g. we'd want values and
tags to be stored on the same Redis node, fetching logic would also need to
be patched, etc.
The added code here on the contrary builds only on existing design roots.
To me it's a first needed step that fixes the unserialization issue in a
generic way.
The split at the storage level would be an extra step to also save
downloading the data on delete. This would benefit ppl that deal with big
enough payloads. I'd wait for someone to report they need it before caring.
for now treat indeed serialization on delete as know issue for file system? *(until
we have a reliable way to do parallel and/or async file reading)*
That's solved already with this PR, see previous answer too.
|
That would be the simple part I guess using key like
ok 👍 If we find time afterwards we can see if we can approach this using non blocking streams for Filesystem or something to contemplate splitting tags in separate files. |
|
Actually for filesystem another approach would work better: we read the first bytes of the file and stop before reading the value. The data format provided by this PR allows that. |
005b90b to
74296cd
Compare
74296cd to
0c32b84
Compare
|
Good news: I managed to remove over fetching for both Redis and Filesystem adapters! Once again, the solution is completely different than we originally anticipated :) |
b666d2e to
b029f52
Compare
b029f52 to
33f31ca
Compare
…stractTagAwareAdapter
33f31ca to
5a4a30c
Compare
…e when using AbstractTagAwareAdapter (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | #33924 | License | MIT | Doc PR | - This is the final touch in my series of PR that fixes the linked issue. Remarkably, the solutions I implemented for this issue are completely different than the one I described there. Fortunately, the issues themselves were correctly identified. Plannification of implementation is gambling :) /cc @andrerom Commits ------- 5a4a30c [Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter
…ssion (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Cache] add DeflateMarshaller - remove phpredis compression | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - phpredis compression doesn't play well with lua scripting as used in #33939 Let's remove it and provide a `DeflateMarshaller` instead. Ppl can use it via decoration: ```yaml services: Symfony\Component\Cache\Marshaller\DeflateMarshaller: decorates: cache.default_marshaller arguments: ['@symfony\Component\Cache\Marshaller\DeflateMarshaller.inner'] ``` It's not enabled by default because that might break pools that are shared between different apps. /cc @andrerom FYI Commits ------- 452c863 [Cache] add DeflateMarshaller - remove phpredis compression
This is the final touch in my series of PR that fixes the linked issue.
Remarkably, the solutions I implemented for this issue are completely different than the one I described there. Fortunately, the issues themselves were correctly identified.
Plannification of implementation is gambling :)
/cc @andrerom