Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing the Schema by a proxy #407

Merged
merged 20 commits into from
Jan 7, 2016

Conversation

mikeSimonson
Copy link
Contributor

This PR try to explore the possibility of using Proxy object instead of the costly initialization of the schema object.

@mikeSimonson
Copy link
Contributor Author

@Ocramius ?

@Ocramius
Copy link
Member

@mikeSimonson the first run was just pseudo-code

@mikeSimonson
Copy link
Contributor Author

it needs a first run to warm up some sort of cache ?

@Ocramius
Copy link
Member

@mikeSimonson you didn't import any of the proxy lib stuff :-)

use ProxyManager\Factory\LazyLoadingGhostFactory;
use ProxyManager\Proxy\LazyLoadingInterface;

class LazySchemaManipulator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not implementing a common interface looks wrong to me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The name also needs fixing: we just mashed it together real quick in a video call yesterday :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea on a better naming ? for the class and the interface ?

@stof
Copy link
Member

stof commented Dec 30, 2015

@mikeSimonson I guess this is an alternative to #323.
I see 1 drawback: if the user makes any call to the Schema by mistake, they would go to the slower codebase even though they might not want it. My proposal makes it an explicit choice, making people aware that they are going the slower way. What do you think ?

/** @var AbstractSchemaManager */
private $schemaManager;

public function _construct(AbstractSchemaManager $schemaManager, $platform)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing _, so this is not a constructor. This is why you are calling on null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeSimonson please fix this so that tests can run for real

@stof
Copy link
Member

stof commented Dec 30, 2015

@mikeSimonson the PR should describe what it really does IMO, not the issue you are facing currently

@mikeSimonson mikeSimonson changed the title the test are failing with a different error on first and second run Replacing the Schema by a proxy Jan 1, 2016
@mikeSimonson
Copy link
Contributor Author

@stof The problem with your solution is that right now most of the people wouldn't benefit from it.
But it's definitely the way to go and the way it will be for v2 (1 interface that allow the schema and one that don't).

Can you look again at the last change I made.
I think that I took care of the issue with the double instantiation instead of the cloning.
All your others remarks should be fixed too except for the interface name.

I was thinking to something along the line of SchemaDiffProvider or SchemaProvider.
Do you have an opinion ?

Thanks for your review

@mikeSimonson
Copy link
Contributor Author

@stof Do you have some time to look at this again ?

Schema::class,
function (& $wrappedObject, $proxy, $method, array $parameters, & $initializer) use ($originalSchemaManipulator, $fromSchema) {
$initializer = null;
$wrappedObject = $originalSchemaManipulator->createToSchema($fromSchema);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you force initializing the fromSchema here before passing it to the originalSchemaManipulator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createToSchema method clone the fromSchema and the clone trigger the initialization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it is good

@stof
Copy link
Member

stof commented Jan 5, 2016

@mikeSimonson it is great to see the build being green here.

* <http://www.doctrine-project.org>.
*/

namespace Doctrine\DBAL\Migrations\Provider;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it in a subnamespace while implementations are not ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no valid reason

@mikeSimonson
Copy link
Contributor Author

@stof @Ocramius Ok I think that this can be merged now ?

mikeSimonson added a commit that referenced this pull request Jan 7, 2016
@mikeSimonson mikeSimonson merged commit 4461bf9 into master Jan 7, 2016
@mikeSimonson
Copy link
Contributor Author

@stof Do you think that you could do a profile of v1.2.1 against master to see the benefits ?

@stof stof deleted the prototype/lazy-schema-manipulator branch January 7, 2016 21:56
@stof
Copy link
Member

stof commented Jan 7, 2016

Here you go. This is the result of running 109 migrations on PostgreSQL 9.3 and PHP 5.6.16 on my laptop (with XDebug enabled as I forgot to disable it before profiling):

We have a 97% performance improvement here (from 2min20s to 4.68s). This is similar to the 94% I reported in #323 (the difference is probably related to the fact that I have more migrations than at that time, making the effect even bigger)

@Ocramius
Copy link
Member

Ocramius commented Jan 7, 2016

That is quite... impressive :O

@mikeSimonson still, are you sure that we want proxymanager installed as hard dependency?

@stof
Copy link
Member

stof commented Jan 7, 2016

@Ocramius I think we do (I'm looking forward the release of a zend-code release without the zend-stdlib dependency though, to have less transitive deps)

@Ocramius
Copy link
Member

Ocramius commented Jan 7, 2016

As far as I know, it doesn't require stdlib, but the eventmanager: https://github.com/zendframework/zend-code/blob/4972c33d975fa0c2b86ff381e83fde177079b249/composer.json#L17

That can't be fixed until we drop annotations support ( zendframework/zend-code#32 ), which will only happen in a major revision.

Making it an optional dependency without dropping that feature will just lead to a lot of people having headaches when they want to actually use the annotations support :-\

@stof
Copy link
Member

stof commented Jan 7, 2016

There is a drawback in this change though: look at the metrics about the GC runs. We went from 1 to 81.
I think this is related to a circular object graph in unitialized proxies. It may be worth investigating it to see whether we can break the object graph when we don't need the proxies anymore (at the end of each migration). But the time spent in GC is not comparable to the time win, so this does not block a 1.3.0 release.

@stof
Copy link
Member

stof commented Jan 7, 2016

And regarding the dependencies, I think a 97% perf increase justifies the mandatory dependency. If we make it optional, people would have to opt-in the fast way, which would not be much better than #323 (well, except the opt-in would be in a single place), and we would have to expose this layer outside the library (currently, it is hidden from the outside, allowing us to treat it as fully internal)

@Ocramius
Copy link
Member

Ocramius commented Jan 7, 2016

It may be worth investigating it to see whether we can break the object graph when we don't need the proxies anymore (at the end of each migration).

The initializer closures will obviously cause cyclic graphs. There is a way to "fix that" (simply setInitializer(null) on proxies), but:

  • it breaks the abstraction
  • obviously isn't that relevant

Agree on all the conclusions.

@stof
Copy link
Member

stof commented Jan 7, 2016

@Ocramius the idea is to reset the initializer at the end of the migration, when we don't need the Schema object anymore (the refcount will then destroy it)

@Ocramius
Copy link
Member

Ocramius commented Jan 7, 2016

@stof I wouldn't do that anyway, as it may lead to unexpected bugs (in case the objects are needed after migration run, or are used in any other context)

@Ocramius
Copy link
Member

Ocramius commented Jan 7, 2016

@mikeSimonson this patch still lacks some tests that are required to prevent any regression that may by accident initialize a migration. I know it is a pain, but that means extremely detailed mocking :-\

*
* @return string[]
*/
public function getSqlDiffToMigrate(Schema $fromSchema,Schema $toSchema);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space

@mikeSimonson
Copy link
Contributor Author

@Ocramius What sort of test do you think are missing ?

@mikeSimonson
Copy link
Contributor Author

@stof detail but when I look at the profile of the future 1.3 and that I go in the Metrics tab I see one call to GC et 81 for the profile of the 1.2. But in the comparison page in the metrics tab the numbers seems inverted. Same for the number of PDO and SQL queries. Bug in blackfire ?

Btw it make more sense to me that there would be less call to the GC with the Proxy because we are not instantiating all those Tables objects in the Schema.

@Ocramius
Copy link
Member

Ocramius commented Jan 8, 2016

@mikeSimonson the tests that I'm referring to would expect the schema object methods to never be called: basically a set of $schema->expects(self::never())->method('theMethod') for all methods.

@mikeSimonson
Copy link
Contributor Author

@Ocramius I don't understand the goal this test ? Do you mean testing that none of the method of the schema are called in the pre and post method or something else ?

@Ocramius
Copy link
Member

Ocramius commented Jan 8, 2016

@mikeSimonson correct. That is just to prevent regressions though.

@stof
Copy link
Member

stof commented Jan 8, 2016

Bug in blackfire ?

Looks like it is. then everything is good (and the issue would then be about the Schema system in DBAL relying on cyclic object graphs, which is off-topic)

@stof
Copy link
Member

stof commented Jan 8, 2016

I reported the bug to the Blackfire team

@mikeSimonson
Copy link
Contributor Author

@Ocramius There are already tests for that. It test that if those methods are used it has no impact on the sql produced fe1447a.
@stof Thanks

@Ocramius
Copy link
Member

Ocramius commented Jan 8, 2016

@mikeSimonson that test is still calling API methods on the schema objects - a test where no method calls on the schema ever happen is required

@mikeSimonson
Copy link
Contributor Author

@Ocramius A test like that one ? #414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants