Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1867653002:
Initial version of Blimp BlobCache. (Closed)
|
Created:
4 years, 8 months ago by nyquist Modified:
4 years, 8 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, cc-bugs_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial version of Blimp BlobCache.
Currently, every time there is a commit from engine to client, all
pictures are sent again, leading to poor performance and high
bandwidth usage.
This CL adds a cache on the client side that never clears out any
received resources. This means that the engine can depend on that
the client has all resources it has previously sent, and uses this fact
to only send down a unique identifier (SHA1 hash of the image) and the
the width and height in place of the image itself. This is wrapped in
the new proto BlobCacheImageMetadata. If this would be a new image for
the client, the proto also includes the payload of the image. This
payload will be removed when the BlobChannel is fully integrated.
The identifier is calculated based on the source image, to ensure that
no encode is required on the engine when the same image is seen again.
To ensure that the cache is used for all images, this changes the
behavior of the serializer to also encode images that were previously
encoded as WebP, to ensure that they become cacheable.
BUG=597807, 600719, 603353
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8
Cr-Commit-Position: refs/heads/master@{#388013}
Patch Set 1 #Patch Set 2 : Some small changes to make things more better. #Patch Set 3 : Now the cache fully works. #Patch Set 4 : git merge origin/master #Patch Set 5 : Refactored to new BlobCache interface #Patch Set 6 : Rename to InfiniteBlobCache #Patch Set 7 : self-review #Patch Set 8 : Undo previous merge-issue and change from |sha1| to |id| in proto field. #Patch Set 9 : Update test name to new API #
Total comments: 53
Patch Set 10 : Addressed all comments. #
Total comments: 26
Patch Set 11 : Addressed comments from kmarshall, except swap #
Total comments: 33
Patch Set 12 : Addressed all comments #Patch Set 13 : Changed to use BlobId and BlobData #
Total comments: 7
Patch Set 14 : Changed BlobId from 'const std::string' to 'std::string' #
Total comments: 16
Patch Set 15 : Addressed last comments in review #Patch Set 16 : Renamed blob_cache sha1_util to id_util #Patch Set 17 : git merge origin/master for good measure #
Total comments: 1
Messages
Total messages: 48 (19 generated)
Description was changed from ========== Initial version of Blimp BlobCache. BUG=597807 ========== to ========== Initial version of Blimp BlobCache. BUG=597807 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Initial version of Blimp BlobCache. BUG=597807 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageIdentifier. In this version, the client is not informed of the SHA1 from the server the first time an image is sent down, and instead it takes the bytes and generates it itself. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
nyquist@chromium.org changed reviewers: + kmarshall@chromium.org, vmpstr@chromium.org
kmarshall, vmpstr: PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867653002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867653002/160001
Description was changed from ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageIdentifier. In this version, the client is not informed of the SHA1 from the server the first time an image is sent down, and instead it takes the bytes and generates it itself. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageIdentifier. In this version, the client is not informed of the SHA1 from the server the first time an image is sent down, and instead it takes the bytes and generates it itself. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1867653002/diff/160001/blimp/client/feature/c... File blimp/client/feature/compositor/decoding_image_generator.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:18: // Check if this is a cache identifier instead of a WebP image. Can you add a prefix byte instead of trying to parse? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:12: #include "base/memory/scoped_ptr.h" Remove https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:14: #include "base/memory/scoped_ptr.h" Remove https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:6: #include <vector> Add newline after <vector> https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:17: scoped_refptr<RefCountedVector> Create(const char* data, size_t length) { Just take a const std::string&? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:24: bool Equal(scoped_refptr<RefCountedVector> a, You can use std::equal() to check if the vectors are equal. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:34: Also test duplicate puts https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:35: TEST(InfiniteBlobCacheTest, SimplePutContainsAndGetOperations) { Convention: create a testing::Test class and use TEST_F instead. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:36: std::unique_ptr<InfiniteBlobCache> cache = Move cache into testing base class https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:39: EXPECT_FALSE(cache->Contains("foo")); Move string literals into kConstants https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:22: std::string ToSHA1HexString(const void* data, size_t data_size) { What about using binary SHA1 for transmission and just converting to human-readable hex for display? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:46: WebPData data = {reinterpret_cast<const uint8_t*>(input), input_size}; Pick a better name? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:46: WebPData data = {reinterpret_cast<const uint8_t*>(input), input_size}; Brace initialization is blacklisted. Sowwy. :( https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:49: LOG(WARNING) << "Throwing away tiny input with size " << input_size; How about DLOG? Is this actionable for release builds? Can you add a comment about why we might be seeing tiny inputs in the first place? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:53: bool found_in_cache = false; Move this declaration down, just above the ParseFromArray block. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:126: for (size_t i = 0; i < data.size; ++i) Use braces for single lines (consistent with rest of blimp) https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:128: g_blob_cache.Get().Put(sha1, to_cache); std::move(to_cache) https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:27: std::string ToSHA1HexString(const void* data, size_t data_size) { Break this out into a library for sharing with client/ https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:35: std::set<std::string> g_client_cache_contents; Use LazyInstance https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:104: // Copy WebP data into SkData. |data| is allocated only on the stack, so Move comment to top of block or add newline before comment. Note that this branch indicates cache miss. https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:108: } else { Add comment about cache hit https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:118: // Copy cache identifier proto into SkData. Newline before comment.
https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:17: typedef base::RefCountedData<std::vector<uint8_t>> RefCountedVector; I prefer "using RefCountedVector = base::RefCountedData<std::vector<uint8_t>>;" but up to you. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:22: class BLIMP_COMMON_EXPORT InfiniteBlobCache : public BlobCache { Since there's only one implementation of BlobCache.. and that implementation is just std::map, do you think it might be worth it to just use an std::map wherever you'd use this BlobCache? That's something that we can refactor later if there will be more than one cache... There are also some base/ caches like MRUCache that you might use... Basically, I'm not sure I would add another cache class since I don't see any benefit over existing maps. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:19: for (size_t i = 0; i < length; ++i) does "entry->data.assign(data, data + length);" work here? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:20: entry.get()->data.push_back(static_cast<uint8_t>(data[i])); you don't need .get() https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:24: bool Equal(scoped_refptr<RefCountedVector> a, On 2016/04/13 23:29:52, Kevin M wrote: > You can use std::equal() to check if the vectors are equal. Yeah.. isn't this just "a->data == b->data"? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:46: WebPData data = {reinterpret_cast<const uint8_t*>(input), input_size}; On 2016/04/13 23:29:52, Kevin M wrote: > Brace initialization is blacklisted. Sowwy. :( Soon though, right? I think pkasting might be trying to allow it.
Description was changed from ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageIdentifier. In this version, the client is not informed of the SHA1 from the server the first time an image is sent down, and instead it takes the bytes and generates it itself. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImage. If this would be a new image for the client, the proto also includes the payload of the image. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImage. If this would be a new image for the client, the proto also includes the payload of the image. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImage. If this would be a new image for the client, the proto also includes the payload of the image. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719, 603353 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
kmarshall, vmpstr: PTAL https://codereview.chromium.org/1867653002/diff/160001/blimp/client/feature/c... File blimp/client/feature/compositor/decoding_image_generator.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:18: // Check if this is a cache identifier instead of a WebP image. On 2016/04/13 23:29:52, Kevin M wrote: > Can you add a prefix byte instead of trying to parse? Moved all of this to a proto instead. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:12: #include "base/memory/scoped_ptr.h" On 2016/04/13 23:29:52, Kevin M wrote: > Remove Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:17: typedef base::RefCountedData<std::vector<uint8_t>> RefCountedVector; On 2016/04/13 23:48:02, vmpstr wrote: > I prefer "using RefCountedVector = base::RefCountedData<std::vector<uint8_t>>;" > but up to you. Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:14: #include "base/memory/scoped_ptr.h" On 2016/04/13 23:29:52, Kevin M wrote: > Remove Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:22: class BLIMP_COMMON_EXPORT InfiniteBlobCache : public BlobCache { On 2016/04/13 23:48:02, vmpstr wrote: > Since there's only one implementation of BlobCache.. and that implementation is > just std::map, do you think it might be worth it to just use an std::map > wherever you'd use this BlobCache? > > That's something that we can refactor later if there will be more than one > cache... There are also some base/ caches like MRUCache that you might use... > > Basically, I'm not sure I would add another cache class since I don't see any > benefit over existing maps. The plan is definitely later to use a different implementation, and the very-very-soon plan is to use this implementation to flesh out the rest of the BlobChannel feature so we can get closer to an asynchronous world of displaying/sending images. So I'd like to keep this in here for now. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:6: #include <vector> On 2016/04/13 23:29:52, Kevin M wrote: > Add newline after <vector> Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:17: scoped_refptr<RefCountedVector> Create(const char* data, size_t length) { On 2016/04/13 23:29:52, Kevin M wrote: > Just take a const std::string&? Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:19: for (size_t i = 0; i < length; ++i) On 2016/04/13 23:48:02, vmpstr wrote: > does "entry->data.assign(data, data + length);" work here? > Done, I think? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:20: entry.get()->data.push_back(static_cast<uint8_t>(data[i])); On 2016/04/13 23:48:02, vmpstr wrote: > you don't need .get() Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:24: bool Equal(scoped_refptr<RefCountedVector> a, On 2016/04/13 23:48:02, vmpstr wrote: > On 2016/04/13 23:29:52, Kevin M wrote: > > You can use std::equal() to check if the vectors are equal. > > Yeah.. isn't this just "a->data == b->data"? Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:34: On 2016/04/13 23:29:52, Kevin M wrote: > Also test duplicate puts Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:35: TEST(InfiniteBlobCacheTest, SimplePutContainsAndGetOperations) { On 2016/04/13 23:29:52, Kevin M wrote: > Convention: create a testing::Test class and use TEST_F instead. Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:36: std::unique_ptr<InfiniteBlobCache> cache = On 2016/04/13 23:29:52, Kevin M wrote: > Move cache into testing base class Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:39: EXPECT_FALSE(cache->Contains("foo")); On 2016/04/13 23:29:52, Kevin M wrote: > Move string literals into kConstants Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:22: std::string ToSHA1HexString(const void* data, size_t data_size) { On 2016/04/13 23:29:52, Kevin M wrote: > What about using binary SHA1 for transmission and just converting to > human-readable hex for display? Done. For now I also keep the cache in hex-format, but transmission is in bytes like you said here. Is that OK? https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:46: WebPData data = {reinterpret_cast<const uint8_t*>(input), input_size}; On 2016/04/13 23:48:02, vmpstr wrote: > On 2016/04/13 23:29:52, Kevin M wrote: > > Brace initialization is blacklisted. Sowwy. :( > > Soon though, right? I think pkasting might be trying to allow it. Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:49: LOG(WARNING) << "Throwing away tiny input with size " << input_size; On 2016/04/13 23:29:52, Kevin M wrote: > How about DLOG? Is this actionable for release builds? > > Can you add a comment about why we might be seeing tiny inputs in the first > place? Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:53: bool found_in_cache = false; On 2016/04/13 23:29:52, Kevin M wrote: > Move this declaration down, just above the ParseFromArray block. Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:126: for (size_t i = 0; i < data.size; ++i) On 2016/04/13 23:29:52, Kevin M wrote: > Use braces for single lines (consistent with rest of blimp) Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:128: g_blob_cache.Get().Put(sha1, to_cache); On 2016/04/13 23:29:53, Kevin M wrote: > std::move(to_cache) Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:27: std::string ToSHA1HexString(const void* data, size_t data_size) { On 2016/04/13 23:29:53, Kevin M wrote: > Break this out into a library for sharing with client/ Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:35: std::set<std::string> g_client_cache_contents; On 2016/04/13 23:29:53, Kevin M wrote: > Use LazyInstance Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:104: // Copy WebP data into SkData. |data| is allocated only on the stack, so On 2016/04/13 23:29:53, Kevin M wrote: > Move comment to top of block or add newline before comment. > > Note that this branch indicates cache miss. Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:108: } else { On 2016/04/13 23:29:53, Kevin M wrote: > Add comment about cache hit Done. https://codereview.chromium.org/1867653002/diff/160001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:118: // Copy cache identifier proto into SkData. On 2016/04/13 23:29:53, Kevin M wrote: > Newline before comment. Done.
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867653002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867653002/180001
https://codereview.chromium.org/1867653002/diff/180001/blimp/client/feature/c... File blimp/client/feature/compositor/decoding_image_generator.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:18: std::unique_ptr<BlobCacheImage> deserialized(new BlobCacheImage); "parsed_metadata"? https://codereview.chromium.org/1867653002/diff/180001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:28: SkImageInfo::MakeN32Premul(deserialized->width(), deserialized->height()); Inline this in the DecodingImageGenerator ctor call https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:10: #include <vector> Necessary? https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:21: class BLIMP_COMMON_EXPORT InfiniteBlobCache : public BlobCache { InMemoryBlobCache (suggested by Wez because the name will still be correct when the cache becomes finite.) https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:24: scoped_refptr<RefCountedVector> Create(const std::string& data) { CreateRefCountedVector? https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... File blimp/common/blob_cache/sha1_util.h (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/sha1_util.h:23: BLIMP_COMMON_EXPORT const std::string ToSHA1HexString(const void* data); It's hard to distinguish this -- which doesn't hash |data| -- from ToSHA1HashBytes, which does hash |data|. How about FormatSHA1Hash? https://codereview.chromium.org/1867653002/diff/180001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:55: DCHECK(deserialized->id().length() == base::kSHA1Length); DCHECK_EQ https://codereview.chromium.org/1867653002/diff/180001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:56: std::string hex_id = blimp::ToSHA1HexString(deserialized->id().c_str()); Why are we using a different encoding of |id| for the map key than the value of deserialized->id()? https://codereview.chromium.org/1867653002/diff/180001/blimp/common/proto/blo... File blimp/common/proto/blob_cache.proto (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/proto/blo... blimp/common/proto/blob_cache.proto:15: message BlobCacheImage { BlobCacheImageMetadata? It'll just be metadata in the brave new BlobChannel world. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/proto/blo... blimp/common/proto/blob_cache.proto:19: optional bytes payload = 4; Add TODO to remove |payload| after we incorporate BlobChannel. https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:34: std::unique_ptr<blimp::BlobCacheImage> proto) { pass as const ref https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:77: std::string sha1 = blimp::ToSHA1HexString(sha1_bytes.data()); Ditto my previous comment in another file about using two different representations of the same SHA1 hash. https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:81: proto->set_id(sha1_bytes.data(), sha1_bytes.size()); .swap() the vectors for great efficiencies.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867653002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867653002/190001
Description was changed from ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImage. If this would be a new image for the client, the proto also includes the payload of the image. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719, 603353 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageMetadata. If this would be a new image for the client, the proto also includes the payload of the image. This payload will be removed when the BlobChannel is fully integrated. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719, 603353 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
kmarshall: PTAL https://codereview.chromium.org/1867653002/diff/180001/blimp/client/feature/c... File blimp/client/feature/compositor/decoding_image_generator.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:18: std::unique_ptr<BlobCacheImage> deserialized(new BlobCacheImage); On 2016/04/14 20:08:30, Kevin M wrote: > "parsed_metadata"? Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:28: SkImageInfo::MakeN32Premul(deserialized->width(), deserialized->height()); On 2016/04/14 20:08:30, Kevin M wrote: > Inline this in the DecodingImageGenerator ctor call Done. Also did above. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:10: #include <vector> On 2016/04/14 20:08:30, Kevin M wrote: > Necessary? Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache.h:21: class BLIMP_COMMON_EXPORT InfiniteBlobCache : public BlobCache { On 2016/04/14 20:08:30, Kevin M wrote: > InMemoryBlobCache (suggested by Wez because the name will still be correct when > the cache becomes finite.) Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... File blimp/common/blob_cache/infinite_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/infinite_blob_cache_unittest.cc:24: scoped_refptr<RefCountedVector> Create(const std::string& data) { On 2016/04/14 20:08:30, Kevin M wrote: > CreateRefCountedVector? Done. I don't see how this improves readability though, given that every time it is used in a test, the left hand side literally says scoped_refptr<RefCountedVector> :-) https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... File blimp/common/blob_cache/sha1_util.h (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/blob_cach... blimp/common/blob_cache/sha1_util.h:23: BLIMP_COMMON_EXPORT const std::string ToSHA1HexString(const void* data); On 2016/04/14 20:08:30, Kevin M wrote: > It's hard to distinguish this -- which doesn't hash |data| -- from > ToSHA1HashBytes, which does hash |data|. > > How about FormatSHA1Hash? Agreed. Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:55: DCHECK(deserialized->id().length() == base::kSHA1Length); On 2016/04/14 20:08:30, Kevin M wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:56: std::string hex_id = blimp::ToSHA1HexString(deserialized->id().c_str()); On 2016/04/14 20:08:30, Kevin M wrote: > Why are we using a different encoding of |id| for the map key than the value of > deserialized->id()? Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/proto/blo... File blimp/common/proto/blob_cache.proto (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/common/proto/blo... blimp/common/proto/blob_cache.proto:15: message BlobCacheImage { On 2016/04/14 20:08:30, Kevin M wrote: > BlobCacheImageMetadata? It'll just be metadata in the brave new BlobChannel > world. Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/common/proto/blo... blimp/common/proto/blob_cache.proto:19: optional bytes payload = 4; On 2016/04/14 20:08:30, Kevin M wrote: > Add TODO to remove |payload| after we incorporate BlobChannel. Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:34: std::unique_ptr<blimp::BlobCacheImage> proto) { On 2016/04/14 20:08:30, Kevin M wrote: > pass as const ref Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:77: std::string sha1 = blimp::ToSHA1HexString(sha1_bytes.data()); On 2016/04/14 20:08:30, Kevin M wrote: > Ditto my previous comment in another file about using two different > representations of the same SHA1 hash. Done. https://codereview.chromium.org/1867653002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:81: proto->set_id(sha1_bytes.data(), sha1_bytes.size()); On 2016/04/14 20:08:30, Kevin M wrote: > .swap() the vectors for great efficiencies. So now, I use the sha1_bytes below when looking up in the cache. I guess then I would have to look it up from the proto after this? Please advise.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.h:32: typedef std::map<std::string, scoped_refptr<RefCountedVector>> Cache; The typedef is kind of unnecessary here, since you don't reference this name anywhere else. What about renaming "map_" as "cache_" to make it clear that the map represents the cache? https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:19: const std::string kFoo("foo"); kConstants must be POD types (in this case, const char[]) https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/sha1_util.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/sha1_util.cc:21: const std::string FormatSHA1Hash(const void* data) { When data becomes a string, DCHECK it for a correct size. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/sha1_util.h (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/sha1_util.h:23: BLIMP_COMMON_EXPORT const std::string FormatSHA1Hash(const void* data); Take a const std::string& as input. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:56: DCHECK_EQ(base::kSHA1Length, deserialized->id().length()); This is network-originating data, so DCHECK isn't really appropriate here. Handle the case of a non-SHA1 ID for release by making a conditional with a DLOG(ERROR) inside. The FormatSHA1Hash call would behave badly if the ID was smaller than kSHA1Length, since it would overrun the string buffer. https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:73: // Each pixel is using 4 bytes (RGBA/BGRA), so hash over the whole input. Is there something we can check to validate the pixel format assertion? I think the consequence of guessing wrongly is that we compute a bogus size value that could potentially overrun the pixmap.addr() buffer.
https://codereview.chromium.org/1867653002/diff/190001/blimp/client/feature/c... File blimp/client/feature/compositor/decoding_image_generator.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:18: std::unique_ptr<BlobCacheImageMetadata> parsed_metadata( Since this seems to be only used locally, can you create this on the stack? That is, BlobCacheImageMetadata parsed_metadata; https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:21: DLOG(WARNING) << "Item with ID " << id << " already exists in cache."; If this is not an expected scenario, then it should be a DCHECK/CHECK instead. Otherwise, can you comment why this might happen? https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:24: map_.insert(std::make_pair(id, data)); you can std::move(data) here to avoid ref/unref if you want. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:33: return a->data == b->data; :) Do you really need an Equal function? https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:38: InMemoryBlobCacheTest() : cache_(base::WrapUnique(new InMemoryBlobCache)) {} Why not just: InMemoryBlobCache cache_;? https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:40: std::unique_ptr<BlobCacheImageMetadata> deserialized( BlobCacheImageMetadata deserialized; https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:50: WebPData webp_data; Define this closer to where it's used please https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:36: size_t unsigned_size = base::checked_cast<size_t>(signed_size); This is super safe and all, but do you need this? Can you comment on when you think proto.ByteSize() would not be representable by size_t? https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:73: // Each pixel is using 4 bytes (RGBA/BGRA), so hash over the whole input. On 2016/04/15 17:28:51, Kevin M wrote: > Is there something we can check to validate the pixel format assertion? I think > the consequence of guessing wrongly is that we compute a bogus size value that > could potentially overrun the pixmap.addr() buffer. SkPixmap has a bunch of functions to tell you the size (I recommend SkPixmap::getSafeSize). https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:80: std::unique_ptr<blimp::BlobCacheImageMetadata> proto( Create this on the stack? https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:89: SkData* sk_data = BlobCacheImageMetadataProtoAsSkData(*proto.get()); If you create proto on the stack, then *proto.get() becomes just proto.
kmarshall, vmpstr: PTAL https://codereview.chromium.org/1867653002/diff/190001/blimp/client/feature/c... File blimp/client/feature/compositor/decoding_image_generator.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/client/feature/c... blimp/client/feature/compositor/decoding_image_generator.cc:18: std::unique_ptr<BlobCacheImageMetadata> parsed_metadata( On 2016/04/15 18:18:54, vmpstr wrote: > Since this seems to be only used locally, can you create this on the stack? That > is, > > BlobCacheImageMetadata parsed_metadata; Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:21: DLOG(WARNING) << "Item with ID " << id << " already exists in cache."; On 2016/04/15 18:18:54, vmpstr wrote: > If this is not an expected scenario, then it should be a DCHECK/CHECK instead. > Otherwise, can you comment why this might happen? Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:24: map_.insert(std::make_pair(id, data)); On 2016/04/15 18:18:54, vmpstr wrote: > you can std::move(data) here to avoid ref/unref if you want. Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.h:32: typedef std::map<std::string, scoped_refptr<RefCountedVector>> Cache; On 2016/04/15 17:28:51, Kevin M wrote: > The typedef is kind of unnecessary here, since you don't reference this name > anywhere else. What about renaming "map_" as "cache_" to make it clear that the > map represents the cache? Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:19: const std::string kFoo("foo"); On 2016/04/15 17:28:51, Kevin M wrote: > kConstants must be POD types (in this case, const char[]) Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:33: return a->data == b->data; On 2016/04/15 18:18:54, vmpstr wrote: > :) Do you really need an Equal function? Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:38: InMemoryBlobCacheTest() : cache_(base::WrapUnique(new InMemoryBlobCache)) {} On 2016/04/15 18:18:54, vmpstr wrote: > Why not just: > > InMemoryBlobCache cache_;? Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/sha1_util.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/sha1_util.cc:21: const std::string FormatSHA1Hash(const void* data) { On 2016/04/15 17:28:51, Kevin M wrote: > When data becomes a string, DCHECK it for a correct size. Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... File blimp/common/blob_cache/sha1_util.h (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/blob_cach... blimp/common/blob_cache/sha1_util.h:23: BLIMP_COMMON_EXPORT const std::string FormatSHA1Hash(const void* data); On 2016/04/15 17:28:51, Kevin M wrote: > Take a const std::string& as input. Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:40: std::unique_ptr<BlobCacheImageMetadata> deserialized( On 2016/04/15 18:18:54, vmpstr wrote: > BlobCacheImageMetadata deserialized; Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:50: WebPData webp_data; On 2016/04/15 18:18:54, vmpstr wrote: > Define this closer to where it's used please Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:56: DCHECK_EQ(base::kSHA1Length, deserialized->id().length()); On 2016/04/15 17:28:51, Kevin M wrote: > This is network-originating data, so DCHECK isn't really appropriate here. > Handle the case of a non-SHA1 ID for release by making a conditional with a > DLOG(ERROR) inside. > > The FormatSHA1Hash call would behave badly if the ID was smaller than > kSHA1Length, since it would overrun the string buffer. Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:36: size_t unsigned_size = base::checked_cast<size_t>(signed_size); On 2016/04/15 18:18:54, vmpstr wrote: > This is super safe and all, but do you need this? Can you comment on when you > think proto.ByteSize() would not be representable by size_t? It returns an int, which could be negative, so we don't want that. After security review of code related to passing protos around, the security team wanted us to do this for protos. https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:73: // Each pixel is using 4 bytes (RGBA/BGRA), so hash over the whole input. On 2016/04/15 18:18:54, vmpstr wrote: > On 2016/04/15 17:28:51, Kevin M wrote: > > Is there something we can check to validate the pixel format assertion? I > think > > the consequence of guessing wrongly is that we compute a bogus size value that > > could potentially overrun the pixmap.addr() buffer. > > SkPixmap has a bunch of functions to tell you the size (I recommend > SkPixmap::getSafeSize). Doh... I thought they looked like row size :-/ they all return the same value as my ugly hack in the normal case, so I went with getSafeSize. Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:80: std::unique_ptr<blimp::BlobCacheImageMetadata> proto( On 2016/04/15 18:18:54, vmpstr wrote: > Create this on the stack? Done. https://codereview.chromium.org/1867653002/diff/190001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:89: SkData* sk_data = BlobCacheImageMetadataProtoAsSkData(*proto.get()); On 2016/04/15 18:18:54, vmpstr wrote: > If you create proto on the stack, then *proto.get() becomes just proto. Done. https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:25: virtual bool Contains(BlobId& id) const = 0; This makes cpplint angry (here and in the sha1util): //blimp/common/blob_cache/blob_cache.h:25: Is this a non-const reference? If so, make const or use a pointer: BlobId& id [runtime/references] [2] But BlobId is already const, so I'm not sure why it's complaining. We can't typedef in const? Please advise. https://codereview.chromium.org/1867653002/diff/230001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:31: static base::LazyInstance<std::set<std::string>> g_client_cache_contents = with base::LazyInstance<std::set<BlobID>> this fails with address() being declared twice. Same of course with base::LazyInstance<std::set<const std::string>> Please advise.
https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:25: virtual bool Contains(BlobId& id) const = 0; On 2016/04/16 00:25:30, nyquist wrote: > This makes cpplint angry (here and in the sha1util): > > //blimp/common/blob_cache/blob_cache.h:25: Is this a non-const reference? If > so, make const or use a pointer: BlobId& id [runtime/references] [2] > > > But BlobId is already const, so I'm not sure why it's complaining. We can't > typedef in const? > > Please advise. I don't think linter is smart enough to do this. Also, I think we should keep const outside of the using clause.. That is, you can have BlodId = std::string, and then const BlodId& wherever you use this. https://codereview.chromium.org/1867653002/diff/230001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:31: static base::LazyInstance<std::set<std::string>> g_client_cache_contents = On 2016/04/16 00:25:30, nyquist wrote: > with base::LazyInstance<std::set<BlobID>> this fails with address() being > declared twice. > Same of course with base::LazyInstance<std::set<const std::string>> > > Please advise. What do ou mean address() being declared twice? Is it a compiler error? What is it?
https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:25: virtual bool Contains(BlobId& id) const = 0; On 2016/04/16 00:52:47, vmpstr wrote: > On 2016/04/16 00:25:30, nyquist wrote: > > This makes cpplint angry (here and in the sha1util): > > > > //blimp/common/blob_cache/blob_cache.h:25: Is this a non-const reference? If > > so, make const or use a pointer: BlobId& id [runtime/references] [2] > > > > > > But BlobId is already const, so I'm not sure why it's complaining. We can't > > typedef in const? > > > > Please advise. > > I don't think linter is smart enough to do this. Also, I think we should keep > const outside of the using clause.. That is, you can have BlodId = std::string, > and then const BlodId& wherever you use this. Can you just make this a string in general and not use BlobId? I'm not sure what it buys you.
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
vmpstr: PTAL https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... File blimp/common/blob_cache/blob_cache.h (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/common/blob_cach... blimp/common/blob_cache/blob_cache.h:25: virtual bool Contains(BlobId& id) const = 0; On 2016/04/16 00:55:08, vmpstr wrote: > On 2016/04/16 00:52:47, vmpstr wrote: > > On 2016/04/16 00:25:30, nyquist wrote: > > > This makes cpplint angry (here and in the sha1util): > > > > > > //blimp/common/blob_cache/blob_cache.h:25: Is this a non-const reference? > If > > > so, make const or use a pointer: BlobId& id [runtime/references] [2] > > > > > > > > > But BlobId is already const, so I'm not sure why it's complaining. We can't > > > typedef in const? > > > > > > Please advise. > > > > I don't think linter is smart enough to do this. Also, I think we should keep > > const outside of the using clause.. That is, you can have BlodId = > std::string, > > and then const BlodId& wherever you use this. > > Can you just make this a string in general and not use BlobId? I'm not sure what > it buys you. As a pure std::string I'm not sure either. Since we've used some time internally to decide on what we wanted for BlobData I'm kind of inclined to keep it very well defined here for now. kmarshall: What do you think? https://codereview.chromium.org/1867653002/diff/230001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/230001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:31: static base::LazyInstance<std::set<std::string>> g_client_cache_contents = On 2016/04/16 00:52:47, vmpstr wrote: > On 2016/04/16 00:25:30, nyquist wrote: > > with base::LazyInstance<std::set<BlobID>> this fails with address() being > > declared twice. > > Same of course with base::LazyInstance<std::set<const std::string>> > > > > Please advise. > > What do ou mean address() being declared twice? Is it a compiler error? What is > it? Sent link to compile error on chat, but basically this and a few others related to _Base::. In file included from ../../blimp/engine/renderer/engine_image_serialization_processor.cc:5: In file included from ../../blimp/engine/renderer/engine_image_serialization_processor.h:9: In file included from ../../base/memory/scoped_ptr.h:12: In file included from ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/memory:65: In file included from ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/allocator.h:48: In file included from ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-gnu/bits/c++allocator.h:34: ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/ext/new_allocator.h:82:7: error: multiple overloads of 'address' instantiate to the same signature 'const_pointer (const_reference) const' (aka 'const std::basic_string<char, std::char_traits<char>, std::allocator<char> > *(const std::basic_string<char, std::char_traits<char>, std::allocator<char> > &) const') address(const_reference __x) const { return std::__addressof(__x); } ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/allocator.h:92:29: note: in instantiation of template class '__gnu_cxx::new_allocator<const std::basic_string<char, std::char_traits<char>, std::allocator<char> > >' requ ested here class allocator: public __glibcxx_base_allocator<_Tp> ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-gnu/bits/c++allocator.h:35:35: note: expanded from macro '__glibcxx_base_allocator' #define __glibcxx_base_allocator __gnu_cxx::new_allocator ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_set.h:92:24: note: in instantiation of template class 'std::allocator<const std::basic_string<char, std::char_traits<char>, std::allocator<char> > >' requested here typedef typename _Alloc::value_type _Alloc_value_type; ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/set.h:45:14: note: in instantiation of template class 'std::__cxx1998::set<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<const std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<const std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >' requested here : public _GLIBCXX_STD_C::set<_Key,_Compare,_Allocator>, ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/x86_64-linux-gnu/bits/c++config.h:266:25: note: expanded from macro '_GLIBCXX_STD_C' # define _GLIBCXX_STD_C __cxx1998 ^ ../../blimp/engine/renderer/engine_image_serialization_processor.cc:30:29: note: in instantiation of template class 'std::__debug::set<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<const std::basic_string<char, std::char_traits<char>, std:: allocator<char> > >, std::allocator<const std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >' requested here std::set<const std::string> g_client_cache_contents; ^ ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/ext/new_allocator.h:79:7: note: previous declaration is here address(reference __x) const { return std::__addressof(__x); }
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867653002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867653002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Lookin' spiffy! I wrote some feedback to your questions to me from the previous patch, but I saw that your most recent patch addressed those issues. So now I'm only left with a few nits. ;) https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:39: proto.SerializeToArray(serialized.data(), signed_size); optimization nit: SerializeWithCachedSizesToArray? (you've already computed the value with ByteSize()) https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:51: // Always encode even WebP data, to enable caching. "even" ? "Encode all images regardless of their format, including WebP images"? https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:102: std::vector<unsigned char> data; Pick a more descriptive name?
lgtm with nits https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:20: // In cases where the engine has miscalculated what is already available I'm ok with leaving this as is, but this would seem like a bug in the engine? That is, if this was all chromium code, then this should be a DCHECK. However, because this is the client I don't know what should happen. https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:34: if (!Contains(id)) { nit: braces optional https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1867653002/diff/250001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:49: LOG(WARNING) << "Length of ID is not base::kSHA1Length, but " Log base::kSHA1Length as well for posterity? https://codereview.chromium.org/1867653002/diff/250001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:69: cached = g_blob_cache.Get().Get(deserialized.id()); Honestly, I'm not sure how I feel about BlobData being a scoped_refptr. This looks like it's doing some sort of a deep copy, so one would be tempted to rewrite this to bind it to a const ref or something. However, it's cheap because it's a pointer... A little non obvious, that's all. This is just a comment, not a suggestion for a fix or anything :P
all done! https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:20: // In cases where the engine has miscalculated what is already available On 2016/04/18 19:38:11, vmpstr wrote: > I'm ok with leaving this as is, but this would seem like a bug in the engine? > That is, if this was all chromium code, then this should be a DCHECK. However, > because this is the client I don't know what should happen. When we deploy this for realz, there idea is to use a bloom filter to estimate if the client has something or not. There might be issues with that, both races and also not a perfect representation, so in practice this could happen. I guess though, that in practice it might more often be the other way around, where the client doesn't have something that the engine thinks it has. I'll keep your comment in mind going forward when I start working on that. https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache.cc:34: if (!Contains(id)) { On 2016/04/18 19:38:11, vmpstr wrote: > nit: braces optional The other reviewer pointed out to me that in //blimp we apparently use braces... https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... File blimp/common/blob_cache/in_memory_blob_cache_unittest.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/common/blob_cach... blimp/common/blob_cache/in_memory_blob_cache_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/18 19:38:11, vmpstr wrote: > nit: 2016 I totally didn't copy-paste this from an existing file. https://codereview.chromium.org/1867653002/diff/250001/blimp/common/composito... File blimp/common/compositor/webp_decoder.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:49: LOG(WARNING) << "Length of ID is not base::kSHA1Length, but " On 2016/04/18 19:38:11, vmpstr wrote: > Log base::kSHA1Length as well for posterity? Done. https://codereview.chromium.org/1867653002/diff/250001/blimp/common/composito... blimp/common/compositor/webp_decoder.cc:69: cached = g_blob_cache.Get().Get(deserialized.id()); On 2016/04/18 19:38:11, vmpstr wrote: > Honestly, I'm not sure how I feel about BlobData being a scoped_refptr. This > looks like it's doing some sort of a deep copy, so one would be tempted to > rewrite this to bind it to a const ref or something. However, it's cheap because > it's a pointer... A little non obvious, that's all. > > This is just a comment, not a suggestion for a fix or anything :P Okay. I'll keep that in mind. If we realize going forward that it's easy to make such mistakes, we should definitely look into changing it. https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... File blimp/engine/renderer/engine_image_serialization_processor.cc (right): https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:39: proto.SerializeToArray(serialized.data(), signed_size); On 2016/04/18 18:13:14, Kevin M wrote: > optimization nit: SerializeWithCachedSizesToArray? (you've already computed the > value with ByteSize()) Done. https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:51: // Always encode even WebP data, to enable caching. On 2016/04/18 18:13:14, Kevin M wrote: > "even" ? > > "Encode all images regardless of their format, including WebP images"? Done. https://codereview.chromium.org/1867653002/diff/250001/blimp/engine/renderer/... blimp/engine/renderer/engine_image_serialization_processor.cc:102: std::vector<unsigned char> data; On 2016/04/18 18:13:14, Kevin M wrote: > Pick a more descriptive name? Done.
The CQ bit was checked by nyquist@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1867653002/#ps310001 (title: "git merge origin/master for good measure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867653002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867653002/310001
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageMetadata. If this would be a new image for the client, the proto also includes the payload of the image. This payload will be removed when the BlobChannel is fully integrated. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719, 603353 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Initial version of Blimp BlobCache. Currently, every time there is a commit from engine to client, all pictures are sent again, leading to poor performance and high bandwidth usage. This CL adds a cache on the client side that never clears out any received resources. This means that the engine can depend on that the client has all resources it has previously sent, and uses this fact to only send down a unique identifier (SHA1 hash of the image) and the the width and height in place of the image itself. This is wrapped in the new proto BlobCacheImageMetadata. If this would be a new image for the client, the proto also includes the payload of the image. This payload will be removed when the BlobChannel is fully integrated. The identifier is calculated based on the source image, to ensure that no encode is required on the engine when the same image is seen again. To ensure that the cache is used for all images, this changes the behavior of the serializer to also encode images that were previously encoded as WebP, to ensure that they become cacheable. BUG=597807, 600719, 603353 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8 Cr-Commit-Position: refs/heads/master@{#388013} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/a2796c212636511499f47597e76865d7577cd1c8 Cr-Commit-Position: refs/heads/master@{#388013}
Message was sent while issue was closed.
davidben@chromium.org changed reviewers: + davidben@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1867653002/diff/310001/blimp/common/blob_cach... File blimp/common/blob_cache/id_util.cc (right): https://codereview.chromium.org/1867653002/diff/310001/blimp/common/blob_cach... blimp/common/blob_cache/id_util.cc:16: base::SHA1HashBytes(static_cast<const unsigned char*>(data), data_size, Drive-by comment: why is this using SHA-1 and not SHA-2? People have been managing to find attacks against SHA-1 and since I assume you don't have any legacy constraints, using SHA-1 for new things is rather poor.
Message was sent while issue was closed.
On 2016/04/22 16:39:49, davidben wrote: > https://codereview.chromium.org/1867653002/diff/310001/blimp/common/blob_cach... > File blimp/common/blob_cache/id_util.cc (right): > > https://codereview.chromium.org/1867653002/diff/310001/blimp/common/blob_cach... > blimp/common/blob_cache/id_util.cc:16: base::SHA1HashBytes(static_cast<const > unsigned char*>(data), data_size, > Drive-by comment: why is this using SHA-1 and not SHA-2? People have been > managing to find attacks against SHA-1 and since I assume you don't have any > legacy constraints, using SHA-1 for new things is rather poor. Do we have any performance stats for SHA-1 vs SHA-2 regarding how long time they take to calculate?
Message was sent while issue was closed.
On 2016/04/22 19:27:12, nyquist wrote: > On 2016/04/22 16:39:49, davidben wrote: > > > https://codereview.chromium.org/1867653002/diff/310001/blimp/common/blob_cach... > > File blimp/common/blob_cache/id_util.cc (right): > > > > > https://codereview.chromium.org/1867653002/diff/310001/blimp/common/blob_cach... > > blimp/common/blob_cache/id_util.cc:16: base::SHA1HashBytes(static_cast<const > > unsigned char*>(data), data_size, > > Drive-by comment: why is this using SHA-1 and not SHA-2? People have been > > managing to find attacks against SHA-1 and since I assume you don't have any > > legacy constraints, using SHA-1 for new things is rather poor. > > Do we have any performance stats for SHA-1 vs SHA-2 regarding how long time they > take to calculate? If you need the security properties, you do not use SHA-1, period. But here are numbers from BoringSSL on my machine: $ ./build-release/tool/bssl speed SHA-1 Did 5067000 SHA-1 (16 bytes) operations in 1000012us (5066939.2 ops/sec): 81.1 MB/s Did 1544000 SHA-1 (256 bytes) operations in 1000107us (1543834.8 ops/sec): 395.2 MB/s Did 70000 SHA-1 (8192 bytes) operations in 1010120us (69298.7 ops/sec): 567.7 MB/s $ ./build-release/tool/bssl speed SHA-256 Did 3215000 SHA-256 (16 bytes) operations in 1000097us (3214688.2 ops/sec): 51.4 MB/s Did 739000 SHA-256 (256 bytes) operations in 1001091us (738194.6 ops/sec): 189.0 MB/s Did 37000 SHA-256 (8192 bytes) operations in 1020697us (36249.7 ops/sec): 297.0 MB/s That ought to be fast enough I think. Note that you aren't even getting the above numbers for SHA-1. You're using //base's copy of SHA-1 which isn't using BoringSSL's optimized implementation. So in reality you will probably get a performance win. |
