-
Notifications
You must be signed in to change notification settings - Fork 473
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
fix: optimize TCP connection establishment time #3032
fix: optimize TCP connection establishment time #3032
Conversation
Thanks for opening this. We already set
I think you may only be testing locally here. The 400ms time is between data centres in us-east-1 and us-west-1. |
@@ -0,0 +1,54 @@ | |||
# TCP Connection Time Test Results |
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.
There is no need to submit files like this. Please add your findings to the PR and it'll be in the git history instead for interested parties.
@@ -4,7 +4,7 @@ export const CODE_CIRCUIT = 290 | |||
export const CODE_UNIX = 400 | |||
|
|||
// Time to wait for a connection to close gracefully before destroying it manually | |||
export const CLOSE_TIMEOUT = 500 | |||
export const CLOSE_TIMEOUT = 250 |
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.
Why is this necessary?
@@ -0,0 +1,74 @@ | |||
/** |
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.
This is a TypeScript project, please only submit TypeScript files.
socket.end() | ||
|
||
// Verify connection time is under threshold | ||
expect(connectionTime).to.be.below(300, 'Connection time should be under 300ms') |
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.
This only runs locally so yes, the connection time will be significantly under 300ms.
} | ||
}) | ||
|
||
it('should establish a libp2p connection in under 300ms', async () => { |
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'm not sure this test works as expected - the nodes don't have any listen addresses, stream muxers or connection encrypters configured.
Did you run this before submitting the PR?
@@ -0,0 +1,87 @@ | |||
/** |
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.
No need to add packageDocumentation
tags - test files are not part of the generated docs and besides, you should only have one packageDocumentation
tag per module - the one for @libp2p/tcp
is in the /src/index.ts
file.
packages/transport-tcp/src/tcp.ts
Outdated
@@ -121,6 +121,10 @@ export class TCP implements Transport<TCPDialEvents> { | |||
...options | |||
}) as (IpcSocketConnectOpts & TcpSocketConnectOpts) | |||
|
|||
// Set TCP_NODELAY to true to disable Nagle's algorithm | |||
// This reduces latency by sending data immediately without buffering | |||
cOpts.noDelay = true |
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.
noDelay
is already set here
|
||
// Ensure TCP_NODELAY is set to true to disable Nagle's algorithm | ||
// This reduces latency by sending data immediately without buffering | ||
socket.setNoDelay(true) |
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.
noDelay
is already set here
@@ -0,0 +1,87 @@ | |||
/** |
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.
This is a TypeScript project, please only submit TypeScript files.
@@ -0,0 +1,131 @@ | |||
/** |
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.
This is a TypeScript project, please only submit TypeScript files.
I'm closing this PR because all it does is change the default close timeout. The problem observed in #3029 occurs during connection establishment, not while closing the connection. Please open a new PR if you find a solution to the problem. |
Fix: Optimize TCP Connection Establishment Time
Problem
Connection establishment time increased from ~0.2s in TCP v9 to ~0.4s in TCP v10 (as reported in issue #3029).
Changes Made
CLOSE_TIMEOUT
from 500ms to 250ms inconstants.ts
noDelay: true
to socket configuration intcp.ts
to disable Nagle's algorithmsocket.setNoDelay(true)
insocket-to-conn.ts
to ensure consistent socket configurationTesting
Manual testing shows connection times of ~15-19ms, significantly faster than the previous 400ms.
Related Issue
Fixes #3029
Change checklist