Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Optimization of connections and database #564

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Optimization of connections and database #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5df8fe9
Optimization of connections and database
juanrossi Sep 3, 2017
0554866
Incorrectly changed the session table in test_schema
juanrossi Sep 3, 2017
37a7acb
Removed MySQLi persistent connections and reconnect
juanrossi Oct 4, 2017
6a9ea13
Merge branch 'dev' into feature-improvements-db-548
juanrossi Dec 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions extra/hhvm.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,9 @@ hhvm.repo.central.path = /var/run/hhvm/hhvm.hhbc
hhvm.mysql.socket = /var/run/mysqld/mysqld.sock
hhvm.pdo_mysql.socket = /var/run/mysqld/mysqld.sock
hhvm.mysqli.socket = /var/run/mysqld/mysqld.sock

# MySQL Optimization
# Use persistent connections and higher timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You should remove the no-longer-accurate comment about persistent connections.

hhvm.mysql.connect_timeout = 1500
Copy link
Contributor

Choose a reason for hiding this comment

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

hhvm.mysql.connect_timeout = 1500

Can you explain the justification for the value chosen?

The default is 1000 milliseconds or 1 seconds. Increasing this by 500 milliseconds means that HHVM has a total of 1.5 seconds to send the connection information (handshake) before the connection is rejected.

The increased time is not a bad thing per say, and increasing the connection timeout likely will provide some modest benefit for intermediate load spikes. Just need to know how this particular value was derived.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While testing the platform with heavy load (200+ concurrent users, which is not a lot) I saw a lot of timeouts connecting the database, so this helped a bit.

hhvm.mysql.read_timeout = 2000
Copy link
Contributor

Choose a reason for hiding this comment

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

hhvm.mysql.read_timeout = 2000

Can you explain the justification for lowering this from the default value?

The default is 60000 milliseconds or 60 seconds. Lowering this to 2000 means that any query that takes more than 2 seconds will fail.

While waiting 60 seconds for a query would mean the user's request would take over 60 seconds, and such a high amount of latency is far from preferred; however, it is likely preferable over not returning any data at all. Given the amount of caching, likely, the only reason the database is being queried is that the data does not exist in cache either. As such, the user's request will ultimately fail (meaning the page will not load in the case of a full page load, like initially loading the gameboard, or the AJAX request will fail, like flag submissions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I kind of agree on the "it's preferred to return data", but imho I don't think any user will wait more than 5 seconds for a requests (and I think 5 it's already a lot).

hhvm.mysql.max_retry_open_on_fail = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

hhvm.mysql.max_retry_open_on_fail = 3

No issue here. This increase will undoubtedly provide a benefit if the connection failed initially, but the HHVM and MySQL services are correctly functioning for all other intents and purposes.

However, did you perform any testing on an increase to hhvm.mysql.max_retry_query_on_fail - the default is 1 - essentially this value will determine if a failed query should be retried multiple times. Most likely, increasing the value will compound any performance issues, but it would be interesting to hear if you have any test data to substantiate that theory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with a heavy load and saw a small improvement with this, but I don't think it's such a big change. I could lower it to two, just to be on the safe side.

4 changes: 3 additions & 1 deletion src/Db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ public static function getInstance(): Db {
private function __construct() {
$this->config = parse_ini_file($this->settings_file);
$options = array(
'idle_timeout_micros' => 200000,
'idle_timeout_micros' => 2000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

'idle_timeout_micros' => 2000000,

Can you explain the justification for the value chosen?

The default value is 4000000 microseconds or 4 seconds. The value selected here is 2000000 or 2 seconds, which is lower than the default, though higher than what is currently set within the project.

Given we are using an expiration_policy of IdleTime, this value will determine when the connections in the pool are destroyed. While this does not directly have any relation to persistent connections, the implications, in this case, are similar. Given the amount of caching, database queries should be rarer than they previously were within the platform, and a value of 0.2 seconds (the old value) is likely too low, resulting in an increased volume of connection overhead. However, 2 seconds might also be too little as well. It seems likely that the new Cache/DB design would benefit from an increased value here.

Do you have any test data that provides merit to this particular value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think with 4 seconds would be ok, but 0.2 was too low and saw that the requests were generating way too many new connections. Also, a high value (like 10s) will keep the connections for a longer time which could also bring some issues if the database doesn't have a lot of available connections in the pool.

I think we should set this to a number we feel confortable but not leave the default of the framework. If this changes tomorrow to a lower value we'll see a lot of performance issues imo.

'expiration_policy' => 'IdleTime',
'per_key_connection_limit' => 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

per_key_connection_limit' => 20

Can you explain the justification for lowering this from the default value?

The default is 50 connections. Lowering this to 20 means that if any additional connections are requested after 20 exist in the pool, an exception will be thrown and the connection will not be created. It should be noted that this value is specific to number of allowed connections per MySQL server hostname, port, database, and username.

In optimal conditions, lowering this value to 20 would not cause any issue. In fact, when things are running smoothly, the maximum number of connections in the pool is only 2. Currently a full AJAX refresh of the gameboard requests just over 50 connections, but the project maintains the default of setReusable so connections in the pool that are idle will be reused (or safely recreated if they have been destroyed).

The potential issue arises when those connections in the pool are not idle. Imagine a scenario where there is a high load on the database and the gameboard AJAX refresh is requesting just over 50 connections, and none of the connections in the pool are available for reuse yet (they are still requesting/retrieving data).

To be honest, given the way the project is structured (especially, with the caching) a connection pool with 50 or even 20 connections seems highly unlikely, and would likely mean other timeouts are being hit or things are failing. So I do not think there is much risk in lowering this value. However, there doesn't seem to be much benefit, especially with the ease of increasing the MySQL connection limit (max_connections). Again, I believe erring on the side of slow data over no data is appropriate.

Do you have any test data that provides credence to lowering this particular value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 50 connections is too much. I agree that requests are going to fail, but I saw that 20 was a number were requests didn't fail and I didn't risk taking all of my available connections (which were around 1500).

I think this is a "hack" and we should improve the reuse of connections, because I was some requests creating up to 17 connections for a SINGLE requests. When you have more than 200 concurrent users playing, that's a LOT. I preferred returning some errors than saturating my MySQL to all my users.

'pool_connection_limit' => 20
Copy link
Contributor

Choose a reason for hiding this comment

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

pool_connection_limit' => 20

Can you explain the justification for lowering this from the default value?

The default is 50 connections. It should be noted that this value is the total number of pool connections, regardless of the MySQL server hostname, port, database, and username.

In theory, this value would be irrelevant, as the per_key_connection_limit would be hit when using a single database server. However, it is possible to use multiple databases. In fact, PR #419 provides the ability to use database replication across numerous replicated databases (though, to be honest, in its current form #419 is highly unlikely to ever be merged). Regardless, a user could utilize multiple databases which would bring this value into relevancy.

Please see the comment on the line above (per_key_connection_limit' => 20) for more details and thoughts on this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I answered the other comment, I agree that this is irrelevant if we have the other line.

);
$this->pool = new AsyncMysqlConnectionPool($options);
}
Expand Down