Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Cache] Inline some hot function calls#26984
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
linaori left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are still severalcount,gettype,is_array,is_object calls that could benefit from the global namespace "fix" in the files you've touched. Would it be worth taking those along while at it, or are those potential merge conflicts? Would it be an idea to just import the whole function to avoid merge conflicts?
useis_array;useis_object;// etc
stof commentedApr 19, 2018
shouldn't we have tests covering these edge case behaviors to avoid recreating them ? |
stof commentedApr 19, 2018
Also, what are these edge cases ? |
stof commentedApr 19, 2018
@iltar the import won't work fine, as 3.4 still supports PHP 5.6, while these require 7.1. And as our CS are about using a backslash for classes in the global namespace, it makes sense to use the same rule for global functions. |
nicolas-grekas commentedApr 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
the remaining ones would actually not benefit from inlining because they are not on the hot path, so inlining or not would make no difference.
Before this PR, the item object fetched from the lowest adapter is directly saved in upper ones. |
This PR was merged into the 3.4 branch.Discussion----------[Cache] Inline some hot function calls| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -The Cache component is typically found in critical code paths, let's inline some calls.I didn't change all calls but only those in potential hot paths.Submitted against 3.4 to reduce future merge conflicts.The PR also embeds edge-case behavior fixes for `ChainAdapter`.Commits-------52b4bfc [Cache] Inline some hot function calls
…inodell)This PR was merged into the 4.4 branch.Discussion----------[Cache] Reword "max lifetime" to "default lifetime"The use of the word "maximum" here is not very clear; it's easy for somebody to mistakenly believe that this parameter limits the TTL of the higher caches to some maximum value. For example, if the second cache returns a hit with a TTL of 3600, then `$maxLifetime = 60` would mean the first cache should use a TTL of 60 since that's the "maximum lifetime". But looking at the code, that's not what happens - this parameter actually works as a **default** TTL for items without an expiry.I believe this possible confusion is why the variable was renamed in [#26984](symfony/symfony#26984) so let's update the documentation accordingly. Hopefully this will prevent others from making the same misunderstandings that I did :)Commits-------e1c8dc8 Reword "max lifetime" to "default lifetime"
The Cache component is typically found in critical code paths, let's inline some calls.
I didn't change all calls but only those in potential hot paths.
Submitted against 3.4 to reduce future merge conflicts.
The PR also embeds edge-case behavior fixes for
ChainAdapter.