[Serializer] ensure ChainEncoder/ChainDecoder properly consider the context #43231
Conversation
|
We must be very careful when touching this part. Removing such cache can have a dramatic performance impact, as it's on the hot path. We need to profile and to double-check that it doesn't make complex apps very slow. We'll probably have to do something like this: #27105 cc @nicolas-grekas. |
|
I think that we should just replicate the logic introduced for normalizers in #27105. Cache when not dynamic, and don't cache when it is. |
|
Another implementation of the chain could be added, it would be simpler IMHO, since it's an edge case |
Dynamic would be a context that is not just an empty array? |
|
I readded caching arrays but utilize them only if no dynamic context is given. |
|
I propose checking for |
|
Closing in favor of #47304 |
…erface` and `ContextAwareDecoderInterface` (nicolas-grekas) This PR was merged into the 6.1 branch. Discussion ---------- [Serializer] Revert deprecation of `ContextAwareEncoderInterface` and `ContextAwareDecoderInterface` | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - While reviewing #43231, I figured out that the correct fix for #38270 was to make `ChainEncoder` properly consider `ContextAwareEncoderInterface` when deciding to cache or not. Since this interface is useful to discriminate the cache/no-cache situations, we have to undeprecate it (from #43982.) /cc `@Guite` and `@mtarld` FYI Commits ------- 9ac7fb1 [Serializer] Revert deprecation of `ContextAwareEncoderInterface` and `ContextAwareDecoderInterface`
…n ChainEncoder/ChainDecoder (Guite) This PR was merged into the 4.4 branch. Discussion ---------- [Serializer] Fix caching context-aware encoders/decoders in ChainEncoder/ChainDecoder | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #38270 | License | MIT | Doc PR | - As proposed in #43231 (comment) Will require #47150 on 6.1 until a proper solution is implemented, hopefully in 6.2. Commits ------- 5902380 [Serializer] Fix caching context-aware encoders/decoders in ChainEncoder/ChainDecoder
This PR removes the "caching" of encoder/decoder instances in a chain so they are re-evaluated for each single call.
The corresponding tests are extended to perform multiple calls with different context arrays. Also they now check not only if encoding/decoding is supported, but also that the responsible encoder/decoder instance is the correct (expected) one.
/cc @xabbuh