Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1530583003:
[Tracing] Adjust allocation register size for low-end devices (Closed)
|
Created:
5 years ago by seantopping Modified:
5 years ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Tracing] Adjust allocation register size for low-end devices
This patch limits the size of the allocation register and prevents OOM
errors when running AllocationRegisterTest.OverflowDeathTest on low-end
devices.
BUG=570242
Committed: https://crrev.com/684e479f9a46580b5d5bdcf58944b832f6317dc1
Cr-Commit-Position: refs/heads/master@{#365905}
Patch Set 1 #Patch Set 2 : AllocationRegister(num_cells) #Patch Set 3 : Remove unused include #
Total comments: 4
Patch Set 4 : CR Feedback #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=None ========== to ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=internal b/25791887 ==========
seantopping@chromium.org changed reviewers: + primiano@chromium.org, ruuda@google.com
Ruud + Primiano: Please take a look, see the internal bug for more context. Thanks!
On 2015/12/16 00:21:34, seantopping wrote: > Ruud + Primiano: Please take a look, see the internal bug for more context. > Thanks! LGTM
If I understand correctly the problem here is that the test fills up all the memory (making it resident), which causes an OOM before reaching the death. I wonder if the most robust approach here is to create an extra constructor (maybe for testing only) that takes num_cell as argument, so we don't have to fill up 100 MB or whatever in the test. This patch LGTM in the meantime to unblock the situation, Ruud, maybe you can fix the test as suggested above in a follow-up ? P.S. As a general rule we don't take internal bug references on codereview here. I just created crbug.com/570242, use BUG=570242 instead.
Description was changed from ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=internal b/25791887 ========== to ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=570242 ==========
The CQ bit was checked by seantopping@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530583003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530583003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL, I went ahead and implemented the suggested fix in crbug.com/570242 (also to remove the SysInfo dependency, since this fix is only ever really needed for the unit test).
Thanks! I have one comment. https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register_unittest.cc (right): https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register_unittest.cc:210: AllocationRegister reg(kNumBuckets * 5); Now that this is configurable, we might as well pick a small value such as |GetNumCellsPerPage()| or 32 (the value of |GetNumCellsPerPage()| for x64 with 4096-byte pages) here. It will have the additional benefit of speeding up the test.
https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:63: AllocationRegister(uint32_t num_cells); +explicit. I guess you don't want integers to automatically cast into AllocationRegisters :)
Re: comments https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:63: AllocationRegister(uint32_t num_cells); On 2015/12/17 11:13:33, Primiano Tucci wrote: > +explicit. I guess you don't want integers to automatically cast into > AllocationRegisters :) Done. https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register_unittest.cc (right): https://codereview.chromium.org/1530583003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register_unittest.cc:210: AllocationRegister reg(kNumBuckets * 5); On 2015/12/17 10:10:06, Ruud van Asseldonk wrote: > Now that this is configurable, we might as well pick a small value such as > |GetNumCellsPerPage()| or 32 (the value of |GetNumCellsPerPage()| for x64 with > 4096-byte pages) here. It will have the additional benefit of speeding up the > test. Done.
The CQ bit was checked by seantopping@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/1530583003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530583003/60001
On 2015/12/17 20:09:51, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1530583003/60001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1530583003/60001 LGTM
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 seantopping@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1530583003/#ps60001 (title: "CR Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530583003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530583003/60001
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=570242 ========== to ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=570242 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=570242 ========== to ========== [Tracing] Adjust allocation register size for low-end devices This patch limits the size of the allocation register and prevents OOM errors when running AllocationRegisterTest.OverflowDeathTest on low-end devices. BUG=570242 Committed: https://crrev.com/684e479f9a46580b5d5bdcf58944b832f6317dc1 Cr-Commit-Position: refs/heads/master@{#365905} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/684e479f9a46580b5d5bdcf58944b832f6317dc1 Cr-Commit-Position: refs/heads/master@{#365905}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1535063003/ by phoglund@chromium.org. The reason for reverting is: Breaks Win GN compile (warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data): https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/9247....
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1538783003/ by dcheng@chromium.org. The reason for reverting is: win64 GN builders are broken: e:\b\build\slave\win_x64_gn\build\src\base\trace_event\heap_profiler_allocation_register_unittest.cc(210) : error C2220: warning treated as error - no 'object' file generated e:\b\build\slave\win_x64_gn\build\src\base\trace_event\heap_profiler_allocation_register_unittest.cc(210) : warning C4267: 'argument' : conversion from 'size_t' to 'uint32_t', possible loss of data .
Message was sent while issue was closed.
On 2015/12/18 09:03:30, dcheng wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/1538783003/ by mailto:dcheng@chromium.org. I went ahead and relanded it: https://crrev.com/1531263004. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
