Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349553002:
Keep allocation size available in minidumps. (Closed)
DescriptionKeep allocation size available in minidumps.
We were previously unable to see the allocation size in the minidump
because it was optimized out. This should keep it around.
BUG=643845
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/10a3909c4a5bf699ee824a3ea24472fef1ffdfe2
Cr-Commit-Position: refs/heads/master@{#419076}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Builds. #
Total comments: 2
Patch Set 3 : size_t instead of auto. Using the variable. #Messages
Total messages: 17 (7 generated)
Description was changed from ========== Keep allocation size available in minidumps. We were previously unable to see the allocation size in the minidump because it was optimized out. This should keep it around. BUG=643845 ========== to ========== Keep allocation size available in minidumps. We were previously unable to see the allocation size in the minidump because it was optimized out. This should keep it around. BUG=643845 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
cblume@chromium.org changed reviewers: + jbauman@chromium.org, vmpstr@chromium.org
PTAL
https://codereview.chromium.org/2349553002/diff/1/cc/tiles/gpu_image_decode_c... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2349553002/diff/1/cc/tiles/gpu_image_decode_c... cc/tiles/gpu_image_decode_controller.cc:9: #include "base/memory/discardable_memory_allocator.h" #include "base/debug/alias.h" https://codereview.chromium.org/2349553002/diff/1/cc/tiles/gpu_image_decode_c... cc/tiles/gpu_image_decode_controller.cc:949: base::debug::Alias(image_data_size); I think you need &image_data_size
https://codereview.chromium.org/2349553002/diff/1/cc/tiles/gpu_image_decode_c... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2349553002/diff/1/cc/tiles/gpu_image_decode_c... cc/tiles/gpu_image_decode_controller.cc:9: #include "base/memory/discardable_memory_allocator.h" On 2016/09/15 23:08:34, jbauman wrote: > #include "base/debug/alias.h" Done. https://codereview.chromium.org/2349553002/diff/1/cc/tiles/gpu_image_decode_c... cc/tiles/gpu_image_decode_controller.cc:949: base::debug::Alias(image_data_size); On 2016/09/15 23:08:34, jbauman wrote: > I think you need &image_data_size Done. Sorry, my mind was elsewhere.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm https://codereview.chromium.org/2349553002/diff/20001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2349553002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:949: auto image_data_size = image_data->size; nit: size_t instead of auto. I'm guessing you don't need to use this var in AllocateLockedDiscardable?
https://codereview.chromium.org/2349553002/diff/20001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2349553002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:949: auto image_data_size = image_data->size; On 2016/09/16 00:07:41, vmpstr wrote: > nit: size_t instead of auto. > > I'm guessing you don't need to use this var in AllocateLockedDiscardable? I plan to remove this code in short order. I only want to add it to see the value in minidumps coming in from a crash that is currently live on Canary.
On 2016/09/16 00:13:58, cblume wrote: > https://codereview.chromium.org/2349553002/diff/20001/cc/tiles/gpu_image_deco... > File cc/tiles/gpu_image_decode_controller.cc (right): > > https://codereview.chromium.org/2349553002/diff/20001/cc/tiles/gpu_image_deco... > cc/tiles/gpu_image_decode_controller.cc:949: auto image_data_size = > image_data->size; > On 2016/09/16 00:07:41, vmpstr wrote: > > nit: size_t instead of auto. > > > > I'm guessing you don't need to use this var in AllocateLockedDiscardable? > > I plan to remove this code in short order. > I only want to add it to see the value in minidumps coming in from a crash that > is currently live on Canary. Ah, ok, then lgtm :)
The CQ bit was checked by cblume@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2349553002/#ps40001 (title: "size_t instead of auto. Using the variable.")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Keep allocation size available in minidumps. We were previously unable to see the allocation size in the minidump because it was optimized out. This should keep it around. BUG=643845 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Keep allocation size available in minidumps. We were previously unable to see the allocation size in the minidump because it was optimized out. This should keep it around. BUG=643845 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/10a3909c4a5bf699ee824a3ea24472fef1ffdfe2 Cr-Commit-Position: refs/heads/master@{#419076} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/10a3909c4a5bf699ee824a3ea24472fef1ffdfe2 Cr-Commit-Position: refs/heads/master@{#419076} |
