-
Notifications
You must be signed in to change notification settings - Fork 70
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
Opt: split large packet to n*16KB packet #82
Conversation
Codecov Report
@@ Coverage Diff @@
## master #82 +/- ##
==========================================
+ Coverage 68.87% 69.14% +0.27%
==========================================
Files 8 8
Lines 1420 1439 +19
==========================================
+ Hits 978 995 +17
- Misses 347 348 +1
- Partials 95 96 +1
Continue to review full report at Codecov.
|
session.go
Outdated
writeSize += maxPacketLen | ||
} | ||
pkgs = append(pkgs, pkg[writeSize:]) | ||
lg, err := s.Connection.send(pkgs) |
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.
u need not to change the codes so quickly. pls test it firstly.
pr 我看了,没多大毛病。关键还是发送大包时,要不要加锁的问题。 |
session.go
Outdated
@@ -129,6 +130,7 @@ type session struct { | |||
// goroutines sync | |||
grNum uatomic.Int32 | |||
lock sync.RWMutex | |||
wlock sync.Mutex |
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.
u should use this wlock before every write action/funcs.
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.
addressed
session.go
Outdated
totalSize -= maxPacketLen | ||
writeSize += maxPacketLen | ||
} | ||
lg, err := s.Connection.send(pkg[writeSize:]) |
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.
@Lvnszn does it need to send if the totalSize = 0
?
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.
IMO, the packet needn't to be send. Addressed.
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.
@Lvnszn yes, should return if totalSize already decreased to 0
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.
Addressed.
session.go
Outdated
writeSize := 0 | ||
s.wlock.Lock() | ||
defer s.wlock.Unlock() | ||
for totalSize >= maxPacketLen { |
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.
totalSize change to leftPackageSize or packageSize should be more readable?
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.
addressed.
session.go
Outdated
} | ||
return lg, nil | ||
|
||
return writeSize + lg, nil |
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.
here can return totalsize directly ?
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.
add a var for return, PTAL.
session.go
Outdated
@@ -418,11 +422,28 @@ func (s *session) WriteBytes(pkg []byte) (int, error) { | |||
return 0, ErrSessionClosed | |||
} | |||
|
|||
lg, err := s.Connection.send(pkg) | |||
leftPackageSize, totalSize, writeSize := len(pkg), len(pkg), 0 | |||
s.wlock.Lock() |
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.
In my opinion, the wlock used for protect big package send, so may be u can change the wlock to rwMutex, and only when package size bigger than 16kb, try to get Lock, other times and other places you can use Rlock.
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.
good idea
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.
address
lg, err := s.Connection.send(pkg) | ||
leftPackageSize, totalSize, writeSize := len(pkg), len(pkg), 0 | ||
if leftPackageSize >= maxPacketLen { | ||
s.packetLock.Lock() |
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.
only leftPackageSize > maxPacketLen need lock, equal not needed?
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.
address
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.
LGTM
What this PR does:
split large packet to 16KB packet and send one by one for reduce risk when send large packet.
recv will compact small packet in a large one.
Which issue(s) this PR fixes:
Fixes #79
Special notes for your reviewer:
Does this PR introduce a user-facing change?: