Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363013003:
Blimp: use dynamically resized decompression buffer vs. fixed size. (Closed)
|
Created:
4 years, 2 months ago by Kevin M Modified:
4 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: use dynamically resized decompression buffer vs. fixed size.
The current decompression code uses the maximum packet length as a
convenient upper bound for the maximum decompressed packet length.
When the length was raised to 20MB, this lazy convenience morphed
into a RAM suck.
This CL grows the decompression buffer across multiple calls to
inflate() until the entire input has been decompressed.
R=scf@chromium.org,lethalantidote@chromium.org
BUG=
Committed: https://crrev.com/36c922341b0da49622ea02fabc7ce73356d89c25
Cr-Commit-Position: refs/heads/master@{#421257}
Patch Set 1 #
Total comments: 14
Patch Set 2 : scf feedback #
Total comments: 4
Patch Set 3 : lethalantidote feedback #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Blimp: use dynamically resized compression buffer vs. fixed size. Blimp: use dynamically resized compression buffer vs. fixed size. The current compression code uses the maximum packet length as a convenient upper bound for the maximum compressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the compression buffer across multiple calls to inflate() until the entire input has been compressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ========== to ========== Blimp: use dynamically resized compression buffer vs. fixed size. The current compression code uses the maximum packet length as a convenient upper bound for the maximum compressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the compression buffer across multiple calls to inflate() until the entire input has been compressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ==========
It would be nice if somehow this area could be unit tested. There are a few corner cases that would be nice to exercise. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.cc (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:24: constexpr double kDecompressionGrowthFactor = 2.0; How did you get this magic number? My intuition tells me that usually compressed data is 10x smaller so we are going to loop through the code on average 4 times before bailing out. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:106: decompressed_buf->set_offset(decompressed_buf->offset() + bytes_out); Would be nice if GrowableIOBuffer had a DidConsume(...) api for symmetry as well https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:120: decompressed_buf->SetCapacity(kDecompressionGrowthFactor * should you cap it at some point? i.e. if kDecompressionGrowthFactor * decompressed_buf->capacity() is already higher than MaxPacketPayloadSizeBytes https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.h (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.h:52: const scoped_refptr<net::GrowableIOBuffer>& decompressed_buf, Noob question, but I thought passing smart pointers by ref were not encouraged. https://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.h:53: int size); minor nit: but I would move size closer to |compressed_buf| and rename to |size_compressed|
Regarding tests: there is already a pretty big suite of tests which, among other things, exercise the size limit boundary cases. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.cc (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:24: constexpr double kDecompressionGrowthFactor = 2.0; On 2016/09/23 22:14:18, scf wrote: > How did you get this magic number? My intuition tells me that usually compressed > data is 10x smaller so we are going to loop through the code on average 4 times > before bailing out. We've instrumented real-world compression rates of 50-60%. The same pipe is used for transmitting compressed image data, which has too much entropy to [re]compress well. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:106: decompressed_buf->set_offset(decompressed_buf->offset() + bytes_out); On 2016/09/23 22:14:18, scf wrote: > Would be nice if GrowableIOBuffer had a DidConsume(...) api for symmetry as well That whole API is a big pile of... asymmetry. >:F https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:120: decompressed_buf->SetCapacity(kDecompressionGrowthFactor * On 2016/09/23 22:14:18, scf wrote: > should you cap it at some point? > > i.e. if kDecompressionGrowthFactor * decompressed_buf->capacity() is already > higher than MaxPacketPayloadSizeBytes > > Added std::min() check so that we can check for cases where the payload size is somewhere in between CURRENT_SIZE and an illegal CURRENT_SIZE*2 https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.h (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.h:52: const scoped_refptr<net::GrowableIOBuffer>& decompressed_buf, On 2016/09/23 22:14:18, scf wrote: > Noob question, but I thought passing smart pointers by ref were not encouraged. > > https://www.chromium.org/developers/smart-pointer-guidelines IOBuffers are one of the rare cases where smart pointers are mandatory. IOBuffers are designed to be shared across threads, and ensuring proper destruction if the originator is destroyed on a different thread than the IO buffer is being filled on is... not fun. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.h:53: int size); On 2016/09/23 22:14:18, scf wrote: > minor nit: but I would move size closer to |compressed_buf| and rename to > |size_compressed| Good idea.
The CL description is a little confusing. It seems like this CL is more bounding the uncompressed buffer rather than the compressed buffer. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.h (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.h:45: // |decompressed_buf|. The size of |compressed_buf| is specified nit: Add a period at the end of this sentence.
Can you specify which parts of the description are confusing? Upon re-reading it, I only see references to compression and compression buffers. https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.h (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.h:45: // |decompressed_buf|. The size of |compressed_buf| is specified On 2016/09/23 23:17:22, CJ wrote: > nit: Add a period at the end of this sentence. Oops, forgot to complete this sentence.
On 2016/09/23 23:27:57, Kevin M wrote: > Can you specify which parts of the description are confusing? Upon re-reading > it, I only see references to compression and compression buffers. > > https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... > File blimp/net/compressed_packet_reader.h (right): > > https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... > blimp/net/compressed_packet_reader.h:45: // |decompressed_buf|. The size of > |compressed_buf| is specified > On 2016/09/23 23:17:22, CJ wrote: > > nit: Add a period at the end of this sentence. > > Oops, forgot to complete this sentence. But aren't we decompressing in this CL?
LOL! You have a point.
Description was changed from ========== Blimp: use dynamically resized compression buffer vs. fixed size. The current compression code uses the maximum packet length as a convenient upper bound for the maximum compressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the compression buffer across multiple calls to inflate() until the entire input has been compressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ========== to ========== Blimp: use dynamically resized decompression buffer vs. fixed size. The current decompression code uses the maximum packet length as a convenient upper bound for the maximum decompressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the decompression buffer across multiple calls to inflate() until the entire input has been decompressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ==========
https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... File blimp/net/compressed_packet_reader.cc (right): https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... blimp/net/compressed_packet_reader.cc:86: // Repeatedly compress |drainable_input| until it's fully consumed, growing Are we compressing or decompressing it here? I feel like drainable_input is being decompressed. https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... File blimp/net/compressed_packet_reader.h (right): https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... blimp/net/compressed_packet_reader.h:52: scoped_refptr<net::GrowableIOBuffer> compressed_buf, Why are we passing this in now? If it was a public function, I'd figure for testability, but since it's not, the value isn't quite clear.
Thanks for keeping me honest, CJ. ;) https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... File blimp/net/compressed_packet_reader.cc (right): https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... blimp/net/compressed_packet_reader.cc:86: // Repeatedly compress |drainable_input| until it's fully consumed, growing On 2016/09/23 23:42:16, CJ wrote: > Are we compressing or decompressing it here? I feel like drainable_input is > being decompressed. Done. https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... File blimp/net/compressed_packet_reader.h (right): https://codereview.chromium.org/2363013003/diff/20001/blimp/net/compressed_pa... blimp/net/compressed_packet_reader.h:52: scoped_refptr<net::GrowableIOBuffer> compressed_buf, On 2016/09/23 23:42:16, CJ wrote: > Why are we passing this in now? If it was a public function, I'd figure for > testability, but since it's not, the value isn't quite clear. Done.
The CQ bit was checked by scf@chromium.org
The CQ bit was unchecked by scf@chromium.org
lgtm https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_reader.cc (right): https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:24: constexpr double kDecompressionGrowthFactor = 2.0; On 2016/09/23 23:16:42, Kevin M wrote: > On 2016/09/23 22:14:18, scf wrote: > > How did you get this magic number? My intuition tells me that usually > compressed > > data is 10x smaller so we are going to loop through the code on average 4 > times > > before bailing out. > > We've instrumented real-world compression rates of 50-60%. The same pipe is used > for transmitting compressed image data, which has too much entropy to > [re]compress well. nice. thanks for the info https://codereview.chromium.org/2363013003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_reader.cc:120: decompressed_buf->SetCapacity(kDecompressionGrowthFactor * On 2016/09/23 23:16:42, Kevin M wrote: > On 2016/09/23 22:14:18, scf wrote: > > should you cap it at some point? > > > > i.e. if kDecompressionGrowthFactor * decompressed_buf->capacity() is already > > higher than MaxPacketPayloadSizeBytes > > > > > > Added std::min() check so that we can check for cases where the payload size is > somewhere in between CURRENT_SIZE and an illegal CURRENT_SIZE*2 Acknowledged.
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Blimp: use dynamically resized decompression buffer vs. fixed size. The current decompression code uses the maximum packet length as a convenient upper bound for the maximum decompressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the decompression buffer across multiple calls to inflate() until the entire input has been decompressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ========== to ========== Blimp: use dynamically resized decompression buffer vs. fixed size. The current decompression code uses the maximum packet length as a convenient upper bound for the maximum decompressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the decompression buffer across multiple calls to inflate() until the entire input has been decompressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: use dynamically resized decompression buffer vs. fixed size. The current decompression code uses the maximum packet length as a convenient upper bound for the maximum decompressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the decompression buffer across multiple calls to inflate() until the entire input has been decompressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= ========== to ========== Blimp: use dynamically resized decompression buffer vs. fixed size. The current decompression code uses the maximum packet length as a convenient upper bound for the maximum decompressed packet length. When the length was raised to 20MB, this lazy convenience morphed into a RAM suck. This CL grows the decompression buffer across multiple calls to inflate() until the entire input has been decompressed. R=scf@chromium.org,lethalantidote@chromium.org BUG= Committed: https://crrev.com/36c922341b0da49622ea02fabc7ce73356d89c25 Cr-Commit-Position: refs/heads/master@{#421257} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/36c922341b0da49622ea02fabc7ce73356d89c25 Cr-Commit-Position: refs/heads/master@{#421257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
