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

Use keep alive connection for JSONRPC client #236

Closed
wants to merge 2 commits into from

Conversation

edubart
Copy link
Contributor

@edubart edubart commented Apr 29, 2024

This PR optimizes JSONRPC remote machine client to use keep alive connections when performing subsequent requests. This is an optimization that increases the amount of request throughput we can perform via remote machines, and also fixes possible issues about exhausting the amount of available client ports for an OS that does not support SO_LINGER socket workaround.

The SO_LINGER workaround is still available in the code, because it is still very useful when performing thousands of fork requests per second, where we cannot make full use of keep alive connections.

The improvement of performance is almost 2x, for instance, for the following loop:

  while true do
    machine:read_mcycle()
  end

These are the statistics in number of iterations per second:

JSONRPC (before this PR)  52500 req/s
JSONRPC ( after this PR)  95105 req/s
local                     19699651 req/s

Therefore this is about +81% of throughput improvement, still way below local machines, but fair enough.

Depends on

@edubart edubart added the enhancement New feature or request label Apr 29, 2024
@edubart edubart self-assigned this Apr 29, 2024
@edubart edubart force-pushed the feature/jsonrpc-keepalive branch 2 times, most recently from 086d6fc to be4abc2 Compare April 29, 2024 15:37
@edubart edubart linked an issue Apr 29, 2024 that may be closed by this pull request
@edubart edubart added the optimization Optimization label Apr 29, 2024
@edubart edubart requested review from diegonehab and a team April 29, 2024 15:42
@edubart edubart force-pushed the feature/jsonrpc-keepalive branch 2 times, most recently from 09a8b25 to 79f70e8 Compare April 29, 2024 15:59
Copy link
Contributor

@diegonehab diegonehab left a comment

Choose a reason for hiding this comment

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

Why does the client seem to support multiple connections open? Doesn't the client always talk to the same server? Does this have something to do with snapshot/rollback? If so, wouldn't it be simpler to keep a single connection open, the one to the top server?

@edubart
Copy link
Contributor Author

edubart commented Apr 30, 2024

Why does the client seem to support multiple connections open?

I made a keep alive connection pool without making assumptions on how the parent class is using it, so I would not have to rework it (fewer changes) and also allowed me to optimize the use case of snapshot/rollback. While it does support keeping alive connections for multiple servers, it won't be used that much, because we will have at maximum 2 keep alive connections left open. I put a check on the amount of maximum keep alive connections to not let connections leak in future code changes, to be explicit about this, and to make sure things are working correctly, would also feel wrong for me to leave a connection pool without an upper limit. In practice this check will always fail because at this moment the jsonrpc client cannot keep alive more than 2 simultaneous connections.

Doesn't the client always talk to the same server? Does this have something to do with snapshot/rollback? If so, wouldn't it be simpler to keep a single connection open, the one to the top server?

Most times the class has one connection open, except when you do a snapshot(), in this case we will have 2 connections open until commit() or rollback() is called. Keeping two connections open we save one extra reconnection when we are done with the snapshot. So this is an optimization to speed up the snapshot/commit/rollback use case.

I lowered the MAX_KEEP_ALIVE_POOL to 2 and made a comment about this, to make more clear.

@edubart edubart requested review from diegonehab and a team April 30, 2024 15:39
@diegonehab
Copy link
Contributor

I was just wondering if this could have been a much simpler/smaller patch with more or less the same performance if you kept a single open connection.

@edubart
Copy link
Contributor Author

edubart commented Apr 30, 2024

I was just wondering if this could have been a much simpler/smaller patch with more or less the same performance if you kept a single open connection.

It could be a little smaller with the downside of having a slightly slower commit/rollback, because one extra reconnection. I think the patch is already simple/small enough.

@edubart
Copy link
Contributor Author

edubart commented May 13, 2024

This PR was superseded by #240

@edubart edubart closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request optimization Optimization
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Optimize jsonrpc request throughput
2 participants