Skip to content

[Serializer] Add local cache to normalizers #24228

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

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Sep 15, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24206
License MIT
Doc PR -

Should help making the Serializer a bit faster.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Sep 15, 2017
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ inline Serializer::denormalizeObject()?


/**
* {@inheritdoc}
*/
public function supportsNormalization($data, $format = null)
{
return parent::supportsNormalization($data, $format) && $this->supports(get_class($data));
return parent::supportsNormalization($data, $format) && (isset($this->cache[$type = \get_class($data)]) ? $this->cache[$type] : $this->cache[$type] = $this->supports($type));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applies to supportsDenormalization as well, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about private static $cache + private static supports() + inline isGetMethod

/**
* {@inheritdoc}
*/
public function supportsNormalization($data, $format = null)
{
return parent::supportsNormalization($data, $format) && $this->supports(get_class($data));
return parent::supportsNormalization($data, $format) && (isset($this->cache[$type = \get_class($data)]) ? $this->cache[$type] : $this->cache[$type] = $this->supports($type));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Sep 16, 2017

  • inline Serializer::denormalizeObject()

Done.
For other comments: static state won't provide anything is practice, and are not better in terms of design.

@@ -66,7 +66,7 @@ public function denormalize($data, $class, $format = null, array $context = arra
*/
public function supportsDenormalization($data, $type, $format = null/*, array $context = array()*/)
{
$context = func_num_args() > 3 ? func_get_arg(3) : array();
$context = \func_num_args() > 3 ? func_get_arg(3) : array();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas nicolas-grekas force-pushed the ser-cache branch 2 times, most recently from 0fdab2f to 23f485e Compare September 16, 2017 08:34
@nicolas-grekas nicolas-grekas changed the title [Seriaizer] Add local cache to normalizers [Serializer] Add local cache to normalizers Sep 16, 2017
@ogizanagi
Copy link
Contributor

Won't it make sense to backport what could be on lower branches?

@nicolas-grekas
Copy link
Member Author

Won't it make sense to backport what could be on lower branches?

AFAIK, we merge perf improvements as new feats usually (even if we diverged sometime from this rule, but we shouldn't anymore unless catastrophic. Not sure it's the case here?)

@nicolas-grekas nicolas-grekas merged commit b0c5cf0 into symfony:3.4 Sep 20, 2017
nicolas-grekas added a commit that referenced this pull request Sep 20, 2017
…kas)

This PR was merged into the 3.4 branch.

Discussion
----------

[Serializer] Add local cache to normalizers

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24206
| License       | MIT
| Doc PR        | -

Should help making the Serializer a bit faster.

Commits
-------

b0c5cf0 [Serializer] Add local cache to normalizers
@nicolas-grekas nicolas-grekas deleted the ser-cache branch September 20, 2017 14:33
nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Oct 1, 2017
…rs (nicolas-grekas)"

This reverts commit aad62c4, reversing
changes made to 779e8ed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants