[Cache] Inline some hot function calls #26984
Conversation
773df1b to
d4b13ff
Compare
There was a problem hiding this comment.
There are still several count, 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?
use is_array;
use is_object;
// etc|
shouldn't we have tests covering these edge case behaviors to avoid recreating them ? |
|
Also, what are these edge cases ? |
|
@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. |
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. |
d4b13ff to
52b4bfc
Compare
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.