Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14559005:
Replace send packet throttling with a scheme based on number of in-flight bytes in IpcPacketSocket. (Closed)
|
Created:
7 years, 7 months ago by miu Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace send packet throttling with a scheme based on number of in-flight bytes in IpcPacketSocket.
This allows short bursts of high-rate sending without dropping packets, but quickly restricts the client to a sustainable steady-state rate.
Added two debug trace counters for visibility under the "p2p" category: P2PSendBytesAvailable and P2PSendPacketsInFlight.
BUG=237960
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198662
Patch Set 1 #
Total comments: 8
Patch Set 2 : s/production builds/release builds/ #Patch Set 3 : rebase #
Total comments: 10
Patch Set 4 : Addressed sergeyu's comments. #
Total comments: 4
Patch Set 5 : Final sergeyu comments addressed. #Patch Set 6 : kMaximumInFlightBytes set to 64KB, per discussions and testing observations. #Messages
Total messages: 14 (0 generated)
sergeyu/hclam: PTAL. Thanks, Yuri
https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:243: token_bucket_level_ -= data_size; I'm not sure how this helps. data_size is always going to be smaller than 1200 because it's packetized by WebRTC. When we get a burst of packets this behaviors like limiting by bytes isn't it? https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:350: // In production builds, auto-recover by resetting the throttling state. How is it related to production builds?
https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:23: const size_t kMaximumInFlightBytes = 256 << 10; // 256 KB This limit should be small. M27 limit is 40KB. We acutally don't want this to be too large so I think this value shouldn't be greater than 64KB which is what talk uses. The reason this should be small is because we want to react to network events or congestion due to IPC traffic quicker, such that we can react by pausing the encoder. https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:243: token_bucket_level_ -= data_size; An alternative is to do this decrement with a delayed task, this way we allow a sudden burst of packets in time but keep the cap over time. Being able to burst out a series of packets is critical to sending out unexpected large frames.
Alpha, Responded to code review comments. Also, please try this patch out on your end. From testing on my end, it was definitely solving the problem we discussed. If you'd like me to send a trace showing this, I can do that. Thanks, Yuri https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:23: const size_t kMaximumInFlightBytes = 256 << 10; // 256 KB On 2013/05/04 07:14:14, Alpha wrote: > This limit should be small. M27 limit is 40KB. We acutally don't want this to be > too large so I think this value shouldn't be greater than 64KB which is what > talk uses. > > The reason this should be small is because we want to react to network events or > congestion due to IPC traffic quicker, such that we can react by pausing the > encoder. I agree with you. One problem, though: When sending a 150KB key frame, if we start error'ing with EWOULDBLOCK after 64 KB worth of packets, will the encoder drop frames? That's the problem we're trying to solve here, correct? https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:243: token_bucket_level_ -= data_size; On 2013/05/04 04:11:20, Alpha wrote: > I'm not sure how this helps. > > data_size is always going to be smaller than 1200 because it's packetized by > WebRTC. When we get a burst of packets this behaviors like limiting by bytes > isn't it? Exactly. That's what I thought we had discussed over VC on Friday. We wanted to stop doing the packet-counting/limiting approach, and limit based on number of bytes instead. These are the goals (from your e-mail): > 1. We still has a cap of buffer limit in bytes The cap is set by the kMaximumInFlightBytes constant. I need guidance from the testing on your end on how we should tune this. > 2. We allow a burst of packets to be sent out This is a behavior we're getting from the token bucket approach. Packets can be sent at *infinite* rate up to the kMaximumInFlightBytes limit. Thereafter, the ACKs from the browser must be received in order to re-fill the token bucket. This means that, after the initial burst, the renderer will only be allowed to send at the rate the current network conditions allow for. > 3. Once the buffer overshoot the limit it returns EWOULDBLOCK This occurs once the token bucket has insufficient value. https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:243: token_bucket_level_ -= data_size; On 2013/05/04 07:14:14, Alpha wrote: > An alternative is to do this decrement with a delayed task, this way we allow a > sudden burst of packets in time but keep the cap over time. Being able to burst > out a series of packets is critical to sending out unexpected large frames. The two are identical approaches, except that the token bucket doesn't rely on the system clock (nor the additional code complexity of a callback-by-timer). Here's a summary of the two approaches: Delayed task scheme: Renderer can send at infinite rate for N seconds, and is then capped. Token bucket scheme: Renderer can send at infinite rate until byte limit, K, is reached. The two are related by a simple equation (assuming 20 Mbps is the bursty send rate): K = N * (20 Mbps / 8) So, if you set up a delayed task to run 100 ms after the burst begins, then K (the kMaximumInFlightBytes constant) should be 250 KB. https://codereview.chromium.org/14559005/diff/1/content/renderer/p2p/ipc_sock... content/renderer/p2p/ipc_socket_factory.cc:350: // In production builds, auto-recover by resetting the throttling state. On 2013/05/04 04:11:20, Alpha wrote: > How is it related to production builds? Wrong wording on my part. I meant: s/production/release/ Will fix.
Please fix the comment then LGTM.
Do I understand it correctly that this patch just changes buffer size to 256kB? I'm fine with that solution, but it's different from what we discussed on Friday. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:23: const size_t kMaximumInFlightBytes = 256 << 10; // 256 KB nit: *1024 is more readable than <<10. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:102: // A token bucket of bytes is used to throttle the sending of packets to the It doesn't look right to me that this is called "Token bucket". Traditional token bucket algorithm adds tokens on regular time intervals, but it's not the way this code works. The way it's implemented this variable just indicates available buffer size. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:107: size_t token_bucket_level_; maybe int instead of size_t, because it's not size? https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:226: if (data_size == 0) I think this can be DCHECK_GT(data_size, 0). https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:352: ResetSendThrottling(); Why do you need this given it's marked as NOTREACHED()? If we get into that state it means browser is broken, so i don't think it's worth trying to recover from this situation. Just crashing or calling OnError() would be better, IMO.
In this curent form yes. But changing this to buffer size is the first step anyway. Post M28 we can land changes that tune the buffer size or implement a lower soft limit easier. It will be great if we can land this prerequisite change first. We'll discuss with Patrik later today how to do the rest.
On 2013/05/06 22:30:04, Alpha wrote: > In this curent form yes. > > But changing this to buffer size is the first step anyway. Post M28 we can land > changes that tune the buffer size or implement a lower soft limit easier. It > will be great if we can land this prerequisite change first. We'll discuss with > Patrik later today how to do the rest. I'm fine with it, but lets not call it "token bucket".
https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:23: const size_t kMaximumInFlightBytes = 256 << 10; // 256 KB On 2013/05/06 22:18:02, Sergey Ulanov wrote: > nit: *1024 is more readable than <<10. Done. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:102: // A token bucket of bytes is used to throttle the sending of packets to the On 2013/05/06 22:18:02, Sergey Ulanov wrote: > It doesn't look right to me that this is called "Token bucket". Traditional > token bucket algorithm adds tokens on regular time intervals, but it's not the > way this code works. The way it's implemented this variable just indicates > available buffer size. Ah, yes. This did start out as a token bucket, where we were going to use 20 Mbps as the rate of adding tokens. Then, it collapsed into a simple "in-flight bytes" counter. Changed to send_bytes_available_. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:107: size_t token_bucket_level_; On 2013/05/06 22:18:02, Sergey Ulanov wrote: > maybe int instead of size_t, because it's not size? But it *is* a size: It's always equal to kMaximumInFlightBytes - SUM(in_flight_packet_sizes_). It can never become negative. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:226: if (data_size == 0) On 2013/05/06 22:18:02, Sergey Ulanov wrote: > I think this can be DCHECK_GT(data_size, 0). After thinking this over, I believe a NOTREACHED() would be most appropriate here. In debug builds, we want to crash so devs can make sure their code isn't trying to send empty packets (probably indicative of a bug in the calling code). However, in release builds, we should take an efficient no-op path. https://codereview.chromium.org/14559005/diff/14001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:352: ResetSendThrottling(); On 2013/05/06 22:18:02, Sergey Ulanov wrote: > Why do you need this given it's marked as NOTREACHED()? If we get into that > state it means browser is broken, so i don't think it's worth trying to recover > from this situation. Just crashing or calling OnError() would be better, IMO. Done. It's a CHECK() now.
On 2013/05/06 22:57:16, Sergey Ulanov wrote: > On 2013/05/06 22:30:04, Alpha wrote: > > In this curent form yes. > > > > But changing this to buffer size is the first step anyway. Post M28 we can > land > > changes that tune the buffer size or implement a lower soft limit easier. It > > will be great if we can land this prerequisite change first. We'll discuss > with > > Patrik later today how to do the rest. > > I'm fine with it, but lets not call it "token bucket". Right. It was a mistake to call it that. When we were going to hard-code an assumed send rate of 20 Mbps (meeting last Friday), it was going to be a token bucket based solution. However, hard-coding such a thing is dangerous: We really need some estimator of actual network bandwidth, one that dynamically adjusts to changing network conditions. I think what we have here should solve the immediate concern: allowing a sender (i.e., the caller of Send()) to be bursty, but still rate-limited to actual network capacity. By using a large "in-flight" buffer size, we're sacrificing a lot of the "instant feedback" mechanism in order to allow the sender to be bursty. In the future, IMHO, the sender should do smarter traffic shaping. It knows best when it's going to be bursty, and should be aware that UDP send buffers are not going big enough to handle the entire payload all at once. Along with that, then, the in-flight buffer size for IpcPacketSocket should be reduced down to provide more instant feedback to the sender.
lgtm when my comments are addressed. https://codereview.chromium.org/14559005/diff/21001/content/renderer/p2p/ipc_... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/21001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:135: void IpcPacketSocket::ResetSendThrottling() { Now this is called only when the socket is being initialized, so maybe move this code to constructor and remove this method? https://codereview.chromium.org/14559005/diff/21001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:352: << "Received SendComplete() with no known in-flight packets."; nit: remove the message. It makes release binaries bigger without giving anything in return.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/14559005/29001
https://codereview.chromium.org/14559005/diff/21001/content/renderer/p2p/ipc_... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/14559005/diff/21001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:135: void IpcPacketSocket::ResetSendThrottling() { On 2013/05/07 00:14:29, Sergey Ulanov wrote: > Now this is called only when the socket is being initialized, so maybe move this > code to constructor and remove this method? Done. https://codereview.chromium.org/14559005/diff/21001/content/renderer/p2p/ipc_... content/renderer/p2p/ipc_socket_factory.cc:352: << "Received SendComplete() with no known in-flight packets."; On 2013/05/07 00:14:29, Sergey Ulanov wrote: > nit: remove the message. It makes release binaries bigger without giving > anything in return. Done.
Message was sent while issue was closed.
Change committed as 198662 |
