Optimizing getting parents by given type #9217
Conversation
|
You shouldn't use underscores for private/protected members. Do you have some numbers? How much time does this save? |
|
@adragus-inviqa |
|
Yeah, they shouldn't be using underscores. Must be some legacy code. But I'll wait for them to confirm. Whenever you optimise stuff, you need to come up with numbers, to actually prove that your change is needed. Is it saving a fraction of a second or 2 minutes? Every refactoring can introduce bugs, so your PR needs to be justified. With numbers. |
|
@adragus-inviqa i agree. can you give some idea to check the improvement. |
| @@ -8,6 +8,11 @@ | |||
| class ClassReader implements ClassReaderInterface | |||
| { | |||
| /** | |||
| * @var array | |||
| */ | |||
| protected $_classNameParents; | |||
There was a problem hiding this comment.
Please avoid using underscores in property names, it is a legacy standard.
There is no need for this property to be protected as we do not encourage inheritance-based APIs
There was a problem hiding this comment.
@ishakhsuvarov so can i use private $classNameParents?
There was a problem hiding this comment.
Yes, that is what I meant :)
| } | ||
| array_unshift($result, $parentClass); | ||
| if (isset($this->_classNameParents[$className])) { | ||
| return $this->_classNameParents[$className]; | ||
| } else { |
There was a problem hiding this comment.
Looks like there is no more need for the else
| @@ -120,6 +120,7 @@ public function getInstanceType($instanceName) | |||
| */ | |||
| public function getPreference($type) | |||
| { | |||
| $type = ltrim($type, '\\'); | |||
There was a problem hiding this comment.
This does not look like a part of in-object cache, which is the purpose of this PR
There was a problem hiding this comment.
I think OP has mixed up branches a bit: #9189 (comment)
| @@ -53,7 +53,6 @@ public function __construct(FactoryInterface $factory, ConfigInterface $config, | |||
| */ | |||
| public function create($type, array $arguments = []) | |||
| { | |||
| $type = ltrim($type, '\\'); | |||
| return $this->_factory->create($this->_config->getPreference($type), $arguments); | |||
There was a problem hiding this comment.
This does not look like a part of in-object cache, which is the purpose of this PR
There was a problem hiding this comment.
@ishakhsuvarov can i remove these changes and sent only changes related to this PR?
There was a problem hiding this comment.
Yes, that would be perfect.
| } | ||
| array_unshift($result, $parentClass); | ||
| if (isset($this->_classNameParents[$className])) { | ||
| return $this->_classNameParents[$className]; |
There was a problem hiding this comment.
Please consider updating tests to verify this case.
|
@sivajik34 Thank you for the PR. Idea looks good overall, however please check review comments. |
|
Hi @sivajik34 |
|
@ishakhsuvarov if its not that much helpful we can close it.no issues. |
|
@sivajik34 |
We can optimize type's parents info by storing into an array,so that next time onwards we can return this array with out calculating whole logic.