hydrators should not be proxy, now works gracefully with final classes #59

pounard · 2017-01-11T15:34:40Z

By not extending the original class, we can hydrate final classes. It makes the generate code simpler, potentially faster for complex classes to hydrate, and generation code a lot simpler.

It uses the Closure::bind() mecanism for all properties, no matter their visibility, but it creates a single closure for each parent class instead of a closure per method, which drastically reduces the number of potential function calls when hydrating objects with a complex hierarchy and a lot of private properties.

If also fixes a bug where when attempting to hydrate objects partially (without all properties within the array) notices were thrown, now they are not anymore!

It also fixes #35

And potentially #51 because Closures are ordered following the hierarchy: if a parent private property is hidden by a child class, the child will always get the private property, and not the parent.

It removes a few code for manipulating the AST, since it's not necessary anymore. Actually, we could probably remove ALL the AST manipulation dependency and just write a PHP template with the added Closures inside, which would make this code even faster and simpler.

pounard · 2017-01-11T16:51:48Z

It also adds tests for class hierarchy and values, it was untested.

flip111 · 2017-01-11T17:39:46Z

can you do some benchmarks on php 5 & 7 for this PR ?

pounard · 2017-01-11T18:43:39Z

I did with PHP7, with no private properties, I have a 40% penalty, with 2 private properties across a parent and a child class, I get 20%, so as an educated guess I would say that reaching 4 or 6 private properties, I'm on-par in term of performances, but I'll do some more tomorrow and give you the methodology and results.

Please check issue #60 it explains it a bit.

If I'm not mistaken, this API doesn't work with PHP5 due to strict types usage.

flip111 · 2017-01-11T18:50:19Z

i was using this library with php5 before and it was working then

not sure what you mean with 40% penalty and on-par performance. i mean to compare this pr with master. i will look at issue 60 later

pounard · 2017-01-11T18:57:10Z

40% penalty means that in ideal conditions (which never happens in real life environments) I am 40% slower, but I'm quite sure that with real life objects (much more complex data structures) there would be no significant difference.

malukenho · 2017-01-11T20:05:53Z

src/GeneratedHydrator/ClassGenerator/HydratorGenerator.php

-        $ast = $builder->fromReflection($originalClass);
+        $ast = array($class);
+        if ($namespace = $originalClass->getNamespaceName()) {
+            $ast = array(new Namespace_(new Name(explode('\\', $namespace)), $ast));


short array syntax

Same note as above applies, don't nitpick, please, saying it once would have been enough for me to search and fix all of them :)

malukenho · 2017-01-11T20:06:07Z

src/GeneratedHydrator/ClassGenerator/HydratorGenerator.php


-        $ast = $builder->fromReflection($originalClass);
+        $ast = array($class);


short array syntax

Most of @Ocramius' code do not use the short array syntax, I did this on purpose to avoid frustrating him. It's actually never wrote with short array syntax in the whole library. I'm just keeping consistency with the existing codebase.

malukenho · 2017-01-11T20:06:53Z

src/GeneratedHydrator/CodeGenerator/Visitor/HydratorMethodsVisitor.php

-     * @var PropertyAccessor[]
-     */
-    private $propertyWriters = array();
+    private $classPropertyMap = array();


short array syntax

Please, dont nitpick.

If a comment is not relevant, simply don't worry about it and defer to merger. I'll probably apply CS changes on merge anyway.

malukenho · 2017-01-11T20:07:32Z

src/GeneratedHydrator/ClassGenerator/HydratorGenerator.php

@@ -35,6 +37,7 @@
 * {@inheritDoc}
 *
 * @author Marco Pivetta <ocramius@gmail.com>
+ * @author Pierre Rineau <pierre.rineau@makina-corpus.com>


Can be removed as it was only changes on existent files

Indeed, wrong file.

malukenho · 2017-01-11T20:07:50Z

src/GeneratedHydrator/CodeGenerator/Visitor/HydratorMethodsVisitor.php

@@ -34,6 +36,7 @@
 * Replaces methods `__construct`, `hydrate` and `extract` in the classes of the given AST
 *
 * @author Marco Pivetta <ocramius@gmail.com>
+ * @author Pierre Rineau <pierre.rineau@processus.org>


same as above

I actually almost rewrote fully the file, so my name is relevant here. But if the original author don't want my name, I'll remove it, but I'll answer only to him.

malukenho · 2017-01-11T20:08:34Z

src/GeneratedHydrator/CodeGenerator/Visitor/HydratorMethodsVisitor.php

            return null;
        }

+        $node->stmts[] = new Property(Class_::MODIFIER_PRIVATE, array(


short array syntax

Once again, just keeping consistency with the rest of the code. I am on your side with this, I'd be please to use the short array syntax, but once again, I prefer to have @Ocramius opinion before changing any code convention this library uses.

malukenho · 2017-01-11T20:11:36Z

tests/GeneratedHydratorTestAsset/ClassWithPrivatePropertiesAndParent.php

+    /**
+     * @param string $property0
+     */
+    public function setProperty0($property0)


It's missing parameter type or it's not needed?

This was written from a copy past of another test class, which doesn't carry any return type. It's definitely not needed. I'm OK to write them, but for consistency all test classes should probably reviewed and fixed too. Not sure this is the scope of the issue, once again I'd like to have @Ocramius' opinion about this.

pounard · 2017-01-11T21:37:59Z

@malukenho I mostly agree with your requested changes, but some do change the codestyle of the library, so I really want to wait for @Ocramius opinion, it's his call to make.

Ocramius · 2017-01-12T15:46:31Z

with real life objects (much more complex data structures) there would be no significant difference.

The library was designed for inclusion in ORM v3, so any bit of performance we can squeeze out of it should be considered. If the hydrator codegen without inheritance is something that fixes handling of final and classes already implementing the hydrator interface, then this should probably be considered a fallback, rather than a replacement solution.

Ocramius · 2017-01-12T15:47:30Z

can you do some benchmarks on php 5 & 7 for this PR ?

Indeed, we need to add benchmarks (phpbench-based) to the library.
PHP 5 is no longer the target audience though.

pounard · 2017-01-12T16:58:09Z

The library was designed for inclusion in ORM v3, so any bit of performance we can squeeze out of it should be considered

It surely needs more benchmarks to tell, but I'd probably say that the performance impact is mostly due to isset() calls, and probably not by the fact there's closures. I'll test that.

Ocramius · 2017-01-12T16:59:42Z

I'll test that.

The amount of opcodes being generated usually gives you a good idea of where the slowness is.

pounard · 2017-01-12T17:08:56Z

The library was designed for inclusion in ORM v3

What are the requirements for ORM v3 regarding this library ?

If the hydrator codegen without inheritance is something that fixes handling of final and classes already implementing the hydrator interface, then this should probably be considered a fallback, rather than a replacement solution.

Not even sure why inheritance here is even wanted to start with, I think that it should get rid of, there is no need at all, and it replaces your objects by proxies and it actually makes no sense to me, except if it is required by ORM v3.

I think it make the overall code of this library much harder to read and maintain since you have to parse the class AST, replace its methods, add some others, change its name, add the extension, etc etc etc... Generating much more simple hydrators could also make the AST completly useless in the end.

And I often read your blog posts, and watch from time to time your sessions, isn't composition and single responsibility supposed to be better than inheritance ? :) My guess here, especially in this case, inheritance doesn't serve any purpose except making the whole much more complex.

Could you elaborate why this should be a fallback instead of being the common case?

Could you also elaborate about why it's a good thing to keep a very complex generation mechanism while it could be very much simpler and serve the exact same features?

I guess you have a roadmap I don't know anything about, but right now I see plenty of code that could be overly simplified and still be on-par in term of features and performance.

Thank you for your time and answers, looking forward to read your explanations!

pounard · 2017-01-12T17:11:10Z

The amount of opcodes being generated usually gives you a good idea of where the slowness is.

I'm doing it sometime, but nothing worth real figures, especially that opcode is itself an high-level IR and all operations do not cost the same, so less doesn't really mean faster. Except if you know pretty well how the zend internals are working, I guess it dangerous to analyse code performance only through this.

Ocramius · 2017-01-12T17:22:14Z

What are the requirements for ORM v3 regarding this library ?

Hydration in ORM has a massive performance impact, specifically method call time (reflection). If we can reduce that time in any way by providing an abstraction that does it for us, then the abstraction should always strive for best performance there.

Not even sure why inheritance here is even wanted to start with, I think that it should get rid of, there is no need at all, and it replaces your objects by proxies and it actually makes no sense to me, except if it is required by ORM v3.

The reason why inheritance was that any scope change would need a closure method call (expensive). Another possible solution is to return a closure that is already bound to the correct scope, and getting rid of the hydrator interface overall.

I think it make the overall code of this library much harder to read and maintain since you have to parse the class AST, replace its methods, add some others, change its name, add the extension, etc etc etc... Generating much more simple hydrators could also make the AST completly useless in the end.

Agree - if we can get rid of it, yet retain the same performance, then that's a very good thing.

And I often read your blog posts, and watch from time to time your sessions, isn't composition and single responsibility supposed to be better than inheritance ? :) My guess here, especially in this case, inheritance doesn't serve any purpose except making the whole much more complex.

As mentioned above, inheritance is used here as a hack to get rid of a single method call, yet retaining compat with the hydrator interfaces.

Could you elaborate why this should be a fallback instead of being the common case?

If the new approach cannot provide the same performance as the previous one, then it should only be used when a final class is involved, or when a class implementing hydrate or extract is involved, or a class with a final constructor.

Ocramius · 2017-01-12T17:24:43Z

Could you also elaborate about why it's a good thing to keep a very complex generation mechanism while it could be very much simpler and serve the exact same features?

The only feature provided by this library is speed. If that is not the absolute focus of the consumer of the library, then a simplistic, manually-written hydrator is sufficient in most scenarios.

I guess you have a roadmap I don't know anything about, but right now I see plenty of code that could be overly simplified and still be on-par in term of features and performance.

No, sorry, I don't have a roadmap for most OSS stuff, since I really work on it when I got time.
The on-par needs to be validated by a proper test suite there, which we should add separately, before eventually merging this patch.

Ocramius · 2017-01-12T17:25:39Z

I'm doing it sometime, but nothing worth real figures, especially that opcode is itself an high-level IR and all operations do not cost the same, so less doesn't really mean faster. Except if you know pretty well how the zend internals are working, I guess it dangerous to analyse code performance only through this.

Yes, but at a quick glance you see what is going on :-)

pounard · 2017-01-12T17:39:08Z

Hydration in ORM has a massive performance impact, specifically method call time (reflection). If we can reduce that time in any way by providing an abstraction that does it for us, then the abstraction should always strive for best performance there.

Definitely agree.

The reason why inheritance was that any scope change would need a closure method call (expensive).

Is the closure method call that expensive ? Looking at your code anyway, it does it as soon as you have private properties, you manually bind the object scope onto the closure because the current inherited scope cannot deal with parent privates, so in the end the negative impact of the closure is still here.

Most tutorials for Doctrine (so I guess would be the same for the future ORM v3) will tell you to write your entities properties as private, so I guess this call is somehow no-avoidable.

Another possible solution is to return a closure that is already bound to the correct scope, and getting rid of the hydrator interface overall.

True, but in that case this library wouldn't have any purpose anymore right ?

Out of the scope of ORM v3, I'm using it to dynamically hydrate objects from arbitrary SQL queries in a custom DBAL, and making it a bit more flexible is what I do need at this point. I guess that in the future, people that will use ORM v3 will also need that flexibility: partial fetches (hence the isset()) entities inheritance and making them final are all very valid use-cases!

As mentioned above, inheritance is used here as a hack to get rid of a single method call, yet retaining compat with the hydrator interfaces.

I'm quite sure a single method call won't hurt, especially when you consider the fact that as soon as you have private properties, you'll have that call in the end! It's already so much faster than other hydrators, and I my guess is as soon as you will want to add features or fix bugs, you'll make it slower, I think that having the flexibility right now might prevent tons of future issues.

If the new approach cannot provide the same performance as the previous one, then it should only be used when a final class is involved, or when a class implementing hydrate or extract is involved, or a class with a final constructor.

I'm quite sure that what costs the most are the isset() calls, they do hashmap lookups in the data array, need to be tested, but I'm quite sure my approach might be faster than the original one as soon as hydrated objects have private properties. In my code, the number of function calls is 1, in your, it is 0 to n where n is the number of private properties!

The only feature provided by this library is speed

Agree, and I like it, but I need to it to not yell when I have missing properties in the array and I need it to be able to use final classes :) Even with my patch, still more than 10 times faster than Zend's faster one!

No, sorry, I don't have a roadmap for most OSS stuff, since I really work on it when I got time

No worries, I understand, we all live in the same world I guess!

pounard · 2017-01-13T15:03:21Z

God I hate github, there is no way to re-attach the PR to the right commit.

pounard · 2017-01-13T15:12:12Z

Sehr gut!

…tion code

…he generator crash

… foreach() on callbacks

…thod

Ocramius · 2017-01-13T16:34:45Z

I was going to clean up some details but then realised that I have to first:

move away from hydrator classes
extract the code generator into two classes:
- extractorFactoryGenerator
- hydratorFactoryGenerator
  Both would return a reference to a callable that will instantiate the hydrator or extractor. Still need to check how to rewrite caching for that.
remove any array access that is not needed from inside the hydrator

Therefore, merging this, then moving this to milestone 3.0

pounard · 2017-01-13T16:38:37Z

By doing that you would also not need the AST dependency anymore. Hum the actual code itself could be rewritten without.

Thank you very much for merging this!

flip111 · 2017-01-13T20:12:49Z

@pounard congratulations on this PR !

flip111 · 2017-01-13T21:20:53Z

maybe update readme performance numbers ?

… more clarity

…be removed)

…nheritance' Cleanups for #59

This was referenced Jan 11, 2017

Optional hydration #16

Open

Private members from the same class - grouping #10

Open

GeneratedHydrator Mixes up private properties #51

Open

Hydrators fails on objects that include hydrate / extract methods #35

Closed

pounard force-pushed the master branch from 6bf14c3 to 26b15a2 Compare January 11, 2017 16:41

pounard mentioned this pull request Jan 11, 2017

Suggested test on the homepage is not fair #60

Closed

malukenho requested changes Jan 11, 2017

View reviewed changes

malukenho assigned Ocramius Jan 11, 2017

malukenho added bug enhancement labels Jan 11, 2017

malukenho added this to the 2.0.1 milestone Jan 11, 2017

pounard mentioned this pull request Jan 12, 2017

Code style changes #61

Closed

Ocramius modified the milestones: 2.1.0, 2.0.1 Jan 12, 2017

pounard force-pushed the master branch from 460a04c to 648215d Compare January 13, 2017 15:02

pounard reopened this Jan 13, 2017

pounard added 6 commits January 13, 2017 16:12

hydrators should not be proxy, overly simplifies generated and genera…

0cbe0a5

…tion code

fixes phpcs error

0b8beef

a untested use case with class parenting had a typo error that made t…

9074907

…he generator crash

restored public against hidden properties differenciation, got rid of…

8a72102

… foreach() on callbacks

phpcs fixes

6784494

added missing isset() on public properties assignment in hydrate() me…

b9ef284

…thod

Ocramius merged commit 725e765 into Ocramius:master Jan 13, 2017

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 short array syntax

eccb9b3

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 removed unused imports

f360a17

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 removed unused property

1940cfb

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 rewrote HydratorMethodsVisitor to have as few paths as possible

f33bc5f

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 s/recursiveFindNonStaticProperties/findAllInstanceProperties/ for…

eebdd34

… more clarity

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 Removed unused imports

dbc55a5

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 adding TODO about current state of the visitor (which needs to …

b63c4c2

…be removed)

Ocramius added a commit that referenced this pull request Jan 14, 2017

#59 deprecating findOrCreateMethod

169143c

Ocramius added a commit that referenced this pull request Jan 14, 2017

Merge branch 'feature/#59-make-hydrators-standalone-classes-without-i…

e58edb1

…nheritance' Cleanups for #59

Ocramius mentioned this pull request Jan 14, 2017

Generated hydrator and extractor should be a closure, not a class #64

Open

4 tasks

pounard mentioned this pull request Apr 28, 2017

Compatibility with nikic/php-parser v3 #67

Closed

Ocramius modified the milestones: 3.0.0, 2.1.0 Jul 22, 2017


		$ast = $builder->fromReflection($originalClass);
		$ast = array($class);

hydrators should not be proxy, now works gracefully with final classes #59

hydrators should not be proxy, now works gracefully with final classes #59

Conversation

pounard commented Jan 11, 2017 • edited Loading

pounard commented Jan 11, 2017 • edited Loading

flip111 commented Jan 11, 2017

pounard commented Jan 11, 2017 • edited Loading

flip111 commented Jan 11, 2017

pounard commented Jan 11, 2017

Choose a reason for hiding this comment

pounard Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pounard Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pounard commented Jan 11, 2017

Ocramius commented Jan 12, 2017

Ocramius commented Jan 12, 2017

pounard commented Jan 12, 2017

Ocramius commented Jan 12, 2017

pounard commented Jan 12, 2017

pounard commented Jan 12, 2017

Ocramius commented Jan 12, 2017

Ocramius commented Jan 12, 2017

Ocramius commented Jan 12, 2017

pounard commented Jan 12, 2017 • edited Loading

pounard commented Jan 13, 2017

pounard commented Jan 13, 2017

Ocramius commented Jan 13, 2017

pounard commented Jan 13, 2017

flip111 commented Jan 13, 2017

flip111 commented Jan 13, 2017

pounard commented Jan 11, 2017 •

edited

Loading

pounard commented Jan 11, 2017 •

edited

Loading

pounard commented Jan 11, 2017 •

edited

Loading

pounard Jan 11, 2017 •

edited

Loading

pounard Jan 11, 2017 •

edited

Loading

pounard commented Jan 12, 2017 •

edited

Loading