Skip to content

Conversation

@alcaeus
Copy link
Member

@alcaeus alcaeus commented Apr 29, 2019

Q A
Type feature
BC Break no
Fixed issues -

Summary

This PR introduces a few basic performance benchmarks to compare the performance of ext-mongo vs. ext-mongodb + mongo-php-adapter in 1.3, and then ext-mongodb natively in 2.0. For now, we have three benchmarks around storing, loading and hydrating documents. I have yet to understand the reason for the change in https://github.com/doctrine/mongodb-odm/pull/1908/files#diff-2022fc4ba746afe6891859e465c4474bR219 but will add a benchmark for that as well.

At the moment, the BaseBench class duplicates code from the PHPUnit bootstrap - not sure if abstracting that makes sense at this time.

@alcaeus alcaeus added the Task label Apr 29, 2019
@alcaeus alcaeus added this to the 1.3.0 milestone Apr 29, 2019
@alcaeus alcaeus self-assigned this Apr 29, 2019
@alcaeus alcaeus modified the milestones: 1.3.0-RC1, 1.3.0 Jun 5, 2019
@alcaeus alcaeus force-pushed the performance-benchmark branch 3 times, most recently from 932a11b to edb17bf Compare September 11, 2019 12:23
@alcaeus alcaeus changed the title [WIP] Performance benchmark Performance benchmark Sep 11, 2019
@alcaeus
Copy link
Member Author

alcaeus commented Sep 11, 2019

@jmikola @malarzm This is ready for now, I made sure to test some examples with embedded documents and references. Right now we check the performance of storing/loading documents as well as hydrating data. These are just basic tests, but I'm hoping that these will help me understand the impact of some of the changes we made in 2.0, but also those suggested in #1908.

We'll try to expand the performance test suite in future 2.0 work, but I don't think it's worth holding up 1.3 or 2.0 much longer just because of some tests.

@alcaeus alcaeus force-pushed the performance-benchmark branch 3 times, most recently from d9251d8 to 6d390c2 Compare September 11, 2019 14:44
@malarzm
Copy link
Member

malarzm commented Sep 11, 2019

Shall we add benchmark folder to .gitattributes so it's ignored during export?

@malarzm
Copy link
Member

malarzm commented Sep 11, 2019

Other than the previous comment this looks awesome!

@alcaeus
Copy link
Member Author

alcaeus commented Sep 12, 2019

I noticed we don't have .gitattributes in 1.3, so I'll add it in a separate PR. Thanks for reminding me 👍

@alcaeus alcaeus requested a review from jmikola September 12, 2019 17:19
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

A couple of questions, but I'll defer to @malarzm's LGTM.

use Doctrine\ODM\MongoDB\Hydrator\HydratorInterface;
use Documents\User;
use MongoDate;
use MongoId;
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is the benchmark for ext-mongo and the adapter, which will be revised after merging this up to master.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct. I've tested this before in master to see what changes we need and will fix this once merging up to master.

{
// Check if the database exists. Calling listCollections on a non-existing
// database in a sharded setup will cause an invalid command cursor to be
// returned
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect anything less from mongos 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that came up when we added tests against a sharded cluster. Took me a while to figure out 🙄

"autoload": {
"psr-0": { "Doctrine\\ODM\\MongoDB": "lib/" }
},
"autoload-dev": {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this never needed before? Wouldn't the test suite's bootstrap file need to include vendor/autoload.php?

Also, I didn't spot a bootstrap file for the benchmark scripts. Is that because vendor/bin/phpbench knows to include vendor/autoload.php on its own?

Copy link
Member Author

Choose a reason for hiding this comment

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

The phpbench configuration uses the same bootstrap file as phpunit, which includes vendor/autoload.php. This change makes the manual adding of namespaces to the loader in said bootstrap file obsolete.

Ironically, I forgot to remove those calls and noticed while double-checking the bootstrap file to confirm that the autoloader is being used. This is now fixed.

@alcaeus alcaeus force-pushed the performance-benchmark branch from 6d390c2 to 1fcab90 Compare September 16, 2019 20:51
@alcaeus alcaeus merged commit e42c857 into doctrine:1.3.x Sep 23, 2019
@alcaeus alcaeus deleted the performance-benchmark branch September 23, 2019 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants