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

Improve aspcore DB benchmarks #4015

Merged
merged 4 commits into from
Aug 28, 2018

Conversation

bgrainger
Copy link
Contributor

@bgrainger bgrainger commented Aug 25, 2018

  • Updates MySqlConnector and npgsql to latest published versions
  • Improves MySqlConnector connection string
  • Uses explicitly prepared commands (MySqlConnector only)

@@ -64,6 +64,8 @@ DbCommand CreateReadCommand(DbConnection connection)
id.Value = _random.Next(1, 10001);
cmd.Parameters.Add(id);

(cmd as MySql.Data.MySqlClient.MySqlCommand)?.Prepare();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebastienros Based on 14ad208 I decided to only opt MySqlConnector into this. It doesn't have npgsql's auto-prepare capability, so this improves performance by 10-15% (in my local Docker testing).

I tested changing this to just cmd.Prepare() with aspcore-ado-pg and got these results:

Test Before After Change
Fortunes 43492.60 43695.85 -0.47%
DB 46917.18 47240.47 +0.69%

I'm not sure if this is significant, or just within the experimental margin-of-error.

Perhaps @roji has an opinion on whether explicit Prepare() should be used for both npgsql and MySqlConnector or left like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we should probably update npgsql from 4.0.0-rc1 to 4.0.2, but I didn't address that in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

In principle an explicit Prepare() should be best for Npgsql. But IIRC there was some unexplained weirdness at some point where using auto-prepare was actually faster than explicit Prepare(). I didn't have any time to look into it (and don't have any right now either...), but maybe @sebastienros can re-confirm this...

@sebastienros
Copy link
Contributor

I confirm autoprepare increased the perf significantly over Prepare().

@bgrainger
Copy link
Contributor Author

@sebastienros Once this is merged, can you change the four instances of

 (cmd as MySql.Data.MySqlClient.MySqlCommand)?.Prepare();

to

cmd.Prepare();

and check to see if you can still repro a performance decrease in aspcore-ado-pg and aspcore-mw-ado-pg when cmd.Prepare() is used?

@sebastienros
Copy link
Contributor

Or instead measure before doing this change?

@bgrainger
Copy link
Contributor Author

That's fine if you want to measure before and after this change; based on my local testing I'm pretty confident in the net effect being about +20%.

However, I really don't know if cmd.Prepare() is bad for npgsql so that's why I'm wondering if you could test that hypothesis without my having to push a potentially-negative change to TFB.

@sebastienros
Copy link
Contributor

That's my concern, regression npgsql because I tested it by the past. Maybe there used to be an issue with how Prepare() works. @roji when Prepare() is called, are you checking for existing prepared statements on the pooled connection before issuing the command to the server?

I will test this branch today for both dbs and update here.

@bgrainger
Copy link
Contributor Author

@sebastienros For npgsql specifically, can you test bgrainger@b4c23df as the baseline (that's updating npgsql to the latest version) then bgrainger@d501e97 as a variant (that's calling cmd.Prepare() for all DBs).

@sebastienros
Copy link
Contributor

sebastienros commented Aug 27, 2018

My local results:

Test Before After Change
Npgsql - Update Libraries 104,905 106,004 1.05%
Npgsql - Prepare() 106,004 104,039 -1.85%
MySql 76,880 80,213 4.34%

Note I wasn't able to test it on the same infra as TE (for reasons I have to investigate).

Conclusion is that

  • Updating to the latest Npgsql version improves the results
  • Call Prepare() explicitly in Npgsql is confirmed to be slower than AutoPrepare
  • You changes do have some positive impact (though I am not measuring 20%)

I'd say we go with this PR for now, and you can update the Npgsql version or we do it in a separate one.

@bgrainger bgrainger changed the title Improve aspcore + MySQL benchmarks Improve aspcore DB benchmarks Aug 27, 2018
@michaelhixson michaelhixson merged commit ddff3ac into TechEmpower:master Aug 28, 2018
@roji
Copy link
Contributor

roji commented Aug 28, 2018

Thanks for measuring the Npgsql side, this is really odd. In a couple months I will start getting more free and will try to investigate why explicit Prepare() is slowing things down.

@bgrainger bgrainger deleted the mysqlconnector branch August 28, 2018 03:01
@sebastienros
Copy link
Contributor

@roji I just want to insist on the fact that this table has AutoPrepare on in both cases. And I prefer this to the explicit call as this also benefits the EF/Dapper scenarios.

@roji
Copy link
Contributor

roji commented Aug 28, 2018

Oh, ok... Is it possible to do a run with explicit Prepare() but without AutoPrepare? Just to get the full picture.

Yeah, AutoPrepare is indeed you're only option when using Dapper/EF, but in theory we still want the raw ADO.NET perf to be as good as possible.

@bgrainger
Copy link
Contributor Author

@sebastienros Looks like your benchmark suite is much more accurate than my local Docker results. Here's what I got from comparing the before and after runs:

Benchmark Framework Before After Delta
Fortunes aspcore-ado-my 181,534 190,574 +5.0%
Fortunes aspcore-vb-mw-ado-my 170,391 170,549 +0.1%
Fortunes aspcore-mw-ado-my 163,827 170,023 +3.8%
Fortunes aspcore-mw-dap-my 155,365 162,011 +4.3%
Fortunes aspcore-mvc-ado-my 94,900 96,662 +1.9%
DB aspcore-ado-my 217,032 231,045 +6.5%
DB aspcore-mw-ado-my 191,171 201,157 +5.2%
DB aspcore-vb-mw-ado-my 191,207 197,575 +3.3%
DB aspcore-mvc-ado-my 133,124 138,630 +4.1%
Query aspcore-mw-ado-my 10,213 18,839 +84.5%
Query aspcore-mvc-ado-my 11,059 18,580 +68.0%
Query aspcore-vb-mw-ado-my 12,258 17,670 +44.2%
Update aspcore-mvc-ado-my 2,747 2,795 +1.7%
Update aspcore-mw-ado-my 2,341 2,550 +8.9%
Update aspcore-vb-mw-ado-my 2,736 2,440 -10.8%
Update aspcore-ado-my 2,538 2,288 -9.9%

One interesting thing is comparing aspcore-vb-mw-ado-my to aspcore-mw-ado-my. As far as I know, the code is generally supposed to be the same (although I haven't verified that the same IL is generated), the MySqlConnector version and connection string are (now) the same; the only difference is that I added the cmd.Prepare() call to the C# version. Thus we can see that for "Fortunes", preparing apparently makes no difference (i.e., C# and VB are tried), but in "DB" and "Query", there may be some benefit to preparing the query for MySQL.

I don't understand the "Update" benchmarks at all (but we've discussed that separately).

roberthusak pushed a commit to roberthusak/FrameworkBenchmarks that referenced this pull request Nov 6, 2018
* Update .NET MySQL benchmarks to latest MySqlConnector version.

* Update optimized connection string settings for MySqlConnector.

* Use prepared commands with MySqlConnector.

* Update npgsql to latest version.
@sebastienros
Copy link
Contributor

@patricka2 please only comment on old PRs if the comment is about the PR. In this case you could just create a new issue and we'd talk on it. You can delete the other duplicated comments as I answered already.

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.

4 participants