-
Notifications
You must be signed in to change notification settings - Fork 244
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
Use separate DB connection pools for read/update in platform update benchmark #1764
Conversation
@@ -203,7 +205,32 @@ public async Task<World[]> LoadMultipleUpdatesRows(int count) | |||
await reader.NextResultAsync(); | |||
} | |||
} | |||
|
|||
using (var connection = await _updateDataSource.OpenConnectionAsync()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: we don't use DisposeAsync
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah :) For cases where we know we don't need to do I/O, we somtimes just use the sync APIs, since they may be (very) slightly more efficient. Dispose only returns the connection to the pool, so no need for async here (for this benchmark).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could troll you on this. But that's not in my nature. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that so :)
In any case,it's been a very long time since I've checked if there's any meaningful difference between the async and sync versions - it may be there's nothing there any more... Even if there is, it's very unlikely to be interesting to anyone except for very high-perf scenarios like these benchmarks.
Converting to draft to experiment a bit with Max Pool Size to boost perf here, please do not merge. |
a519daf
to
fffbfea
Compare
This uses separate connection pools (data sources) for the queries and the updates in the platform updates benchmark, as suggested by @NinoFloris. Improvement isn't as significant as we hoped (and saw earlier): 3.64%. But given the variability/unreliability of the updates benchmark, we may see a better trend on the graph after a bit of time. Note that I ran this for 60 seconds instead of the default 15.
Command lines
/cc @ajcvickers @sebastienros