Optimization of connections and database

[juanrossi]

Hello, this pull request should improve the database schema and making correct use of database types (For example session and use of INDEX [#548]).

In HHVM the pool of MySQL connections is created by request, which is now opening between 5 and 15 connections depending on the section. This generated a big overhead to the database, which will result in max_allowed_connections errors with 50 concurrent users.

Changing the idle timeout of pool from 0.2 to 2 seconds will result in reutilization of connections instead of generating new ones and 20 max connections to make sure the db is not stressed (Default is 50).

Let me know if have any questions!

[justinwray]

@juanrossi Thank you for the submission! We will review and test the database optimization changes submitted here and then provide any feedback we might have.

[stevcoll]

@juanrossi Great work! I noticed a significant speed increase while using the platform. Testing looks good. @justinwray ready for your review.

[wisco24]

How long is it going to take to get this merged in??!?!?!? This issue was identified a long time ago but the admin's didn't believe it. Please test and approve this ASAP.

[justinwray]

@juanrossi

We have had a chance to test and review your submission. The modifications to the schema are great. Specifically, the indexing provides a valuable optimization with very little overhead.

The one area we are concerned about within this PR is the inclusion of persistent connections. While on the surface persistent connections sound like a performance improvement they bring along a large number of downfalls. In fact, often the use of persistent connections can negatively impact performance. These issues are well researched, so take a look at: http://php.net/manual/en/features.persistent-connections.php and https://meta.wikimedia.org/wiki/Why_persistent_connections_are_bad.

Generally, one would utilize persistent connections when the TCP/IP overhead of the database connections are increasing latency. Given that the most common deployment of the platform has the MySQL server on the very same system, the connections, in this case, are all contained locally. In other words, there is almost no TCP/IP overhead in the most common deployment. Even still, if one were to deploy the database to a different server (using the new installation option, or via a cloud solution like RDS), the distance from the platform stack and the database should be relatively small. In the extreme cases where a deployment is setup with a database that is significantly far (network path wise) from the rest of the stack, persistent connections may provide some benefit.

Thankfully, as you outlined in your Issue (#548), we are caching the database responses in Memcached. Even sessions queries are cached. So the number of connections to MySQL directly utilized is fairly minimal.

As such, we believe it would be better to leave the inclusion of persistent connections left up to the end-user. We believe persistent connections to be an uncommon option and in most cases to have negative consequences.

Could you update this PR with the removal of the persistent connection change, or make it optional (through the provisioning process)?

[fredemmott]

Is this actually enabling persistent connections? I see the option is being set //for mysqli//, but according to github search, this project isn't using mysqli (though options are also set in a few other places), and the mysqli options shouldn't affect the async mysql extension

[juanrossi]

Hi @justinwray, thanks for the review and information.

Have in mind that right now the framework (CTF) and HHVM is generating several connections per request (Between 5 to 15 for EACH request) which is a lot. With sessions and memcached I saw between 4 to 7 new connections being created in the game (around 20 in total, but 15 being reused). This generates a lot of overhead.

So, we can stop using async connections and make a change to Db.php:genConnection to something like this (Without the Async):

public async function genConnection(): Awaitable<AsyncMysqlConnection> {
    if(!$this->isConnected()) {
        await $this->genConnect();
        invariant($this->conn !== null, 'Connection cant be null.');
        return $this->conn;
    }
    return $this->conn;
}

This will create one connection and re-use (with Async I saw a few errors, so this is not going to work directly with that change).

The other option is to embrace the persistent connections (which is not bad, you just have to use them correctly), and reuse them when possible.

You can see in HHVM MySQL documentation that we are creating a pool of connections for each request. And you can also validate this in the game adding var_dump($this->pool->getPoolStats()); after setting $this->conn.

Let me know if you have questions.

[fredemmott]

Memoizing async connections like that is definitely unsafe, given that an async function can make another mysql request while another is waiting for a response.

[juanrossi]

@fredemmott That's correct, that's why we shouldn't use that code with Async, it was just as an example.

I think we should use async (with persistent connections which HHVM uses), or change ALL the queries to use synchronous connections. I'd go for the first option.

[fredemmott]

Yeah, I'm fine with us using async mysql's pool, that's pretty much a requirement (but we need to be sure there's nothing setting options on the connection or otherwise mutating state)

This is what confuses me:

+# Use persistent connections and higher timeout
 +mysqli.reconnect = 1
 +mysqli.allow_persistent = On
 +mysqli.max_persistent = 400

What are those doing on FBCTF, given FBCTF doesn't use mysqli?

[justinwray]

@fredemmott
Is this actually enabling persistent connections? I see the option is being set //for mysqli//, but according to github search, this project isn't using mysqli (though options are also set in a few other places), and the mysqli options shouldn't affect the async mysql extension

This is a good point. The change is impacting mysqli, not AsyncMysqlConnectionPool.

@juanrossi
This will create one connection and re-use (with Async I saw a few errors, so this is not going to work directly with that change).

Please do not misunderstand, I have nothing against persistent connections. When used properly they can be valuable. (MediaWiki used the term 'bad')

@fredemmott
Memoizing async connections like that is definitely unsafe, given that an async function can make another mysql request while another is waiting for a response.

Agreed. This sort of re-use introduces a lot of hard-to-track bugs and worse complete lock ups in some cases (or at least such is true in mysqli). Same goes for persistent connections in general, when not throughly planned around.

@juanrossi:

We are using AsyncMysqlConnectionPool which should be giving us re-use of connections in the pool:

The connection pool will either create a new connection or use one of the recently available connections from the pool itself.

Much like you would experience with persistent connections in native mysqli.

[juanrossi]

Hi guys, after reading some posts, docs and issues I thought it was necessary, but I might misunderstood that.

If you are sure it's not required and we are not using MySQLi, I'll remove those lines.

[justinwray]

@juanrossi

Given the very clear benefits of the indexing, and the contention over persistent connections: Let's remove the persistent connections from this PR and we will merge the schema updates.

A discussion can then be started in Issues to further discuss the implementation and implications of persistent connections.

[juanrossi]

@justinwray sorry for the delay, I pushed the commit removing MySQLi vars.

Let me know if you think we need to add more changes to this!

[juanrossi]

Hi @justinwray, did you have the chance to review this changes? Thanks.

[justinwray]

@juanrossi Thank you again for your submission!

Overall, the schema changes are excellent and provide a significant improvement in the performance of the platform.

We have been developing and testing a notable set of performance improvements, which will be submitted soon, and your schema changes (as well as a bit more indexing) have been included in that development and testing. So the new indexing has been well tested and has proven to be very beneficial.

Please take a look at the specific review items and see if you can provide feedback or make any changes necessary.

[justinwray]

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.

[juanrossi]

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.

[justinwray]

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).

[juanrossi]

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).

[justinwray]

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.

[juanrossi]

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.

[justinwray]

'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?

[juanrossi]

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.

[justinwray]

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?

[juanrossi]

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.

[justinwray]

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.

[juanrossi]

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

[justinwray]

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

[juanrossi]

Hi @justinwray, sorry for the looong delay, I never saw the Github notification. I saw that many of the db improvements were merged in #594 which is great!

Let me know what do you think of my comments so we can close this asap. Thanks!

[juanrossi]

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.

[juanrossi]

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).

[juanrossi]

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.

[juanrossi]

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.

[juanrossi]

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.

[juanrossi]

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

[wisco24]

Any idea when this will be merged? Getting close to 6 months....

[juanrossi]

Hi @wisco24, although it's right it's been a while, most of the optimizations have been implemented in PR #594.