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

Add Db queries verification to tfb toolset verif procedure #5145

Merged
merged 167 commits into from Nov 1, 2019
Merged

Add Db queries verification to tfb toolset verif procedure #5145

merged 167 commits into from Nov 1, 2019

Conversation

jcheron
Copy link
Contributor

@jcheron jcheron commented Oct 11, 2019

It seems necessary to know if each tested framework or platform always responds in case of stress (high concurrency level) and actually perform all queries to the database.
The Ubiquity-workerman case proved that a framework can silently fail (no logs, no errors issued when not all db queries are made).

I guess the Techempower team has all the skills to make this change, so if I could just save you some time.

This PR proposes a solution to evaluate the number of queries actually made, at the maximum concurrency level (512), and integrates this counting into the verification phase:

./tfb --mode verify --test frameworkName 

For each test requesting the database (Db, Queries, Updates, Fortunes), the principle is as follows:

  1. Checking of failed transactions (0 expected) -> except on Travis-ci
  2. Retrieval from the database statistics of the number of queries made, the number of rows read (and eventually updated).
  3. Realization of n http requests at the concurrency level 512 with a bench tool (siege).
  4. Retrieving statistics again, calculating the number of queries made, the number of rows read.
  5. Comparison with expected values, and failure in case of deviation (insufficient number).
    A margin of 1% is acceptable for the lines read (it seems to me that the stats are not 100% reliable)

Note:
The acceptance margin on db queries is higher for Travis-ci runs, as resources are limited.

The method can certainly be improved, this is a first proposal...

Having tested quite a few frameworks for development purposes, it seems that Ubiquity workerman is not the only one in his case.

@jcheron jcheron changed the title Add tfb toolset Db queries verification Add Db queries verification to tfb toolset verif procedure Oct 11, 2019
Copy link
Member

@nbrady-techempower nbrady-techempower left a comment

Unfortunately I think you just missed the merge of #4549, but it shouldn't be too hard to make this fit with that.

Copy link
Member

@nbrady-techempower nbrady-techempower left a comment

Overall, this is great work and we were just talking about getting this done, so this is really appreciated. We just have to fit this in to the model we currently have. Let me know if you need some help doing that. I'm out for the week, but I'll be back on Monday.

toolset/benchmark/test_types/framework_test_type.py Outdated Show resolved Hide resolved
toolset/benchmark/test_types/verifications.py Outdated Show resolved Hide resolved
@jcheron
Copy link
Contributor Author

@jcheron jcheron commented Oct 11, 2019

Unfortunately I think you just missed the merge of #4549, but it shouldn't be too hard to make this fit with that.

Yes, it's unfortunate!
I therefore merge my Db classes with those of this PR?

@nbrady-techempower
Copy link
Member

@nbrady-techempower nbrady-techempower commented Oct 11, 2019

Yes, that should pretty much do it and then we can clean up from there.

@nbrady-techempower
Copy link
Member

@nbrady-techempower nbrady-techempower commented Oct 31, 2019

@jcheron Could you merge in the latest master and give this a full run. I think we're about ready to get this in so that new PR's will test against it while we're fixing other frameworks.

@jcheron
Copy link
Contributor Author

@jcheron jcheron commented Nov 1, 2019

@nbrady-techempower
It may be more convenient if you have control over the initial repo.

@nbrady-techempower
Copy link
Member

@nbrady-techempower nbrady-techempower commented Nov 1, 2019

We're just gonna go for it.

@nbrady-techempower nbrady-techempower merged commit a8ae3f2 into TechEmpower:master Nov 1, 2019
@nbrady-techempower
Copy link
Member

@nbrady-techempower nbrady-techempower commented Nov 1, 2019

I got this in because there was a new run on Citrine about to start, so let's see how this does there.

@msmith-techempower
Copy link
Member

@msmith-techempower msmith-techempower commented Nov 1, 2019

I got this in because there was a new run on Citrine about to start, so let's see how this does there.

I confirmed that it is included in the latest run on Citrine.

@nbrady-techempower
Copy link
Member

@nbrady-techempower nbrady-techempower commented Nov 1, 2019

Good work team

@jcheron
Copy link
Contributor Author

@jcheron jcheron commented Nov 1, 2019

Good work team

yes to self-congratulation!
We have been efficient.

And for now, everything looks normal on Citrine

@jcheron
Copy link
Contributor Author

@jcheron jcheron commented Nov 2, 2019

That's a little worrying....

image

@michaelhixson
Copy link
Member

@michaelhixson michaelhixson commented Nov 2, 2019

It looks like it crashed while verifying the updates test for akka-http-slick-postgres. That framework's updates/verification.txt file is the most recently updated file that I see, and it has the "VERIFYING UPDATE" line but no results (such as "PASS") below that. I don't see any stack traces or error messages in the logs. The TFB application is not stuck; it exited.

I'm restarting it now, though I'm expecting it to get stuck in a similar way later tonight or tomorrow.

@jcheron
Copy link
Contributor Author

@jcheron jcheron commented Nov 2, 2019

It looks like it crashed while verifying the updates test for akka-http-slick-postgres. That framework's updates/verification.txt file is the most recently updated file that I see, and it has the "VERIFYING UPDATE" line but no results (such as "PASS") below that. I don't see any stack traces or error messages in the logs. The TFB application is not stuck; it exited.

I'm restarting it now, though I'm expecting it to get stuck in a similar way later tonight or tomorrow.

I will add a timeout on the siege command execution.

nbrady-techempower pushed a commit that referenced this issue Nov 4, 2019
* [ci skip] try to fix siege command blocking

see #5145

* python 2.7 version (no timeout for subprocess)

* add PopenTimeout class

* [ci skip] Remove unused methods

* try all systems version

* Increase timeout (20s)
worlds[i] = getRandomWorld();
}
//Pick unique random numbers
final AtomicInteger i = new AtomicInteger(0);
Copy link
Contributor

@zloster zloster Nov 5, 2019

Choose a reason for hiding this comment

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

IMO using AtomicInteger is not necessary here. Maybe the approach from https://github.com/TechEmpower/FrameworkBenchmarks/pull/5183/files#diff-59538f8c6919604024599c6e6b708eecR70-R76 could be used?

Copy link
Contributor

@zloster zloster Nov 5, 2019

Choose a reason for hiding this comment

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

Copy link
Member

@michaelhixson michaelhixson Nov 5, 2019

Choose a reason for hiding this comment

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

@zloster If I were writing the code I'd also use mapToObj, but I'll share some findings about the performance that I thought were interesting.

Last week I tried using JMH to measure the overhead of different techniques for generating distinct world ids. I just uploaded it to GitHub: repository, Java code.

The results from my workstation are like this (lower is better):

Benchmark                        Mode  Cnt     Score    Error  Units
WorldIdsBenchmark.arrayCounter   avgt    5  1123.397 ± 44.659  ns/op
WorldIdsBenchmark.atomicCounter  avgt    5  1115.252 ± 50.794  ns/op
WorldIdsBenchmark.boxedSet       avgt    5   525.349 ± 15.149  ns/op
WorldIdsBenchmark.primitiveSet   avgt    5   375.261 ±  4.094  ns/op
WorldIdsBenchmark.streamOnly     avgt    5  1336.233 ± 27.164  ns/op

It seems that the overhead of additional stream ops in my preferred approach (streamOnly) is higher than the overheard from using AtomicInteger (atomicCounter). But they are very close. I feel like the impact of choosing any one approach over the other isn't going to make a noticeable difference in the context of TFB. This piece of it affects around +/-1 microsecond per request (using the measurements from my workstation) and I imagine that's imperceptible next to all the database queries.

Copy link
Contributor

@zloster zloster Nov 5, 2019

Choose a reason for hiding this comment

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

@michaelhixson This is interesting, thank you. I see the JMH test was run with Java 13. I'll check if the results are different with older JVM versions.
Note to myself: I don't like the too big difference between the basic loops and the streams variants.

Copy link
Contributor Author

@jcheron jcheron Nov 5, 2019

Choose a reason for hiding this comment

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

My concern that day was not optimization... but when it is, I will tend to say that there is never any unnecessary optimization, even for a micro-second, and even if I agree with @michaelhixson that it has no effect in the context of our db tests.
The benchmark is interesting, I wouldn't have bet on this result:
the simplest methods are often the best (+1 for IntOpenHashSet)

Copy link
Contributor

@zloster zloster Nov 8, 2019

Choose a reason for hiding this comment

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

@jcheron I use this as an illustration how many CPU cycles are wasted these days. Echoing 4 chars in a buffer is several instructions. The others (up to a ~0,7 million) instructions are very hefty price to achieve the goal. JMH has support for reporting similar data (instructions count). I'm curious to see the data.

@zloster
Copy link
Contributor

@zloster zloster commented Nov 8, 2019

There is one more active issue with similar problem. I have suggested something similar in a comment there.

tomciopp pushed a commit to tomciopp/FrameworkBenchmarks that referenced this issue Oct 21, 2020
…er#5145)

* Add toolset Db queries verification

* [ci skip] revert easier debug

* [ci skip] Reintegrate the modified files in PR TechEmpower#4549

* [ci skip] Add check methods to Mysql Database

* [ci skip] Add forgotten empty line

* [ci skip] Add super class Database

* [ci skip] call verify_queries

* [ci skip] Add queries checking in Postgres & Mongodb

* [ci skip] Fix indentation

* [ci skip] Fix tbl_name for Postgres

* [ci skip] db_type pb

* [ci skip] staticmethod->classmethod

* [ci skip] cls attr in abstract_database

* [ci skip] Remove cls arg in calls

* [ci skip] test with abstractmethod

* [ci skip] Fix indented block

* [ci skip] Add return!

* [ci skip] ABC?

* [ci skip] Fix abstractmethod

* [ci skip] Add abstract methods in AbstractDatabase

* [ci skip] Add verification for updates

* [ci skip] add pass log

* [ci skip] Fix typo

* [ci skip] Add pass log for rows & rows_updated

* [ci skip] Fix typos

* [ci skip] check updates in Mongo

* [ci skip] Include the error deviation for Mysql.

* [ci skip] remove the old deviation

* [ci skip] Test with siege instead of ab

* [ci skip] Fix float error in Mysql

* [ci skip] create siege config

* [ci skip] create siege dir

* [ci skip] siege keep alive

* [ci skip] Fix typo

* [ci skip]( pb in sh command

* [skip ci] ( pb in sh

* [ci skip] add siegerc file

* [ci skip] try without mysql approximation

* [ci skip] revert remove Mysql approx (1.001)

* [ci skip] fix getQueries in Mongodb (argument!)

* [ci skip] remove unused tbl_name arg in Mongodb

* [ci skip] Inclusion of Mongodb rows

* [ci skip] Fix pb with rows in Fortunes (Mongodb)

* [ci skip] fortune count return 0 (mongodb)

* [ci skip] usage of count_documents({})

* [ci skip] code documentation

* [ci skip] Remove siege verbose

* Ready for clean up

Run Travis-ci tests

* [ci skip] no query test for cached_queries

* [ci skip] Fix spaces

* [ci skip] display real number of rows without margin

* [ci skip] try Mysql stats with global status

for adjusting the approximation on the results

* [ci skip] try with flush status (Mysql)

* [ci skip] give reload privileges to Mysql user

* [ci skip] revert flush status + global (Mysql)

+ increase Mysql margin to 1.0015

* [ci skip] Try with INFORMATION_SCHEMA.SESSION_STATUS (Mysql)

* [ci skip] Performance_shema since 5.7

* [ci skip] Remove updated rows from get_rows in Mysql

* [ci skip] display queries refactoring

* [ci skip] add verbose to siege for Python frameworks pb

* [ci skip] Update Postgres for table names with quotes in queries

* [ci skip] Update Postgre stats regex

* Remove siege verbose + Fix Python-pg pb

Run Travis tests

* [ci skip] Trying more precision for Mysql requests

* [ci skip] Fix query (one field!)

* [ci skip] Fix Mysql query

* [ci skip] more precision in queries couting for Mysql

* [skip ci] add connexion infos

* [ci skip] Fix arg omited

* [ci skip] log db infos

* reduce travis execution time (1 repetition * 512) + add mysql stats info (errors)

* Add toolset Db queries verification

* [ci skip] revert easier debug

* [ci skip] Reintegrate the modified files in PR TechEmpower#4549

* [ci skip] Add check methods to Mysql Database

* [ci skip] Add forgotten empty line

* [ci skip] Add super class Database

* [ci skip] call verify_queries

* [ci skip] Add queries checking in Postgres & Mongodb

* [ci skip] Fix indentation

* [ci skip] Fix tbl_name for Postgres

* [ci skip] db_type pb

* [ci skip] staticmethod->classmethod

* [ci skip] cls attr in abstract_database

* [ci skip] Remove cls arg in calls

* [ci skip] test with abstractmethod

* [ci skip] Fix indented block

* [ci skip] Add return!

* [ci skip] ABC?

* [ci skip] Fix abstractmethod

* [ci skip] Add abstract methods in AbstractDatabase

* [ci skip] Add verification for updates

* [ci skip] add pass log

* [ci skip] Fix typo

* [ci skip] Add pass log for rows & rows_updated

* [ci skip] Fix typos

* [ci skip] check updates in Mongo

* [ci skip] Include the error deviation for Mysql.

* [ci skip] remove the old deviation

* [ci skip] Test with siege instead of ab

* [ci skip] Fix float error in Mysql

* [ci skip] create siege config

* [ci skip] create siege dir

* [ci skip] siege keep alive

* [ci skip] Fix typo

* [ci skip]( pb in sh command

* [skip ci] ( pb in sh

* [ci skip] add siegerc file

* [ci skip] try without mysql approximation

* [ci skip] revert remove Mysql approx (1.001)

* [ci skip] fix getQueries in Mongodb (argument!)

* [ci skip] remove unused tbl_name arg in Mongodb

* [ci skip] Inclusion of Mongodb rows

* [ci skip] Fix pb with rows in Fortunes (Mongodb)

* [ci skip] fortune count return 0 (mongodb)

* [ci skip] usage of count_documents({})

* [ci skip] code documentation

* [ci skip] Remove siege verbose

* Ready for clean up

Run Travis-ci tests

* [ci skip] no query test for cached_queries

* [ci skip] Fix spaces

* [ci skip] display real number of rows without margin

* [ci skip] try Mysql stats with global status

for adjusting the approximation on the results

* [ci skip] try with flush status (Mysql)

* [ci skip] give reload privileges to Mysql user

* [ci skip] revert flush status + global (Mysql)

+ increase Mysql margin to 1.0015

* [ci skip] Try with INFORMATION_SCHEMA.SESSION_STATUS (Mysql)

* [ci skip] Performance_shema since 5.7

* [ci skip] Remove updated rows from get_rows in Mysql

* [ci skip] display queries refactoring

* [ci skip] add verbose to siege for Python frameworks pb

* [ci skip] Update Postgres for table names with quotes in queries

* [ci skip] Update Postgre stats regex

* Remove siege verbose + Fix Python-pg pb

Run Travis tests

* [ci skip] Trying more precision for Mysql requests

* [ci skip] Fix query (one field!)

* [ci skip] Fix Mysql query

* [ci skip] more precision in queries couting for Mysql

* [skip ci] add connexion infos

* [ci skip] Fix arg omited

* [ci skip] log db infos

* reduce travis execution time (1 repetition * 512) + add mysql stats info (errors)

* [ci skip] Remove Mysql error infos: irrelevant

* Decrease concurrency for checking pg stats

* [ci skip] cancel concurrency change

* [ci skip] Possible bulk updates included

* postgres factorization + run travis

* [ci skip] cleaner getQueries for Mysql

* [ci skip] add marge for bulk update queries

* [ci skip] Fix typo

* [ci skip] warn for excessive number + restore query number for no bulk

* run travis

* [ci skip] max(self.config.concurrency_levels) usage

* Fixed Hibernate cache problem for ninja-standalone (demo)

* [ci skip] import AtomicInteger !!

* travis for ninja + remove -1 in Mysql

* [ci skip] Add a margin based on the number of processors

* run travis with margin on queries (1.015)

* Increase default margin

for roda-sequel, sinatra...
Fail on Travis, but pass elsewhere

* [ci skip] Add transactions failures checking

* [ci skip] better names

* [ci skip] update to pgsql 12

* [ci skip] Remove failed transactions checking on Travis

Fix Hasket/yesod pb (see https://travis-ci.org/TechEmpower/FrameworkBenchmarks/jobs/605281645#L6023)

* [ci skip] merge last 5 PRs
tomciopp pushed a commit to tomciopp/FrameworkBenchmarks that referenced this issue Oct 21, 2020
* [ci skip] try to fix siege command blocking

see TechEmpower#5145

* python 2.7 version (no timeout for subprocess)

* add PopenTimeout class

* [ci skip] Remove unused methods

* try all systems version

* Increase timeout (20s)
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.

None yet

7 participants