Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] ensure ChainEncoder/ChainDecoder properly consider the context#43231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dunglas commentedSep 29, 2021
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 |
Uh oh!
There was an error while loading.Please reload this page.
dunglas commentedSep 29, 2021
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. |
lyrixx commentedSep 29, 2021
Another implementation of the chain could be added, it would be simpler IMHO, since it's an edge case |
Guite commentedOct 1, 2021
Dynamic would be a context that is not just an empty array? |
Guite commentedOct 1, 2021
I readded caching arrays but utilize them only if no dynamic context is given. |
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedAug 2, 2022
I propose checking for |
nicolas-grekas commentedAug 17, 2022
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` FYICommits-------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
Uh oh!
There was an error while loading.Please reload this page.
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