I found this use case legit and right now I don't see an easy way to achieve this perf optim (quite huge in this project)

It used to have a cache for this method, it has been removed in#17140. But the removal of this cache introduced a huge performance penalty (the one you suffered).
Maybe can it be a good idea to reintroduce this cache but make it disabled by default?

The reason why we remove the cache would be still valid, so that this option would be a foot gun I guess.
Instead, what about adding an interface that would provide a method that would return a cache key? Then if it is implemented, we use the cache.

@welcoMattic, can you share your profile publicly?

@nicolas-grekas I think it won't help, the previous cache allowed to skip the whole loop. Here we still need to loop and to callsupports*.supports* is usually lightweight. Calling a method to get a cache key instead will have no benefit.

Another proposal: introduce aNonCacheableSupportsReturnInterface interface and use the cache if no normalizer implements it.

we talked with@dunglas and might have found an idea:
we could add an new interface with agetProvidedTypes
this method would return an array keyed by PHP-type/class, valued by a bitfield of "vary" consts: DATA | CONTEXT.
this would tell the outside what the "supports*" methods vary on, so that good enough cache/optimizations strategies could be implemented with that info.

is the vary strategy worth it? It's nice.. but feels a bit over-engineered =/ Eventually theSerializer is simple chain implementation, which is fine to me.

Exposing here is the easiest no? Or introduce a new abstraction layer?

While at it;Serializer::denormalizeObject() can be inlined i guess.

@ro0NL exposing only fix partially the problem, from my experience, this performance problem occurs often when you use the serializer intensively (yes I've API Platform in mind).

Unlike exposing, fixing this problem would benefit to every users without requiring custom developments.

What about a newAbstractKnownObjectNormalizer 😱 or so. It would havefinal supports method and exposes it supported object types.

So kinda following the same path proposed by@nicolas-grekas, but perhaps easier to implement.

I'd be interested in knowing if a simple approach like#24228 could be enough?
Can any of you measure any significant improvement with it?

Alsoextends Serializer might not be the right trade-off. If$normalizerClassName = get_class($normalizer); is your convention, perhaps implement the required interfaces directly? Most would be simple method forwarding anyway.

Yet i understand you prefer this extension point, in favor to copy/pastingSerializer::__construct or so. A new abstraction layer effectively is the same as this PR, probably not worth it.

@dunglas hmm, sorry it's a private application. But I can make a new simple Sf app with as many normalizers.

Thanks all for discussing about this performance issue. I'll try@nicolas-grekas PR soon.

Hi

I'd be interested in knowing if a simple approach like#24228 could be enough?

In our case it's not enough. It's logical because our normalizers don't have this cache layer. The only way to make it faster in our case is to allow us to override thegetDenormalizer /getNormalizer method.

@welcoMattic@lyrixx@dunglas any OK/KO about#24206 (comment)?

Status: waiting feedback

we talked with@dunglas and might have found an idea:
we could add an new interface with a getProvidedTypes
this method would return an array keyed by PHP-type/class, valued by a bitfield of "vary" consts: DATA | CONTEXT.
this would tell the outside what the "supports*" methods vary on, so that good enough cache/optimizations strategies could be implemented with that info.

I'm not sure to follow you. Because how would you generated a cache-id when the normalizer vary on the the data? If it's an object it's quite simple (spl_object_hash) but if it's an array ...

how would you generated a cache-id when the normalizer vary on the the data

We would not. That'd then be the responsibility of the normalizer class. But in you example, you don't vary by "data", but only by "type(data)", which would be covered by the interface so caching would happen.

Ok, so your cache layer will only cache something based on the context ?

Almost like http vary yes, with 3 vary-by possible: type(data) and/or context yes, state inside data cannot be considered from this layer

Oh, but there would still be a benefit when the data is considered: if you say "for this type, it varies by data", then the cache layer will still be able to skip the normalizer for any other non specified type.

It looks good to me. We will try to update this PR as soon as possible.

Thanks for all your ideas, I'll see with@lyrixx to update the PR before the feature freeze

I think it's too late for 3.4, adding this vary-by cache should be done for 4.1 now.
I'm closing this PR as we figured out a way to not open these methods, which is always the solution we prefer for maintenance purpose, and replacing it by an issue with a description of what we discussed.

See#24436