Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetching normalizers not scalable / slow. #29387

Closed
ndobromirov opened this issue Nov 30, 2018 · 8 comments
Closed

Fetching normalizers not scalable / slow. #29387

ndobromirov opened this issue Nov 30, 2018 · 8 comments

Comments

@ndobromirov
Copy link

ndobromirov commented Nov 30, 2018

Description
Runtime cache on the serailizers / normalizers component system.

Example
We are using symfony/serializer as a dependency in Drupal. There is this module that seems to abuse this piece of code. and is causing very big delays (like 10-20% or request time) just finding the right normalizers.

Furhter details can be seen in Drupal's issue queue for jsonapi module: here

Direction
I've managed to drop overhead like 15-20 times with the following pseudo code.
I know it's not production ready but for you can get the idea:

Symfony\Component\Serializer\Serializer::getNormalizer($data, $format, array $context)

/**
     * Returns a matching normalizer.
     *
     * @param mixed  $data    Data to get the serializer for
     * @param string $format  Format name, present to give the option to normalizers to act differently based on formats
     * @param array  $context Options available to the normalizer
     *
     * @return NormalizerInterface|null
     */
    private function getNormalizer($data, $format, array $context)
    {
        static $cache = [];

        $cid = get_class($data) . ':' . $format;
        if (isset($cache[$cid])) {
          return $cache[$cid];
        }

        foreach ($this->normalizers as $normalizer) {
            if ($normalizer instanceof NormalizerInterface && $normalizer->supportsNormalization($data, $format, $context)) {
                return $cache[$cid] = $normalizer;
            }
        }
    }

Additional thoughts

  • This can be moved to an internal property to be managed along-side the list of normalizers. Any time a normalizer list is mutated - reinitialize the property.
  • At the moment this is not handling native types well (ints, strings) likely this overhead can be further reduced.
  • Maybe just expose the getting of normalizers from private to protected, so this kind of optimizations can be implemented outside from here.
@bendavies
Copy link
Contributor

bendavies commented Nov 30, 2018

this was fixed in various PRs this year:
#27049
#27105

You need to up your dependencies for a version including these changes.

@jibran
Copy link

jibran commented Nov 30, 2018

Unfortunately, Drupal can't update its dependencies until Drupal 9 is released in 2020.

@mdeboer
Copy link
Contributor

mdeboer commented Dec 2, 2018

Unfortunately, Drupal can't update its dependencies until Drupal 9 is released in 2020.

Well that's too bad but how is this a Symfony issue? You can't fix what's already been fixed (see
#29387 (comment)).

@xabbuh
Copy link
Member

xabbuh commented Dec 3, 2018

closing here as these changes are already available

@xabbuh xabbuh closed this as completed Dec 3, 2018
@ndobromirov
Copy link
Author

Let's change the question.

If there is a PR for a back-port of this for 3.4. Is it going to be committed at any point?

@xabbuh
Copy link
Member

xabbuh commented Dec 10, 2018

That wouldn't be a bug fix. So no that wouldn't be happening.

@ndobromirov
Copy link
Author

ndobromirov commented Dec 11, 2018

Ok 😢.
So only way is to chase Drupal core maintainers for this. 🎉

For anyone that is interested this is the issue there: https://www.drupal.org/project/drupal/issues/3017291

@wimleers
Copy link

To clarify:

  • Symfony 4 broke BC. Therefore Drupal 8 cannot update to Symfony 4, or we'd force those BC breaks on Drupal 8 users too.
  • Hence only Drupal 9 can update to Symfony 4.

It is what it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants