-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Performance regression in 0.18 (at least compared to 0.12) #357
Comments
Looking into this a little bit more, I originally thought it was https://github.com/whatyouhide/xandra/blob/main/lib/xandra/cluster/connection_pool.ex#L14-L20, which would explain the query time scaling with pool size, but it seems to not be the case. Even setting the pool size to 1 results in ~.7ms for the simple select case. I pulled some perf graphs (right click and download and open in a new table for zooming in, the svg is interactive), but I suspect something else is happening. |
Ah, this is super interesting, thank you @peixian! Fantastic report and investigation. 💟 I’m no expert in flame graphs, but from what I can tell a lot of the time in 0.18 is spent just taking the next (random) ID out of the set of free stream IDs. Basically, this code: {stream_id, data} =
get_and_update_in(data.free_stream_ids, fn ids ->
id = Enum.at(ids, 0)
{id, MapSet.delete(ids, id)}
end) I don't think we can get this to go much faster with the current data structures (a set of free stream IDs), but we could definitely store the IDs as a list and just take the head of the list every time, which should be significantly cheaper to do. Do you want to give that a try and measure perf for that? |
@whatyouhide @peixian , I also noticed the performance problem while fetching random ids. I solved it using
This is what I'm refering to: https://elixirforum.com/t/complexity-of-enum-functions/10493/3 |
Getting random IDs until you get a free one works for now but it's going to perform worse the less free IDs you have. Later on, let's go with the list, but let's do one thing at a time to avoid cluttering too much 🙃 |
@harunzengin I just benchmarked your fix, we're a lot closer now. 0.12.0 - 300 connections. First chunk is non-prepared statements, second chunk is prepared statements.
0.18.1 - 300 connections
0.18.1 - 5 connections
However, it seems like large pool sizes still incur a pretty significant slowdown, with also significantly lower throughput (1.7k rps drops to 1.3k when pool sizes go from 5 -> 300). I'm testing some fixes in connection selection, I suspect it has to do with something there. |
@peixian I don't know how you're testing this, but one important thing to keep in mind is this: the change in 0.18 was done to dramatically increase concurrent performance on a single connection. Before, one connection could only perform requests serially (but yes, with higher throughput since it was cheaper to make the single request). Now, a single connection can make multiple concurrent requests. So, an ideal comparison would be something like this:
|
@whatyouhide Ah yeah moving to multiplexed connections is the primary reason we're upgrading to We won't need 300 connections, but I suspect we'll still need a significant amount (~50-100), so figuring out the scaling costs with each additional connection will likely be important for us later on. I'm benching this with benchee, with running different selects for 5 seconds at maximum throughput. At least for us, individual performance is less important than the raw amount of thoughput a node can handle. I can do individual query performance as well, once I figure out a bit more on the pool scaling issue. Also waiting on Scylla to get back to me on if I they have info on stream_ids not returning for #356 |
The question is whether you're benching this with |
@peixian ping? |
@whatyouhide we're actively testing the latest changes now, on our production workload it seems like average query time has gone from .4ms to .6ms, which matches to what the benchmarks had. I think this is a slight regression, but the latest changes seem promising. I'm going to test with dialing the max concurrent requests per connection down and seeing if it changes later this week. |
I have another facet to the performance regressions in .18. Let me know if this should be a separate issue. The performance of large result sets has significantly worsened. One of the ones I'm looking at is about 100% slower, going from 400ms to 800ms. I believe this is because the new connection code uses active mode sockets and by default is receiving something like a single packet of data per message (byte_size says about 1400). For each tiny message, we append it into the buffer and try to parse a frame from it. I experimented a bit and while I don't know if this is the right way to solve this, if I set the For instance
Thoughts? |
I'm closing this, in our experiments the performance regression has disappeared with a combination of @harunzengin's and @kennethito's changes. |
That's metal news 🤘 |
Adding this in case other people run into it. I'm still trying to figure out what the root cause is, but it seems like configuring the pool size affects runtime latency on the newest version.
On 0.12.0, we were running with 300 connections to each Scylla node underneath. On 0.18.1, I tried it with 2 connections per node, as well as 300 connections per node in
pool_size
.0.12.0
, 300 connections:0.18.1
, 300 connections:0.18.1
, 5 connections:I also reordered and randomly ran the benchmark multiple times to make sure it wasn't some cache issue.
Configuration:
The text was updated successfully, but these errors were encountered: