Reduce function calls in creation of Item classes #182
Conversation
| @@ -99,36 +99,33 @@ public function setItemClass($class) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function getItem() | |||
| public function getItem($arg1, $arg2) | |||
There was a problem hiding this comment.
The second arg should default to null, so it can be called with one arg without a notice beeing emitted
|
Did you profile this change? Does it really make the function faster? A function call in php is not that expensive IMO. The change to get ridd of func-get-args might indeed optimize it though |
|
well depends on what you define as expensive, it is more then other languages, but compared to loading files it is of course not so high. This is definitely micro optimization (see branch name), so I'm not saying this is a must. But given this is a caching library where you might consider using it as backend for Symfony Proxy (the one you use if you don't use Varnish) or in other micro frameworks, then every function call and file include counts in zend php (hhvm not so much) afaik, even if phpng might improve on this quite a bit. |
| * @param array $key | ||
| * @throws \InvalidArgumentException | ||
| */ | ||
| protected function setupKey($key) |
There was a problem hiding this comment.
note: given the doc, this being protected and only being called from one place it seemed natural to move it into setKey, but might be doc is wrong and it is meant for subclassing Item class, however that is still not main focus of this class rigth? there is an interface now.
|
Unsure what the CS failure is about. |
|
Moving/removing the setupKey method is another BC break from this PR, because people might have subclassed i and rely on it beeing overridde |
|
sub classing is not a extension point imho(interfaces are contracts, implementations are not), however fully I agree this PR should anyway be for 0.13 if accepted in current form given the changes to ItemInterface::setKey signature. |
|
The next release is going to break BC anyways- we're pretty close to finalizing the FIG standard, after which I'll be focused on making Stash compliant and then reach towards a 1.0 release. Feel free to break all the shit you need. |
|
:) |
|
@tedivm Assuming you plan a new release (ref discussion on the bundle), any of these changes that would be of interest? I'll either rebase and cleanup, or close this one. |
|
If you want to rebase and clean this up I'll merge it for the next release. |
With heavy use of Stash creation of Item classes & setKey shows up in profile data, so the change tries to reduce function calls in these. Suggested followup is executeGet().
|
done, the failing test is composer not managing to pull in packages. feel free to nitpick :) |
Reduce function calls in creation of Item classes
With heavy use of Stash creation of Item classes & setKey shows up
in profile data, so the change tries to reduce function calls in these.
Suggested followup is executeGet().
Note: Might need another pass on this tomorrow for CS / tests if this is otherwise accepted.
Note2: Contains interface change on Item to force array type on $key, felt like a easy way to avoid having to check.