Initial implementation of benchmarks #60

dbalabka · 2017-12-20T18:40:34Z

I think it is important to see some benchmarks comparing with PDO due to pure PHP implementations usually are slow. It is important to know how much slower.
In case of the async driver, we can come up with a conclusion to use the async driver instead of PDO to get a performance boost by executing queries in an async way.

I'm proposing to run benchmarks right after each build to see the performance. Hope that in future PHP releases pure PHP driver implementation performance will be very close to PDO.

There is only one benchmark with simple query SELECT $n with following implementations:

Sync execution using PDO
Sync execution using AmpMySQL
Async execution using AmpMySQL
Async execution using AmpMySQL with connection pool

kelunik · 2017-12-20T18:45:22Z

.travis.yml

@@ -69,6 +69,8 @@ before_script:
 script:
  - phpdbg -qrr vendor/bin/phpunit --coverage-text --coverage-clover build/logs/clover.xml
  - PHP_CS_FIXER_IGNORE_ENV=1 php vendor/bin/php-cs-fixer --diff --dry-run -v fix
+  - chmod +x benchmarks/bin/php_no_xdebug


Just commit the file go Git with chmod +x and it will be fine. No need to run this each time.

Thank you. Never used this feature :) Fixed

kelunik · 2017-12-20T18:46:41Z

.travis.yml

@@ -69,6 +69,8 @@ before_script:
 script:
  - phpdbg -qrr vendor/bin/phpunit --coverage-text --coverage-clover build/logs/clover.xml
  - PHP_CS_FIXER_IGNORE_ENV=1 php vendor/bin/php-cs-fixer --diff --dry-run -v fix
+  - chmod +x benchmarks/bin/php_no_xdebug
+  - PATH=$PATH:$(pwd)/benchmarks/bin vendor/bin/phpbench run --report=aggregate


Not sure we have to run that for each build job. It's probably better to have a matrix that separates builds which run (unit) tests and then only one job (one PHP version is enough) to run these benchmarks.

Fixed. Fully agree :)

kelunik · 2017-12-20T18:48:18Z

benchmarks/QueryBench.php

+            foreach (range(1, $this->maxQueries) as $i) {
+                /** @var ResultSet $resultSet */
+                $resultSet = yield $connection->query("SELECT $i");
+                yield $resultSet->advance();


This will fetch one row, while PDO fetches all rows.

Fixed. Previously I have used fetchAll, but in the new version it's not available.

kelunik · 2017-12-20T18:49:27Z

benchmarks/QueryBench.php

+            yield all(array_map(function ($resultSet) {
+                /** @var ResultSet $resultSet */
+                return $resultSet->advance();
+            }, $resultSets));


You can leave of the all if you want, it will implicitly be applied if you yield an array of promises, but that's personal preference, just a hint if you didn't know.

Thank you. Not sure, but probably a previous version of amp had required all here. Cool, now much clearer.

Yes, 1.x didn't allow yielding array of promises.

kelunik · 2017-12-20T18:50:53Z

benchmarks/bin/php_no_xdebug

+temporaryPath="$(mktemp -t php.XXXX).ini"
+
+# Using awk to ensure that files ending without newlines do not lead to configuration error
+php -i | grep "\.ini" | grep -o -e '\(/[a-z0-9._-]\+\)\+\.ini' | grep -v xdebug | xargs awk 'FNR==1{print ""}1' > "$temporaryPath"


There's no need for that on Travis at least:

mysql/.travis.yml

Line 57 in 67f101e

- phpenv config-rm xdebug.ini || echo "No xdebug config."

As you suggested, now I remove all configuration with -n, so think that there no need for this configuration anymore.

kelunik · 2017-12-20T18:51:20Z

benchmarks/bin/php_no_xdebug

+# Using awk to ensure that files ending without newlines do not lead to configuration error
+php -i | grep "\.ini" | grep -o -e '\(/[a-z0-9._-]\+\)\+\.ini' | grep -v xdebug | xargs awk 'FNR==1{print ""}1' > "$temporaryPath"
+
+php -n -c "$temporaryPath" -dopcache.enable_cli=1 -dzend.assertions=-1 "$@"


How about just doing -n and not passing any config at all?

hmm... probably PDO might not work, but let try :)

kelunik · 2017-12-20T18:51:36Z

composer.json

@@ -3,7 +3,8 @@
    "description": "Asynchronous parallel Mysql client built on the Amp concurrency framework",
    "require": {
        "amphp/amp": "^2",
-        "amphp/socket": "^0.10"
+        "amphp/socket": "^0.10",
+        "phpbench/phpbench": "^0.13.0"


This should go into require-dev, actually it's there, too, so delete it here.

Fixed. Probably during merge I have added this in both sections.

trowski · 2017-12-20T19:08:25Z

benchmarks/QueryBench.php

+        wait(call(function() {
+            $connection = $this->connectionPool;
+            /** @var ResultSet[] $resultSets */
+            $resultSets = yield all(array_map(function ($i) use ($connection) {


This is also benching creating the connections to the database. Priming the pool would give more realistic benchmarks for a running application doing queries simultaneously.

@trowski I already have faced with this problem. To avoid such situation I have added @Warmup(1) to create all needed connection during the first run and avoid connection overhead to be included in overall benchmark time.

When I copy the body of benchAsyncQueriesUsingPool to init, it reduces the time for the benchmark relative to the others significantly. Can you explain what's going on there?

@trowski in nutshell PHPBench does not count the time that was spent during init execution.

To understand how it works need to describe how works Revs(revolutions), Iterations and BeforeMethods(read more http://phpbench.readthedocs.io/en/latest/writing-benchmarks.html)

During one Warmup(it is equivalent to one revolution) we create all needed connections in the pool(read more http://phpbench.readthedocs.io/en/latest/writing-benchmarks.html?highlight=Warmup)

kelunik · 2017-12-21T12:20:22Z

@torinaki I think switching to stages could make the build way easier like https://travis-ci.org/FriendsOfPHP/PHP-CS-Fixer does it.

dbalabka · 2018-01-09T06:34:32Z

@kelunik I did a migration to stages. The only issue is that I can't get build green due to unstable test:
Amp\Mysql\Test\ConnectionPoolTest::testIdleConnectionsRemovedAfterTimeout
I see same failing test in master:
https://travis-ci.org/amphp/mysql/jobs/324452178
Also, it affects benchmark stage. It doesn't run because test stage is failing.

dbalabka · 2018-02-04T12:08:54Z

@kelunik, @trowski I have implemented stages. It works. Currently, benchmarks would not be started if tests are failing. I see there are some unstable tests in master. Probably it would be better to enable benchmarks to run always in spite of tests status. How do you think?

kelunik · 2018-02-04T13:34:43Z

No, these tests should just be fixed.

Could you add your new directory to .php_cs.dist and adjust the code style accordingly, please?

Other than that, it looks pretty fine now. 👍

dbalabka · 2018-02-05T14:35:32Z

@kelunik done. Please check

kelunik · 2018-02-05T16:50:17Z

Thanks!

kelunik reviewed Dec 20, 2017

View reviewed changes

trowski reviewed Dec 20, 2017

View reviewed changes

dbalabka force-pushed the benchmarks branch from a09be02 to 13071bc Compare February 4, 2018 11:45

dbalabka force-pushed the benchmarks branch from 13071bc to 16984f9 Compare February 5, 2018 08:13

Initial implementation of benchmarking

4679dbf

dbalabka force-pushed the benchmarks branch from 16984f9 to 4679dbf Compare February 5, 2018 08:55

kelunik merged commit fd33e79 into amphp:master Feb 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of benchmarks #60

Initial implementation of benchmarks #60

dbalabka commented Dec 20, 2017 •

edited

Loading

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

kelunik Dec 20, 2017

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

kelunik Dec 20, 2017

dbalabka Dec 20, 2017

trowski Dec 20, 2017 •

edited

Loading

dbalabka Dec 20, 2017 •

edited

Loading

trowski Dec 20, 2017

dbalabka Jan 4, 2018

kelunik commented Dec 21, 2017

dbalabka commented Jan 9, 2018

dbalabka commented Feb 4, 2018

kelunik commented Feb 4, 2018 •

edited

Loading

dbalabka commented Feb 5, 2018

kelunik commented Feb 5, 2018

Initial implementation of benchmarks #60

Initial implementation of benchmarks #60

Conversation

dbalabka commented Dec 20, 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

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

trowski Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

dbalabka Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik commented Dec 21, 2017

dbalabka commented Jan 9, 2018

dbalabka commented Feb 4, 2018

kelunik commented Feb 4, 2018 • edited Loading

dbalabka commented Feb 5, 2018

kelunik commented Feb 5, 2018

dbalabka commented Dec 20, 2017 •

edited

Loading

trowski Dec 20, 2017 •

edited

Loading

dbalabka Dec 20, 2017 •

edited

Loading

kelunik commented Feb 4, 2018 •

edited

Loading