Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386123003:
Add heap allocator usage to task profiler. (Closed)
|
Created:
4 years, 2 months ago by Sigurður Ásgeirsson Modified:
4 years ago CC:
chromium-reviews, ssid Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd heap allocator usage to task profiler.
This enables heap tracking in the task profiler (chrome://profiler) under a build flag, which is enabled for developer builds, but disabled for official builds for now.
As-is, this surfaces the following heap metrics per task by default:
- Avg number of allocations per run.
- Avg number of frees per run.
- Avg net bytes allocated (or freed if negative).
- Max allocated bytes at any time in any run.
This is a high-watermark for memory usage in the task.
Additionally it's possible to enable the following metrics by ticking a checkbox in chrome://profiler:
- Allocation count.
- Free count.
A running count of how many allocations and frees a task
has performed in all runs.
- Allocated bytes.
- Freed bytes.
A running tally of how many bytes a task has allocated and
freed in all runs.
- Overhead bytes.
An estimate of how many allocated bytes go to heap overhead.
This might be helpful to guide implementation to use larger
allocations or chunked data structures like std::vector
BUG=644385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/de38d0cc1f554562dc9062c97079585cc6522fe7
Cr-Commit-Position: refs/heads/master@{#436003}
Patch Set 1 #Patch Set 2 : Plumb data through to chrome://profiler UI. #Patch Set 3 : WIP: Expose net usage, misc tweaks. #Patch Set 4 : WIP: upload for fdoray@ #Patch Set 5 : Figure out where the @#$%! corruption is coming from. Move heap tracking to TaskStopwatch." #
Total comments: 24
Patch Set 6 : Address Primiano's comments. #Patch Set 7 : Merge with ToT and https://codereview.chromium.org/2421243003/ #Patch Set 8 : Push rename through + cleanup. Merge to ToT. #Patch Set 9 : Cosmetic cleanup. #Patch Set 10 : Add a build flag to enable or disable memory profiling in the task profiler. #Patch Set 11 : Move heap tracking initialization to the tracked_object init, engage marshaling. #Patch Set 12 : Fix initialization of heap tracking. #Patch Set 13 : Revert an uneccessary security policy override. #Patch Set 14 : Fix Clang compile (hopefully). #Patch Set 15 : Endow DeathDataSnapshot with a non-inlined constructor, per Clang's demand. #Patch Set 16 : Fix remaining clang compile errors. #
Total comments: 14
Patch Set 17 : Address HTML presubmit nits. #Patch Set 18 : Address Primiano's comments. #Patch Set 19 : Fix self-reinitialization on thread exit which was barfing all the tests. #Patch Set 20 : Make ThreadHeapUsage alloc/free symmetrical, fix TaskProfilerDataSerializerTest. #Patch Set 21 : Reformat. #Patch Set 22 : Disable ThreadChecker to eliminate it as cause for test failure fallout. #Patch Set 23 : Revert ThreadChecker disabling. #Patch Set 24 : Disable memory profiling for NaCL, dummy! #Patch Set 25 : Fix borked define flag. #
Total comments: 4
Patch Set 26 : Merge in the tcp_socket_win.cc fix. #Patch Set 27 : Address Primiano's comments, re-enable that irksome DCHECK, now that the fix is in. #
Total comments: 8
Patch Set 28 : Address Daniel's comments. #Patch Set 29 : Fix DeathData member initialization order. #Patch Set 30 : Unbreak snapshots by adding the new averages to the exceptions array. #Patch Set 31 : Now fix snapshots, not merely unbreak them. #
Total comments: 4
Patch Set 32 : Move SaturatingMemberAdd per Nico's comment. #
Total comments: 2
Patch Set 33 : Address Eric's comment. #Depends on Patchset: Messages
Total messages: 104 (77 generated)
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Add heap allocator usage to task profiler. BUG= ========== to ========== Add heap allocator usage to task profiler. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
siggi@chromium.org changed reviewers: + chrisha@chromium.org, primiano@chromium.org
Hey guys, heads up on this CL. This is currently a WIP, and more or less a proof of concept. I think it'll be useful to bring heap data into the task profiler, especially if we phone the aggregate allocation data home through UMA. I think it might be sane to put this under a buildflag, which we'd enable initially on Windows only. Assuming we think this is a good idea(TM), I think it makes sense to move the ScopedThreadHeapUsage instance into the TaskStopwatch class. LMK what you think. Siggi
not too fmailiar with the metric/profiler but overall this makes sense, with some comments here and there. Just hope that you can sort out all that checking for TLS weirdeness. https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:8: if defined(OS_WINDOWS) ?? (but then you need build_config first) https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:141: if (!g_thread_allocator_usage.initialized()) uh? Shouldn't just you initialize this in some main? I guess this is the reason of that "DO NOT SUBMIT"? https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:169: // CHECK(thread_checker_.CalledOnValidThread()); Please tell me you did never hit this CHECK :) https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:174: if (!g_thread_allocator_usage.initialized()) hmm the TLS slot itself should be initialzied only once per all int he process. how do you end up in this state? https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:180: DCHECK_NE(nullptr, thread_usage_); well, even without this dcheck, if this happens to be null it will fail on the next line. https://codereview.chromium.org/2386123003/diff/80001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/debug/task_annotat... base/debug/task_annotator.cc:34: base::debug::HeapUsageTracker heap_usage; btw liked Scoped in the name, made it clear that heap_usage here is a scoped thing. https://codereview.chromium.org/2386123003/diff/80001/base/debug/task_annotat... base/debug/task_annotator.cc:58: silly question: what's the point of this scoped HeapUsageTracker if you never collect the data at the end here? https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:79: // Bail quick if no work or alreadu saturated. typo: alreadu https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX) I think you are supposed to use at least NoBarrier_Load or this will stop building the day we fix base::subtle to use proper atomics https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:88: base::subtle::NoBarrier_Store(sum, new_sum); this is not atomic anymore now? Don't you want a NoBarrier_AtomicExchange here? https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:205: if (max_allocated_bytes > INT_MAX) I think base::saturated_cast might help here?
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:268: g_thread_allocator_usage.Initialize([](void* allocator_usage) { How can you be sure that this isn't initialized concurrently by multiple threads?
https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:8: On 2016/10/13 20:47:44, Primiano Tucci wrote: > if defined(OS_WINDOWS) ?? (but then you need build_config first) Thanks, this is debugging remnants - <windows.h> has no business here. Gone! https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:268: g_thread_allocator_usage.Initialize([](void* allocator_usage) { On 2016/10/14 13:03:32, fdoray wrote: > How can you be sure that this isn't initialized concurrently by multiple > threads? By contract, EnableHeapTracking can only be called from a single thread, once.
Thanks. I addressed this set of comments, but now I'm going to break out the ScopedThreadHeapUsage changes to a separate CL, as this CL is getting pretty unwieldy. https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... File base/debug/scoped_thread_heap_usage.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:141: if (!g_thread_allocator_usage.initialized()) On 2016/10/13 20:47:44, Primiano Tucci wrote: > uh? Shouldn't just you initialize this in some main? I guess this is the reason > of that "DO NOT SUBMIT"? Sorry, more debugging remnants. https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:169: // CHECK(thread_checker_.CalledOnValidThread()); On 2016/10/13 20:47:44, Primiano Tucci wrote: > Please tell me you did never hit this CHECK :) No, it just wrecked Chrome's state to the point where youtube.com was accessible, but slashdot.org wasn't :/ https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:174: if (!g_thread_allocator_usage.initialized()) On 2016/10/13 20:47:44, Primiano Tucci wrote: > hmm the TLS slot itself should be initialzied only once per all int he process. > how do you end up in this state? Yeah, I'm not sure what's the right way to do this. The thing is that as this bakes into existing instrumentation, either I mandate a check before using Start/Stop, or else I noop them when off. https://codereview.chromium.org/2386123003/diff/80001/base/debug/scoped_threa... base/debug/scoped_thread_heap_usage.cc:180: DCHECK_NE(nullptr, thread_usage_); On 2016/10/13 20:47:44, Primiano Tucci wrote: > well, even without this dcheck, if this happens to be null it will fail on the > next line. Done. https://codereview.chromium.org/2386123003/diff/80001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/debug/task_annotat... base/debug/task_annotator.cc:34: base::debug::HeapUsageTracker heap_usage; On 2016/10/13 20:47:44, Primiano Tucci wrote: > btw liked Scoped in the name, made it clear that heap_usage here is a scoped > thing. Yeah, the problem is that the existing instrumentation collects exclusive metrics (e.g. an inner measured thing does not contribute cost to the outer measured thing). A Scoped* thingy can't support that, as there will always be a gap from Scoped*::ReadUsage to it passing out of scope, and that gap will be altogether missing from the tally. I could reinstate a Scoped* as a subclass of HeapUsageTracker, which only supports inclusive measurement? https://codereview.chromium.org/2386123003/diff/80001/base/debug/task_annotat... base/debug/task_annotator.cc:58: On 2016/10/13 20:47:44, Primiano Tucci wrote: > silly question: what's the point of this scoped HeapUsageTracker if you never > collect the data at the end here? Good catch - I've moved the HUT into the StopWatch, forgot to clean this up. https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:79: // Bail quick if no work or alreadu saturated. On 2016/10/13 20:47:44, Primiano Tucci wrote: > typo: alreadu Done. https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX) On 2016/10/13 20:47:44, Primiano Tucci wrote: > I think you are supposed to use at least NoBarrier_Load or this will stop > building the day we fix base::subtle to use proper atomics I'm modeling on what's done elsewhere in this file. The threading model here is quite interesting, as there's only one thread that'll write to each datum. So, from the perspective of data integrity, it's fine for this thread to read the data without atomic primitives, as no other thread will ever modify it. https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:88: base::subtle::NoBarrier_Store(sum, new_sum); On 2016/10/13 20:47:44, Primiano Tucci wrote: > this is not atomic anymore now? Don't you want a NoBarrier_AtomicExchange here? This thread is the only writer, and in the interest of efficiency, we accept low atomicity guarantees here, as elsewhere in this file. Notably we only get per-datum atomicity at best. https://codereview.chromium.org/2386123003/diff/80001/base/tracked_objects.cc... base/tracked_objects.cc:205: if (max_allocated_bytes > INT_MAX) On 2016/10/13 20:47:44, Primiano Tucci wrote: > I think base::saturated_cast might help here? Thanks, neat! I didn't know of that.
Message was sent while issue was closed.
Description was changed from ========== Add heap allocator usage to task profiler. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add heap allocator usage to task profiler. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Add heap allocator usage to task profiler. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add heap allocator usage to task profiler. BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Add heap allocator usage to task profiler. BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add heap allocator usage to task profiler. This enables heap tracking in the task profiler (chrome://profiler) under a build flag, which is enabled for developer builds, but disabled for official builds for now. As-is, this surfaces the following heap metrics per task by default: - Avg number of allocations per run. - Avg number of frees per run. - Avg net bytes allocated (or freed if negative). - Max allocated bytes at any time in any run. This is a high-watermark for memory usage in the task. Additionally it's possible to enable the following metrics by ticking a checkbox in chrome://profiler: - Allocation count. - Free count. A running count of how many allocations and frees a task has performed in all runs. - Allocated bytes. - Freed bytes. A running tally of how many bytes a task has allocated and freed in all runs. - Overhead bytes. An estimate of how many allocated bytes go to heap overhead. This might be helpful to guide implementation to use larger allocations or chunked data structures like std::vector BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Hey guys, I think this is ready for another round of review. Siggi
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Generally the code looks good with some minor comments on my side. My major question is: why the build time switch? Since you have also a runtime switch, can this be just built by default? By experience #ifdef code is very hard to maintain, causes headaches for refactorings and goes bitrotten very easily. It doesn't seem that you are causing any binary particular binary size inflation here. https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c... base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX) I think you are supposed to use NoBarrier_Load and not dereference atomics directly, it might break once we move to std::atomics otherwise https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c... base/tracked_objects.cc:85: if (new_sum < *sum) I think that integer overflow is still considered undefined behavior. There is some discussion in http://stackoverflow.com/questions/16188263/is-signed-integer-overflow-still-... Maybe just safer if you do if ((added >= std::numeric_limits(...) - current_sum) ... https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h... base/tracked_objects.h:254: // forward no need for the // forward annotation, it's generally quite common to have these. https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h... base/tracked_objects.h:345: const uint32_t random_number); nit: add newline here https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h... base/tracked_objects.h:822: const base::debug::ThreadHeapUsageTracker& heap_usage() const; why not inlining also this accessor?
On Wed, Nov 16, 2016 at 11:39 AM <primiano@chromium.org> wrote: > Generally the code looks good with some minor comments on my side. > My major question is: why the build time switch? Since you have also a > runtime > switch, can this be just built by default? > By experience #ifdef code is very hard to maintain, causes headaches for > refactorings and goes bitrotten very easily. > It doesn't seem that you are causing any binary particular binary size > inflation > here. > > It's more that this code runs pretty frequently, so I'd like to be able to turn it on and off cleanly to see whether even the flag testing and additional local initialization upsets the applecart. Ideally we'll be able to run this everywhere, always, but I'm not sure that's the best starting point? Maybe I can make eliminating the build flag a TODO with a bug attached? As for bitrotting, I think the code strikes a reasonable balance in that I didn't make the new members in the ThreadData and DeathDataSnapshot classes conditional. The memory wasted is in excess storage in ThreadData is on the order of tens of kilobytes, so I didn't think that was a big deal. It also compiles both ways on multiple builders, as e.g. the Win debug builds don't have the shim, but the release builds do. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Addressed comments and fixed a gnarly bug on thread that thankfully triggered CHECKs. https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c... base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX) On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote: > I think you are supposed to use NoBarrier_Load and not dereference atomics > directly, it might break once we move to std::atomics otherwise This is modeled after all other Atomic32 (member) accesses in this file, which may all need a revamp once we we go to std::atomics. The threading model here is that only the owning thread will ever modify a ThreadData, and so it is safe for it to read it any old how. I have no idea whether direct access vs *_Load impacts performance, nor how that breaks down on different CPU architectures. I'd just as soon not add that variable to this CL? https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c... base/tracked_objects.cc:85: if (new_sum < *sum) On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote: > I think that integer overflow is still considered undefined behavior. > There is some discussion in > http://stackoverflow.com/questions/16188263/is-signed-integer-overflow-still-... > > Maybe just safer if you do > if ((added >= std::numeric_limits(...) - current_sum) > ... Ah, thanks. Went to base::CheckedNumeric - it seems to fit the bill? https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h... base/tracked_objects.h:254: // forward On 2016/11/16 16:39:10, Primiano - slow(travelling) wrote: > no need for the // forward annotation, it's generally quite common to have > these. Done. https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h... base/tracked_objects.h:345: const uint32_t random_number); On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote: > nit: add newline here Done. https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.h... base/tracked_objects.h:822: const base::debug::ThreadHeapUsageTracker& heap_usage() const; On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote: > why not inlining also this accessor? Thanks, done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Looks like this is ready for another review - I've squashed all the test failures by the looks of it.
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #23 (id:440001) has been deleted
The CQ bit was checked by siggi@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...
The CQ bit was checked by primiano@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...
Patchset #26 (id:520001) has been deleted
I am terribly sorry for the delay on this.
Allright, non-owner LGTM with some final comments.
I'd still strongly encourage you to remove the build flags. It seems to me that
the only thing this is guarding are a bunch of "if
(stopwatch.heap_tracking_enabled())".
I mean if you are really really paranoid you can create in this CL a bool
IsHeapTrackerEnabledInChromeProfiler() {
#if defined(OFFICIAL_BUILD) && defined(SHIM_WHATEVER)
return true
#end if
return false
}
this will:
-avoid creating a new build flag that you'll get rid soon after
- avoid getting other reviewers in the loop
- avoid having to touch too many lines and scare reviewers in the future.
Essentially if you do that in the future you just have to change the
implementation of IsHeapTrackerEnabledInChromeProfiler()
https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.cc
File base/tracked_objects.cc (right):
https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c...
base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX)
On 2016/11/16 21:30:28, Sigurður Ásgeirsson wrote:
> On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote:
> > I think you are supposed to use NoBarrier_Load and not dereference atomics
> > directly, it might break once we move to std::atomics otherwise
>
> This is modeled after all other Atomic32 (member) accesses in this file, which
> may all need a revamp once we we go to std::atomics. The threading model here
is
> that only the owning thread will ever modify a ThreadData, and so it is safe
for
> it to read it any old how.
> I have no idea whether direct access vs *_Load impacts performance, nor how
that
> breaks down on different CPU architectures. I'd just as soon not add that
> variable to this CL?
what I am saying is that all the rest of the code in this file seems to use
NoBarrier_Load, why this is special?
Also note how below you use NoBarrier_Store, so this access is inconsistent even
within this function.
In an ideal world NoBarrier_Load just evaluated to dereferencing a volatile
pointer.
In practice on Linux this is a bit slower because of the sysroot... long story
(crbug.com/592903) but that's going away.
https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c...
base/tracked_objects.cc:85: if (new_sum < *sum)
On 2016/11/16 21:30:28, Sigurður Ásgeirsson wrote:
> On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote:
> > I think that integer overflow is still considered undefined behavior.
> > There is some discussion in
> >
>
http://stackoverflow.com/questions/16188263/is-signed-integer-overflow-still-...
> >
> > Maybe just safer if you do
> > if ((added >= std::numeric_limits(...) - current_sum)
> > ...
>
> Ah, thanks. Went to base::CheckedNumeric - it seems to fit the bill?
ah perfect thanks.
https://codereview.chromium.org/2386123003/diff/500001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.cc (right):
https://codereview.chromium.org/2386123003/diff/500001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:150: const AllocatorDispatch* next =
allocator_dispatch.next;
add a compile_assert(std::is_pod(ThreadHeapUsage)), or this entire code is
really fragile. this works as long as the THP doesn't require a real ctor.
https://codereview.chromium.org/2386123003/diff/500001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:152: next->alloc_function(next,
sizeof(ThreadHeapUsage)));
maybe a placement new is slighlty cleaner here.
I think this needs to be a reinterpret_cast otherwise.
The CQ bit was checked by siggi@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...
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 siggi@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...
Thanks. I'm afraid the build flag has to stay, as I want to be able to turn this off cleanly in case there's moar random fallout like I fixed in the tcp_socket_win.cc file. Also brucedawson@ is going to skin me alive if I don't measure the performance impact of this on official builds properly. https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c... base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX) On 2016/11/28 18:38:31, Primiano Tucci wrote: > On 2016/11/16 21:30:28, Sigurður Ásgeirsson wrote: > > On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote: > > > I think you are supposed to use NoBarrier_Load and not dereference atomics > > > directly, it might break once we move to std::atomics otherwise > > > > This is modeled after all other Atomic32 (member) accesses in this file, which > > may all need a revamp once we we go to std::atomics. The threading model here > is > > that only the owning thread will ever modify a ThreadData, and so it is safe > for > > it to read it any old how. > > I have no idea whether direct access vs *_Load impacts performance, nor how > that > > breaks down on different CPU architectures. I'd just as soon not add that > > variable to this CL? > > what I am saying is that all the rest of the code in this file seems to use > NoBarrier_Load, why this is special? > Also note how below you use NoBarrier_Store, so this access is inconsistent even > within this function. No, this consistent with all other ThreadData member field access in this file. The way it works is that all in owner-thread READS are direct, all off owner-thread READs use NoBarrier_Load. All ThreadData member variable WRITEs us NoBarrier store, to support off owner-thread reads without race. The threading model here is that only the owner thread will ever modify these variables, so these objects aren't generally multithreaded. > In an ideal world NoBarrier_Load just evaluated to dereferencing a volatile > pointer. > In practice on Linux this is a bit slower because of the sysroot... long story > (crbug.com/592903) but that's going away. https://codereview.chromium.org/2386123003/diff/500001/base/debug/thread_heap... File base/debug/thread_heap_usage_tracker.cc (right): https://codereview.chromium.org/2386123003/diff/500001/base/debug/thread_heap... base/debug/thread_heap_usage_tracker.cc:150: const AllocatorDispatch* next = allocator_dispatch.next; On 2016/11/28 18:38:31, Primiano Tucci wrote: > add a compile_assert(std::is_pod(ThreadHeapUsage)), or this entire code is > really fragile. this works as long as the THP doesn't require a real ctor. Got one of those down a couple of lines, added another one here. https://codereview.chromium.org/2386123003/diff/500001/base/debug/thread_heap... base/debug/thread_heap_usage_tracker.cc:152: next->alloc_function(next, sizeof(ThreadHeapUsage))); On 2016/11/28 18:38:31, Primiano Tucci wrote: > maybe a placement new is slighlty cleaner here. > I think this needs to be a reinterpret_cast otherwise. Ah, placement new - cool - haven't used it for this purpose ever. What a great idea!
siggi@chromium.org changed reviewers: + dcheng@chromium.org, thakis@chromium.org - fdoray@chromium.org
Nico and Daniel, can I ask you for owners approval, please? As per the CL description, this shouldn't (yet) affect official builds, only developer builds and waterfalls. I'm going to add some finch-ing before I impose this on real users(tm).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fwiw still lgtm. good luck with all the rest ;-) https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/300001/base/tracked_objects.c... base/tracked_objects.cc:80: if (addend == 0U || *sum == INT_MAX) On 2016/11/30 14:41:13, Sigurður Ásgeirsson wrote: > On 2016/11/28 18:38:31, Primiano Tucci wrote: > > On 2016/11/16 21:30:28, Sigurður Ásgeirsson wrote: > > > On 2016/11/16 16:39:09, Primiano - slow(travelling) wrote: > > > > I think you are supposed to use NoBarrier_Load and not dereference atomics > > > > directly, it might break once we move to std::atomics otherwise > > > > > > This is modeled after all other Atomic32 (member) accesses in this file, > which > > > may all need a revamp once we we go to std::atomics. The threading model > here > > is > > > that only the owning thread will ever modify a ThreadData, and so it is safe > > for > > > it to read it any old how. > > > I have no idea whether direct access vs *_Load impacts performance, nor how > > that > > > breaks down on different CPU architectures. I'd just as soon not add that > > > variable to this CL? > > > > what I am saying is that all the rest of the code in this file seems to use > > NoBarrier_Load, why this is special? > > Also note how below you use NoBarrier_Store, so this access is inconsistent > even > > within this function. > > No, this consistent with all other ThreadData member field access in this file. > The way it works is that all in owner-thread READS are direct, all off > owner-thread READs use NoBarrier_Load. Uh! This is quite inconsistent, but at this point not something you are changing or making worse so not a point of discussion anymore for this CL. AFAIK there is no semantic difference between NoBarrier_Load and *, so not sure why the code here uses both. "*" just happen to work and is leveraring the implementation detail of base::subtle::atomic The only extra guarantee that NoBarrier_Load/Store give is atomicity, which: 1) you really want in all cases, as you don't want to see half integers 2) you get anyways, because on all platforms we support integers load/stores are intrinsically atomic. > All ThreadData member variable WRITEs us NoBarrier store, to support off > owner-thread reads without race. > > The threading model here is that only the owner thread will ever modify these > variables, so these objects aren't generally multithreaded. Right, but this is still not a reason for not using NoBarrier_Load/Store. "*" is leveraging an implementation detail of atomics and could potentially hit compiler optimizations (because you are dereferncing a non-volatile variable, so you are not telling the compiler that another thread might read/write it.)
https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.c... base/tracked_objects.cc:84: base::CheckedNumeric<int32_t> new_sum = *sum; How come it's safe to skip using an atomic load operation here? (I see primiano@ made the same comment, and I share the same concerns: see RecordDurations, which does use atomic loads to read out variables) https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.h... base/tracked_objects.h:254: class DeathData; Nit: newline after this for consistency https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.h... base/tracked_objects.h:437: // The cumulative number of allocation and free operations. Is this ordering still consistent with the comment on line 414?
https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.c... base/tracked_objects.cc:84: base::CheckedNumeric<int32_t> new_sum = *sum; On 2016/12/01 07:44:28, dcheng wrote: > How come it's safe to skip using an atomic load operation here? > > (I see primiano@ made the same comment, and I share the same concerns: see > RecordDurations, which does use atomic loads to read out variables) (Argh, upon looking again, I see that it's just one thing that decided to Load, everything else only uses atomics for store...) Still, it seems hard to understand why this is considered correct... guess I'll go do some reading.
The CQ bit was checked by siggi@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...
Thanks - another look? https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.c... base/tracked_objects.cc:84: base::CheckedNumeric<int32_t> new_sum = *sum; On 2016/12/01 07:44:28, dcheng wrote: > How come it's safe to skip using an atomic load operation here? > > (I see primiano@ made the same comment, and I share the same concerns: see > RecordDurations, which does use atomic loads to read out variables) This is done for consistency with all other DeathData member access. See ::RecordDurations for how this is originally authored. Presumably the original author has a performance argument for doing it this way, but I believe this is safe as only the owning thread will ever write DeathData members. There's also documentation at the top of tracked_objects.h on how all this machinery sacrifices absolute momentary correctness for performance and eventual consistency, which I believe is the right tradeoff here. I've turned this this function into a private member of the DeathData class to enforce that this won't be applied to anything but members of that class. Note that I'm in no way opposed to using the atomic methods for accessing these members. However, I think that change should be made separately, for all DeathData members, in a different CL. The performance impact of that CL should be measured VERY carefully, as I don't pretend to understand the perf implications to any of this. https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.c... base/tracked_objects.cc:84: base::CheckedNumeric<int32_t> new_sum = *sum; On 2016/12/01 07:48:26, dcheng wrote: > On 2016/12/01 07:44:28, dcheng wrote: > > How come it's safe to skip using an atomic load operation here? > > > > (I see primiano@ made the same comment, and I share the same concerns: see > > RecordDurations, which does use atomic loads to read out variables) > > (Argh, upon looking again, I see that it's just one thing that decided to Load, > everything else only uses atomics for store...) > > Still, it seems hard to understand why this is considered correct... guess I'll > go do some reading. Acknowledged. https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.h... base/tracked_objects.h:254: class DeathData; On 2016/12/01 07:44:28, dcheng wrote: > Nit: newline after this for consistency Done. https://codereview.chromium.org/2386123003/diff/560001/base/tracked_objects.h... base/tracked_objects.h:437: // The cumulative number of allocation and free operations. On 2016/12/01 07:44:28, dcheng wrote: > Is this ordering still consistent with the comment on line 414? Ah, good catch, thanks. Moved the samples back to the tail.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by siggi@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...
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 siggi@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...
The CQ bit was checked by siggi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis@chromium.org changed reviewers: + eroman@chromium.org
lgtm, but +eroman for chrome/browser/resources/profiler/OWNERS (looks right to me, but I'm not very familiar with that code) https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.c... base/tracked_objects.cc:181: // give the compiler the maximal opportunity to inline it, if appropriate. This comment only makes sense for compilers that due one-pass codegen. I think by now all C++ compilers out there (even MSVC, as of 2015 :-) ) build an AST and then codegen from that. I'd be surprised if the location of this function in this file made a difference on inlining decisions.
there's a lot of handwaving around the atomics here from my perspective, but it seems consistent with the code that's already there... so lgtm
Thanks Nico and Daniel. Eric for the chrome/browser/resources/profiler HTMLish stuff OWNERs review please? https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.c... base/tracked_objects.cc:181: // give the compiler the maximal opportunity to inline it, if appropriate. On 2016/12/02 01:45:12, Nico wrote: > This comment only makes sense for compilers that due one-pass codegen. I think > by now all C++ compilers out there (even MSVC, as of 2015 :-) ) build an AST and > then codegen from that. I'd be surprised if the location of this function in > this file made a difference on inlining decisions. I don't know much of modern compilers - should I move the method to declaration order, scrub the comment?
https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.c... base/tracked_objects.cc:181: // give the compiler the maximal opportunity to inline it, if appropriate. On 2016/12/02 15:34:28, Sigurður Ásgeirsson wrote: > On 2016/12/02 01:45:12, Nico wrote: > > This comment only makes sense for compilers that due one-pass codegen. I think > > by now all C++ compilers out there (even MSVC, as of 2015 :-) ) build an AST > and > > then codegen from that. I'd be surprised if the location of this function in > > this file made a difference on inlining decisions. > > I don't know much of modern compilers - should I move the method to declaration > order, scrub the comment? I'd do that unless there's evidence that shows putting it here helps.
The CQ bit was checked by siggi@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...
https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2386123003/diff/640001/base/tracked_objects.c... base/tracked_objects.cc:181: // give the compiler the maximal opportunity to inline it, if appropriate. On 2016/12/02 15:35:57, Nico wrote: > On 2016/12/02 15:34:28, Sigurður Ásgeirsson wrote: > > On 2016/12/02 01:45:12, Nico wrote: > > > This comment only makes sense for compilers that due one-pass codegen. I > think > > > by now all C++ compilers out there (even MSVC, as of 2015 :-) ) build an AST > > and > > > then codegen from that. I'd be surprised if the location of this function in > > > this file made a difference on inlining decisions. > > > > I don't know much of modern compilers - should I move the method to > declaration > > order, scrub the comment? > > I'd do that unless there's evidence that shows putting it here helps. Done.
LGTM chrome/browser/resources/profiler/* https://codereview.chromium.org/2386123003/diff/660001/chrome/browser/resourc... File chrome/browser/resources/profiler/profiler.js (right): https://codereview.chromium.org/2386123003/diff/660001/chrome/browser/resourc... chrome/browser/resources/profiler/profiler.js:131: var KEY_AVG_ALLOC_OPS = END_KEY++; I suggest giving these a common prefix, like "KEY_MEMORY_" to more easily recognize these (given that they are conditionally defined).
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 siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, thakis@chromium.org, dcheng@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2386123003/#ps680001 (title: "Address Eric's comment.")
Thanks, I'm going to try and land this. https://codereview.chromium.org/2386123003/diff/660001/chrome/browser/resourc... File chrome/browser/resources/profiler/profiler.js (right): https://codereview.chromium.org/2386123003/diff/660001/chrome/browser/resourc... chrome/browser/resources/profiler/profiler.js:131: var KEY_AVG_ALLOC_OPS = END_KEY++; On 2016/12/02 17:13:37, eroman (slow) wrote: > I suggest giving these a common prefix, like "KEY_MEMORY_" to more easily > recognize these (given that they are conditionally defined). Ah, good idea!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 680001, "attempt_start_ts": 1480705063642470,
"parent_rev": "79b513cf89afb8bd51b0c2e5dd9e78ef13f7d445", "commit_rev":
"5e16e695dc82665f475a1287c714ea0850967c4d"}
Message was sent while issue was closed.
Description was changed from ========== Add heap allocator usage to task profiler. This enables heap tracking in the task profiler (chrome://profiler) under a build flag, which is enabled for developer builds, but disabled for official builds for now. As-is, this surfaces the following heap metrics per task by default: - Avg number of allocations per run. - Avg number of frees per run. - Avg net bytes allocated (or freed if negative). - Max allocated bytes at any time in any run. This is a high-watermark for memory usage in the task. Additionally it's possible to enable the following metrics by ticking a checkbox in chrome://profiler: - Allocation count. - Free count. A running count of how many allocations and frees a task has performed in all runs. - Allocated bytes. - Freed bytes. A running tally of how many bytes a task has allocated and freed in all runs. - Overhead bytes. An estimate of how many allocated bytes go to heap overhead. This might be helpful to guide implementation to use larger allocations or chunked data structures like std::vector BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add heap allocator usage to task profiler. This enables heap tracking in the task profiler (chrome://profiler) under a build flag, which is enabled for developer builds, but disabled for official builds for now. As-is, this surfaces the following heap metrics per task by default: - Avg number of allocations per run. - Avg number of frees per run. - Avg net bytes allocated (or freed if negative). - Max allocated bytes at any time in any run. This is a high-watermark for memory usage in the task. Additionally it's possible to enable the following metrics by ticking a checkbox in chrome://profiler: - Allocation count. - Free count. A running count of how many allocations and frees a task has performed in all runs. - Allocated bytes. - Freed bytes. A running tally of how many bytes a task has allocated and freed in all runs. - Overhead bytes. An estimate of how many allocated bytes go to heap overhead. This might be helpful to guide implementation to use larger allocations or chunked data structures like std::vector BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #33 (id:680001)
Message was sent while issue was closed.
Description was changed from ========== Add heap allocator usage to task profiler. This enables heap tracking in the task profiler (chrome://profiler) under a build flag, which is enabled for developer builds, but disabled for official builds for now. As-is, this surfaces the following heap metrics per task by default: - Avg number of allocations per run. - Avg number of frees per run. - Avg net bytes allocated (or freed if negative). - Max allocated bytes at any time in any run. This is a high-watermark for memory usage in the task. Additionally it's possible to enable the following metrics by ticking a checkbox in chrome://profiler: - Allocation count. - Free count. A running count of how many allocations and frees a task has performed in all runs. - Allocated bytes. - Freed bytes. A running tally of how many bytes a task has allocated and freed in all runs. - Overhead bytes. An estimate of how many allocated bytes go to heap overhead. This might be helpful to guide implementation to use larger allocations or chunked data structures like std::vector BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add heap allocator usage to task profiler. This enables heap tracking in the task profiler (chrome://profiler) under a build flag, which is enabled for developer builds, but disabled for official builds for now. As-is, this surfaces the following heap metrics per task by default: - Avg number of allocations per run. - Avg number of frees per run. - Avg net bytes allocated (or freed if negative). - Max allocated bytes at any time in any run. This is a high-watermark for memory usage in the task. Additionally it's possible to enable the following metrics by ticking a checkbox in chrome://profiler: - Allocation count. - Free count. A running count of how many allocations and frees a task has performed in all runs. - Allocated bytes. - Freed bytes. A running tally of how many bytes a task has allocated and freed in all runs. - Overhead bytes. An estimate of how many allocated bytes go to heap overhead. This might be helpful to guide implementation to use larger allocations or chunked data structures like std::vector BUG=644385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/de38d0cc1f554562dc9062c97079585cc6522fe7 Cr-Commit-Position: refs/heads/master@{#436003} ==========
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/de38d0cc1f554562dc9062c97079585cc6522fe7 Cr-Commit-Position: refs/heads/master@{#436003} |
