avoid multiple reads/writes for inherited properties in hydrator class #85
Conversation
| static function (ReflectionProperty $property) : bool { | ||
| return ! $property->isStatic(); | ||
| static function (ReflectionProperty $property) use ($class) : bool { | ||
| return ! ($property->isStatic() || $property->class !== $class->name); |
There was a problem hiding this comment.
Please avoid using public properties from ext-reflection: they are very unreliable.
Besides that: could you expand https://github.com/Ocramius/GeneratedHydrator/blob/ad230228c71c32279af94ed1484b907cd403886f/tests/GeneratedHydratorTest/CodeGenerator/Visitor/HydratorMethodsVisitorTest.php to verify this change?
There was a problem hiding this comment.
Refactored property access to use functions instead. Thanks for the heads up 👍
I looked into the testing before but didn't see a nice way to test this. The easiest way would be to inspect the property maps of the HydratorMethodsVisitor. They are currently private and there are no getters for them. Would you consider weakening the isolation for them to enable easier testing or should I look into other possibilities?
There was a problem hiding this comment.
I looked into the testing before but didn't see a nice way to test this.
Verifying the generated code is the correct way here. I can't remember how I initially tested this, but I'd suggest expanding https://github.com/Ocramius/GeneratedHydrator/blob/ad230228c71c32279af94ed1484b907cd403886f/tests/GeneratedHydratorTest/CodeGenerator/Visitor/HydratorMethodsVisitorTest.php to include an "expected generated method body" (can compare AST of produced nodes against AST of a parsed expected block)
There was a problem hiding this comment.
@Ocramius added a test to verify the change. It's quite ugly and will break if the parser changes structure of the AST but I guess that's a trade off we are willing to make.
|
Thanks for the quick turn around time @Ocramius. Happy to run the benchmark before and after. How do you normally share the results as part of a PR? |
|
@pvonr a run before/after (of phpbench) should be sufficient. The tool also allows comparing benchmarks 👍 Results could just be a comment added here. |
|
|
| $classCode = 'class '.$className.' { private $bar; private $baz; protected $tab; ' | ||
| .'protected $tar; }'; | ||
|
|
||
| eval($classCode); |
There was a problem hiding this comment.
Hmm, interesting, I thought I made this library kinda side-effect free :|
Guess I'll have more work to do.
| 'values', | ||
| function (Assign $assign) { | ||
| return $assign->var->dim->value; | ||
| } |
There was a problem hiding this comment.
Not important, but \t is generally to be avoided. Can fix on merge from my end though.
There was a problem hiding this comment.
ah sorry. didn't switch to spaces (working with tabs internally).
There was a problem hiding this comment.
No worries: that is entirely my preference, and I just forgot to add PHPCS to the repo ;-)
| /** | ||
| * @dataProvider propertyAstProvider | ||
| */ | ||
| public function testPropertyCodeGeneration(string $className, Class_ $classNode, array $properties) : void |
There was a problem hiding this comment.
Hmm, this code is indeed complex and fragile. I applaud your efforts!
Still, I was wondering if we could write something like this:
$expected = <<<'PHP'
public function hydrate(...)
{
// ...
}
PHP;
$actual = <<<'PHP'
public function hydrate(...)
{
// ...
}
PHP;
$expectedAst = $parser->parse($expected);
$actualAst = $parser->parse($actual);
self::assertEquals(
$expected->some->path->to->the->method->node,
$actual->some->path->to->the->method->node
);Is such a test feasible, in your opinion?
Also feasible: using the PHP-Parser pretty-printer to compare the AST via string rather than AST.
There was a problem hiding this comment.
Was also thinking about using the pretty-printer and comparing strings :) though you would not like it ;)
Not sure whether assertEquals really works on a deeply nested array with different objects.. does it?
|
@pvonr This is weird, I don't reproduce the behaviour you describe, could you please show an example of the generated code along with the hydrated classes causing the issue where it attempt to hydrate the same property twice ? |
|
@pounard an example is in the test ;) |
|
This is what I get when generating the Hydrator for last class in the test... <?php
namespace GeneratedHydratorGeneratedClass\__PM__\Foo5d00ef39eeb48169186538;
class YToxOntzOjc6ImZhY3RvcnkiO3M6NDE6IkdlbmVyYXRlZEh5ZHJhdG9yXEZhY3RvcnlcSHlkcmF0b3JGYWN0b3J5Ijt9
{
private $hydrateCallbacks = array(), $extractCallbacks = array();
function __construct()
{
$this->hydrateCallbacks[] = \Closure::bind(static function ($object, $values) {
if (isset($values['bar']) || $object->bar !== null && \array_key_exists('bar', $values)) {
$object->bar = $values['bar'];
}
if (isset($values['baz']) || $object->baz !== null && \array_key_exists('baz', $values)) {
$object->baz = $values['baz'];
}
if (isset($values['tab']) || $object->tab !== null && \array_key_exists('tab', $values)) {
$object->tab = $values['tab'];
}
if (isset($values['tar']) || $object->tar !== null && \array_key_exists('tar', $values)) {
$object->tar = $values['tar'];
}
if (isset($values['tab']) || $object->tab !== null && \array_key_exists('tab', $values)) {
$object->tab = $values['tab'];
}
if (isset($values['tar']) || $object->tar !== null && \array_key_exists('tar', $values)) {
$object->tar = $values['tar'];
}
if (isset($values['tab']) || $object->tab !== null && \array_key_exists('tab', $values)) {
$object->tab = $values['tab'];
}
if (isset($values['tar']) || $object->tar !== null && \array_key_exists('tar', $values)) {
$object->tar = $values['tar'];
}
}, null, 'Foo5d00ef39eea07326266986');
$this->extractCallbacks[] = \Closure::bind(static function ($object, &$values) {
$values['bar'] = $object->bar;
$values['baz'] = $object->baz;
$values['tab'] = $object->tab;
$values['tar'] = $object->tar;
$values['tab'] = $object->tab;
$values['tar'] = $object->tar;
$values['tab'] = $object->tab;
$values['tar'] = $object->tar;
}, null, 'Foo5d00ef39eea07326266986');
$this->hydrateCallbacks[] = \Closure::bind(static function ($object, $values) {
if (isset($values['fuz']) || $object->fuz !== null && \array_key_exists('fuz', $values)) {
$object->fuz = $values['fuz'];
}
if (isset($values['buz']) || $object->buz !== null && \array_key_exists('buz', $values)) {
$object->buz = $values['buz'];
}
if (isset($values['buz']) || $object->buz !== null && \array_key_exists('buz', $values)) {
$object->buz = $values['buz'];
}
}, null, 'Foo5d00ef39eea91595044410');
$this->extractCallbacks[] = \Closure::bind(static function ($object, &$values) {
$values['fuz'] = $object->fuz;
$values['buz'] = $object->buz;
$values['buz'] = $object->buz;
}, null, 'Foo5d00ef39eea91595044410');
$this->hydrateCallbacks[] = \Closure::bind(static function ($object, $values) {
if (isset($values['bis']) || $object->bis !== null && \array_key_exists('bis', $values)) {
$object->bis = $values['bis'];
}
}, null, 'Foo5d00ef39eeb48169186538');
$this->extractCallbacks[] = \Closure::bind(static function ($object, &$values) {
$values['bis'] = $object->bis;
}, null, 'Foo5d00ef39eeb48169186538');
}
function hydrate(array $data, $object)
{
$this->hydrateCallbacks[0]->__invoke($object, $data);
$this->hydrateCallbacks[1]->__invoke($object, $data);
$this->hydrateCallbacks[2]->__invoke($object, $data);
return $object;
}
function extract($object)
{
$ret = array();
$this->extractCallbacks[0]->__invoke($object, $ret);
$this->extractCallbacks[1]->__invoke($object, $ret);
$this->extractCallbacks[2]->__invoke($object, $ret);
return $ret;
}
} |
|
Hmm, looking at it closer, this entire array-access thing can also be elided :-\ Time to dig more into this lib, I guess |
|
I can't spend much more time on this but think we should move away from generating the code via AST and just have a simple template that we render out. Wondering what the use case was to implement code increments in the first place? |
Agree. Let's just finish this patch as-is though ;-)
There was no use-case: I initially built it this way because I was poking around with various mechanisms, especially to avoid In the end, I think that this would work if the AST were simpler, but PHP is a damn frikken nightmare from a language complexity. |
|
For more context, while it is true that I only publish code once I think it has reached a decent level of quality, this library is and was always a prototype to demonstrate that we can do things faster by misusing the engine. |
|
Haha, ok got it. We want to integrate the library to guarantee cache consistency for our internal database models during canary deploys. So I was looking for a fast solution. Thanks for pushing it out in the wild anyway! |
The reflection api returns public and protected properties of all super classes. With this commit we stop reading and writing those properties multiple times in the generated hydrator classes. This improves performance when hydrating hierarchical classes.