Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1744293002:
Lazily allocate input buffer for AsyncTCPSocket. (Closed)
DescriptionLazily allocate input buffer for AsyncTCPSocket.
As a follow-up to https://codereview.webrtc.org/1737053006/ this CL further
improves memory usage by lazily allocating input buffers up to the passed
maximum size. This also changes the input buffer to a Buffer object.
BUG=
Committed: https://crrev.com/313afba2ebc702d2d6f1b871025f8a671a43ee8f
Cr-Commit-Position: refs/heads/master@{#11859}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Feedback from Tommi. #Patch Set 3 : Updated TODOs #
Messages
Total messages: 20 (5 generated)
jbauch@webrtc.org changed reviewers: + tommi@webrtc.org
Ptal, here is another memory optimization for tcp sockets.
https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc File webrtc/base/asynctcpsocket.cc (right): https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:35: // The input buffer will be resized up to the maximum size so that at least do you mean minimum? https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:211: // TODO: Do something better like forwarding the error to the user. can you assign the TODO to someone? https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:213: LOG(LS_ERROR) << "Recv() returned error: " << socket_->GetError(); nit: You can use LOG_IF() to combine the LOG and the check for blocking.
Please see my comments below. https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc File webrtc/base/asynctcpsocket.cc (right): https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:35: // The input buffer will be resized up to the maximum size so that at least On 2016/03/01 09:22:13, tommi-webrtc wrote: > do you mean minimum? No, the input buffer will be resized so that at least kMinimumRecvSize bytes can be received (but it will not grow above the maximum size passed to the constructor). Updated comment accordingly, I hope that's a bit clearer. https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:211: // TODO: Do something better like forwarding the error to the user. On 2016/03/01 09:22:13, tommi-webrtc wrote: > can you assign the TODO to someone? Assigned both TODOs in the file to "henrike" who committed these lines. https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:213: LOG(LS_ERROR) << "Recv() returned error: " << socket_->GetError(); On 2016/03/01 09:22:13, tommi-webrtc wrote: > nit: You can use LOG_IF() to combine the LOG and the check for blocking. Where is LOG_IF defined? I think it's only available in Chromium, but not in WebRTC - otherwise, which header should I include to get it?
lgtm with someone that's currently working on the code owning the TODO. https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc File webrtc/base/asynctcpsocket.cc (right): https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:35: // The input buffer will be resized up to the maximum size so that at least On 2016/03/01 10:55:23, joachim wrote: > On 2016/03/01 09:22:13, tommi-webrtc wrote: > > do you mean minimum? > > No, the input buffer will be resized so that at least kMinimumRecvSize bytes can > be received (but it will not grow above the maximum size passed to the > constructor). > > Updated comment accordingly, I hope that's a bit clearer. Thanks. https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:211: // TODO: Do something better like forwarding the error to the user. On 2016/03/01 10:55:23, joachim wrote: > On 2016/03/01 09:22:13, tommi-webrtc wrote: > > can you assign the TODO to someone? > > Assigned both TODOs in the file to "henrike" who committed these lines. henrike doesn't work on webrtc anymore actually. Can you assign to someone that has been working in this file recently? https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:213: LOG(LS_ERROR) << "Recv() returned error: " << socket_->GetError(); On 2016/03/01 10:55:23, joachim wrote: > On 2016/03/01 09:22:13, tommi-webrtc wrote: > > nit: You can use LOG_IF() to combine the LOG and the check for blocking. > > Where is LOG_IF defined? I think it's only available in Chromium, but not in > WebRTC - otherwise, which header should I include to get it? oh, sorry. fine as is.
https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc File webrtc/base/asynctcpsocket.cc (right): https://codereview.webrtc.org/1744293002/diff/1/webrtc/base/asynctcpsocket.cc... webrtc/base/asynctcpsocket.cc:211: // TODO: Do something better like forwarding the error to the user. On 2016/03/01 17:42:42, tommi-webrtc wrote: > On 2016/03/01 10:55:23, joachim wrote: > > On 2016/03/01 09:22:13, tommi-webrtc wrote: > > > can you assign the TODO to someone? > > > > Assigned both TODOs in the file to "henrike" who committed these lines. > > henrike doesn't work on webrtc anymore actually. Can you assign to someone that > has been working in this file recently? Last changes to that file were from stefan@ and pbos@, who do you suggest I should assign the TODOs to? https://chromium.googlesource.com/external/webrtc/+log/master/webrtc/base/asy...
let's go with stefan
Done
On 2016/03/02 16:23:13, joachim wrote: > Done Sorry, what's your policy on this? Can I submit this to CQ if I did (trivial) changes like here after getting a l-g-t-m with a request to change something minor?
On 2016/03/03 08:23:45, joachim wrote: > On 2016/03/02 16:23:13, joachim wrote: > > Done > > Sorry, what's your policy on this? Can I submit this to CQ if I did (trivial) > changes like here after getting a l-g-t-m with a request to change something > minor? If someone has commit rights, addresses all outstanding comments that accompany an lgtm without any concerns, then yes it's fine to upload a patch and send it to the cq. The delta should be trivial and assumed to be understood by both reviewer and author. It's also important in my opinion to go through the review console and the cq as opposed to landing directly via git, since we'll have better trackability and the reviewer can go back to the review to easily see what was landed.
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1744293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1744293002/40001
On 2016/03/03 09:26:44, tommi-webrtc wrote: > On 2016/03/03 08:23:45, joachim wrote: > > On 2016/03/02 16:23:13, joachim wrote: > > > Done > > > > Sorry, what's your policy on this? Can I submit this to CQ if I did (trivial) > > changes like here after getting a l-g-t-m with a request to change something > > minor? > > If someone has commit rights, addresses all outstanding comments that accompany > an lgtm without any concerns, then yes it's fine to upload a patch and send it > to the cq. The delta should be trivial and assumed to be understood by both > reviewer and author. It's also important in my opinion to go through the review > console and the cq as opposed to landing directly via git, since we'll have > better trackability and the reviewer can go back to the review to easily see > what was landed. Thanks for the clarification. So in that case (just changing the TODO) it would have been ok to send to CQ. Since there is a CQ for WebRTC I never had to land something manually and try to avoid that if possible.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1744293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1744293002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Lazily allocate input buffer for AsyncTCPSocket. As a follow-up to https://codereview.webrtc.org/1737053006/ this CL further improves memory usage by lazily allocating input buffers up to the passed maximum size. This also changes the input buffer to a Buffer object. BUG= ========== to ========== Lazily allocate input buffer for AsyncTCPSocket. As a follow-up to https://codereview.webrtc.org/1737053006/ this CL further improves memory usage by lazily allocating input buffers up to the passed maximum size. This also changes the input buffer to a Buffer object. BUG= Committed: https://crrev.com/313afba2ebc702d2d6f1b871025f8a671a43ee8f Cr-Commit-Position: refs/heads/master@{#11859} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/313afba2ebc702d2d6f1b871025f8a671a43ee8f Cr-Commit-Position: refs/heads/master@{#11859} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
