Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2859493002:
Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation (Closed)
|
Created:
3 years, 7 months ago by Sigurður Ásgeirsson Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation.
Since Chrome churns heap memory at a prodigious rate, it only takes an hour or two for the 32 bit byte counts to saturate. After saturation occurs, the derived memory metrics described in https://sites.google.com/a/chromium.org/dev/developers/threaded-task-tracking run off the rails and become useless.
BUG=644385
Review-Url: https://codereview.chromium.org/2859493002
Cr-Commit-Position: refs/heads/master@{#469145}
Committed: https://chromium.googlesource.com/chromium/src/+/f467a630a42f536db8a502d820b83266c4342f6b
Patch Set 1 #Patch Set 2 : Move to 64 bit storage on 32 bit systems, using an even/odd locking scheme. #
Total comments: 2
Patch Set 3 : Fix 64 bit, now may actually compile! #Patch Set 4 : Address Chris' comment. #
Total comments: 16
Patch Set 5 : Address Gab's comments, except the ThreadChecker one. #Patch Set 6 : Change to Barrier writes and Acquire reads as per our offline discussion. #
Total comments: 6
Patch Set 7 : Address Gab's comments. #
Total comments: 15
Patch Set 8 : Address Gabs comments. #Patch Set 9 : Fix 64 bit compile, doofus!. #
Messages
Total messages: 48 (33 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...
Description was changed from ========== Tracked objects: Bump cumulative byte count storage to 64 bits on 64 bit systems to avoid saturation. BUG= ========== to ========== Tracked objects: Bump cumulative byte count storage to 64 bits on 64 bit systems to avoid saturation. BUG=644385 ==========
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
Description was changed from ========== Tracked objects: Bump cumulative byte count storage to 64 bits on 64 bit systems to avoid saturation. BUG=644385 ========== to ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. BUG=644385 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
siggi@chromium.org changed reviewers: + chrisha@chromium.org, gab@chromium.org
Hey Chris, please review for functionality, Gab for owners pretty please. Siggi
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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 mechanism lgtm! https://codereview.chromium.org/2859493002/diff/20001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/20001/base/tracked_objects.cc... base/tracked_objects.cc:215: // read 64 bit sums consistently. Maybe a small repeat of the fact that *writing* can only occur on a single thread by design, so there's no race to lock the data?
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Thanks, comment added. https://codereview.chromium.org/2859493002/diff/20001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/20001/base/tracked_objects.cc... base/tracked_objects.cc:215: // read 64 bit sums consistently. On 2017/05/02 18:48:32, chrisha wrote: > Maybe a small repeat of the fact that *writing* can only occur on a single > thread by design, so there's no race to lock the data? 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: This issue passed the CQ dry run.
The bug doesn't mention issues about limits being reached here? Is there a specific use case that justifies this? Please document in CL description. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:216: // thread owning this DeathData, there's no race on these writes. Can we have a base::ThreadChecker check confirm that these writes are always on the same thread? This scheme depends on there being a single writer of |byte_update_counter_|. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:292: while (true) { This can theoretically starve readers. I assume this data is only consumed in developer-only UIs where this matters less? Is the API clear about that? https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:297: } while (update_counter & 1); PlatformThread::YieldCurrentThread() after each failed cycle https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:304: static_cast<uint32_t>(base::subtle::NoBarrier_Load(&count->lo_word)); I'm not convinced that NoBarrier is what you want here and elsewhere. NoBarrier means that reads/writes can be reordered (i.e. you could see the desired |update_counter| but only half the writes to |count| -- and the final validity check in this method doesn't change that). NoBarrier is fine in the single variable use case because it's merely reads/writes of a single piece of data (without caring about relative ordering of other reads/writes on involved threads), but now ordering does matter as you depend on one piece of state being set one way to assume things about other pieces of state. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:308: if (update_counter == base::subtle::NoBarrier_Load(&byte_update_counter_)) Can't this check see the same value twice while there was a write in the middle since you're decrementing at the end of DeathData::RecordAllocation() instead of rolling forward to even? Should it merely roll forward? If merely moving forward and back, it's no better than a "writing"/"not writing" flag which has this issue as well. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:328: if (addend == 0U || CumulativeByteCountRead(sum) == LONG_MAX) Use std::numeric_limits<int64_t>::max() instead of LONG_MAX here and below https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects_un... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects_un... base/tracked_objects_unittest.cc:350: data->RecordAllocations(INT_MAX, INT_MAX, INT_MAX, INT_MAX, INT_MAX, INT_MAX); INT_MAX no longer saturates though right? https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects_un... base/tracked_objects_unittest.cc:362: } Do we have any tests that verify the new range beyond 32-bit integers?
Description was changed from ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. BUG=644385 ========== to ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. Since Chrome churns heap memory at a prodigious rate, it only takes an hour or two for the 32 bit byte counts to saturate. After saturation occurs, the derived memory metrics described in <https://sites.google.com/a/chromium.org/dev/developers/threaded-task-tracking> run off the rails and become useless. BUG=644385 ==========
I've addressed everything but the ThreadChecker, which warrants a separate patch methinks (if it works). https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:292: while (true) { On 2017/05/02 21:17:12, gab wrote: > This can theoretically starve readers. I assume this data is only consumed in > developer-only UIs where this matters less? Is the API clear about that? I don't know where you'd see this documented, but this is only active under "--enable-heap-profiling=task-profiler", as sadly there's sufficient performance impact to this that it shows on benchmarks. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:297: } while (update_counter & 1); On 2017/05/02 21:17:12, gab wrote: > PlatformThread::YieldCurrentThread() after each failed cycle Mmmm, I added a spin count to yield. These reads happen on the main UI thread, as I understand it... https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:304: static_cast<uint32_t>(base::subtle::NoBarrier_Load(&count->lo_word)); On 2017/05/02 21:17:12, gab wrote: > I'm not convinced that NoBarrier is what you want here and elsewhere. > > NoBarrier means that reads/writes can be reordered (i.e. you could see the > desired |update_counter| but only half the writes to |count| -- and the final > validity check in this method doesn't change that). > > NoBarrier is fine in the single variable use case because it's merely > reads/writes of a single piece of data (without caring about relative ordering > of other reads/writes on involved threads), but now ordering does matter as you > depend on one piece of state being set one way to assume things about other > pieces of state. The final (now-)increment to the guarding counter is a barrier write, which - to my understanding - makes all previous writes on that thread/core consistent to other cores. These reads can then be inconsistent only if there are concurrent updates, which is (now) guarded by the update counter. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:308: if (update_counter == base::subtle::NoBarrier_Load(&byte_update_counter_)) On 2017/05/02 21:17:12, gab wrote: > Can't this check see the same value twice while there was a write in the middle > since you're decrementing at the end of DeathData::RecordAllocation() instead of > rolling forward to even? Should it merely roll forward? If merely moving forward > and back, it's no better than a "writing"/"not writing" flag which has this > issue as well. You're absolutely right and that was my intent. I guess I zoned in a copy/paste someplace. Thanks! https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:328: if (addend == 0U || CumulativeByteCountRead(sum) == LONG_MAX) On 2017/05/02 21:17:12, gab wrote: > Use std::numeric_limits<int64_t>::max() instead of LONG_MAX here and below Ah, new hotness, thanks! https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects_un... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects_un... base/tracked_objects_unittest.cc:350: data->RecordAllocations(INT_MAX, INT_MAX, INT_MAX, INT_MAX, INT_MAX, INT_MAX); On 2017/05/02 21:17:12, gab wrote: > INT_MAX no longer saturates though right? Comment amended to moar specific specificity. https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects_un... base/tracked_objects_unittest.cc:362: } On 2017/05/02 21:17:12, gab wrote: > Do we have any tests that verify the new range beyond 32-bit integers? The test above now goes to 2^32 + some.
Thanks - pleas take another look? https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/60001/base/tracked_objects.cc... base/tracked_objects.cc:216: // thread owning this DeathData, there's no race on these writes. On 2017/05/02 21:17:12, gab wrote: > Can we have a base::ThreadChecker check confirm that these writes are always on > the same thread? This scheme depends on there being a single writer of > |byte_update_counter_|. I looked into this, and it's not straightforward to do. The object that has thread affinity is ThreadData, which in turn keeps a stash of these DeathData objects. The problem is that now LuckyLuke is churning threads, the ThreadData objects are thread-promiscuous, though they associate with zero or one threads at any time. The threading model here is ancient as time, quite well documented and deeply embedded throughout this implementation. I'd beg off from fitting a ThreadChecker in there, unless you strongly object.
The CQ bit was checked by siggi@chromium.org to run a CQ dry run
Description was changed from ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. Since Chrome churns heap memory at a prodigious rate, it only takes an hour or two for the 32 bit byte counts to saturate. After saturation occurs, the derived memory metrics described in <https://sites.google.com/a/chromium.org/dev/developers/threaded-task-tracking> run off the rails and become useless. BUG=644385 ========== to ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. Since Chrome churns heap memory at a prodigious rate, it only takes an hour or two for the 32 bit byte counts to saturate. After saturation occurs, the derived memory metrics described in https://sites.google.com/a/chromium.org/dev/developers/threaded-task-tracking run off the rails and become useless. BUG=644385 ==========
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.
https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.c... base/tracked_objects.cc:303: constexpr size_t kYieldProcessorTries = 10000; spin_lock.cc uses 1000 not 10000 spin_lock.cc also uses YieldProcessor() to inform the processor that this code is in a busy loop (e.g. yielding resources to other threads without giving up this thread's quantum on hyperthreaded CPUs) Short of bringing YieldProcessor() here, I'm not sure what's worse... spinning to 1000 and yielding thread less often or yielding thread every cycle but first? FWIW, one_writer_seqlock.cc does the latter. https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.c... base/tracked_objects.cc:313: update_counter = base::subtle::Acquire_Load(&byte_update_counter_); optional: After reading http://preshing.com/20130922/acquire-and-release-fences/ (in particular example around std::atomic_thread_fence(std::memory_order_acquire)) I think you could avoid the Acquire here if you add a base::subtle::MemoryBarrier() right after exiting the loop. That barrier would guarantee that you see everything that was written prior to the value of |byte_update_counter_| you just saw (i.e. same property as Acquire_Load(). base::subtle::MemoryBarrier() is a full-on barrier whereas you just need acquire semantics, but maybe a full barrier outside the loop is better than an acquire barrier inside the loop..? https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.c... base/tracked_objects.cc:325: if (update_counter == base::subtle::Acquire_Load(&byte_update_counter_)) Acquire semantics at the end of the method do nothing (they only guarantee that reads coming after are consistent which is irrelevant). I think you want Release_Load() -- which is implemented as MemoryBarrier() + NoBarrier_Load() @ https://cs.chromium.org/chromium/src/base/atomicops_internals_portable.h?rcl=... so that would make sense as it would fence the above reads. Looking in the codebase for existing usage of Release_Load() I found one_writer_seqlock.h which seems very similar to what you're trying to do.
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, good enough yet? https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.c... base/tracked_objects.cc:303: constexpr size_t kYieldProcessorTries = 10000; On 2017/05/03 17:25:37, gab wrote: > spin_lock.cc uses 1000 not 10000 > > spin_lock.cc also uses YieldProcessor() to inform the processor that this code > is in a busy loop (e.g. yielding resources to other threads without giving up > this thread's quantum on hyperthreaded CPUs) > > Short of bringing YieldProcessor() here, I'm not sure what's worse... spinning > to 1000 and yielding thread less often or yielding thread every cycle but first? > > FWIW, one_writer_seqlock.cc does the latter. Ugh, typo alert! Lemme try it this way - like you say, I'll be the first to feel the pain :). https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.c... base/tracked_objects.cc:313: update_counter = base::subtle::Acquire_Load(&byte_update_counter_); On 2017/05/03 17:25:37, gab wrote: > optional: > > After reading http://preshing.com/20130922/acquire-and-release-fences/ (in > particular example around std::atomic_thread_fence(std::memory_order_acquire)) > > I think you could avoid the Acquire here if you add a > base::subtle::MemoryBarrier() right after exiting the loop. > > That barrier would guarantee that you see everything that was written prior to > the value of |byte_update_counter_| you just saw (i.e. same property as > Acquire_Load(). > > base::subtle::MemoryBarrier() is a full-on barrier whereas you just need acquire > semantics, but maybe a full barrier outside the loop is better than an acquire > barrier inside the loop..? Done. https://codereview.chromium.org/2859493002/diff/100001/base/tracked_objects.c... base/tracked_objects.cc:325: if (update_counter == base::subtle::Acquire_Load(&byte_update_counter_)) On 2017/05/03 17:25:37, gab wrote: > Acquire semantics at the end of the method do nothing (they only guarantee that > reads coming after are consistent which is irrelevant). > > I think you want Release_Load() -- which is implemented as MemoryBarrier() + > NoBarrier_Load() @ > https://cs.chromium.org/chromium/src/base/atomicops_internals_portable.h?rcl=... > so that would make sense as it would fence the above reads. > > Looking in the codebase for existing usage of Release_Load() I found > one_writer_seqlock.h which seems very similar to what you're trying to do. Ah, cool, that makes sense.
Thanks, good enough yet?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lg, one last full pass, cheers! https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:279: int64_t DeathData::CumulativeByteCountRead(const CumulativeByteCount* count) { // static https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:284: static_cast<uint32_t>(count->lo_word); Atomics should always be read using atomic ops (i.e. shouldn't assume underlying typedef), please use NoBarrier_Load() for these 3 accesses (on x86 these are merely read the way you do it here [1] but reading directly isn't correct on all architectures). [1] https://cs.chromium.org/chromium/src/base/atomicops_internals_x86_msvc.h?type... https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:327: // Otherwise go around and try again. // Release_Load() semantics here ensure that the |byte_update_counter_| value seen is at least as old as the |hi_word|/|lo_word| values seen above -- which means that if it's still equal to |update_counter|, the read is consistent, since the above MemoryBarrier() ensures they're at least as new as the afore-obtained |update_counter|'s value. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:334: void DeathData::SaturatingMemberAdd(const uint32_t addend, // static https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:359: #else For documentation: // This must only be called while the update counter is "locked" (i.e. odd). DCHECK_EQ(base::subtle::NoBarrier_Load(&byte_update_counter_) & 1, 1); https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.h... base/tracked_objects.h:432: static int64_t CumulativeByteCountRead(const CumulativeByteCount* count); "UnsafeCumulativeByteCountRead" to explicitly document that this is dangerous and should only be used where intended to. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.h... base/tracked_objects.h:440: void SaturatingByteCountMemberAdd(const uint32_t addend, Document this method
Thanks - one moar look? https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:279: int64_t DeathData::CumulativeByteCountRead(const CumulativeByteCount* count) { On 2017/05/03 19:40:33, gab wrote: > // static Done. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:284: static_cast<uint32_t>(count->lo_word); On 2017/05/03 19:40:33, gab wrote: > Atomics should always be read using atomic ops (i.e. shouldn't assume underlying > typedef), please use NoBarrier_Load() for these 3 accesses (on x86 these are > merely read the way you do it here [1] but reading directly isn't correct on all > architectures). > > [1] > https://cs.chromium.org/chromium/src/base/atomicops_internals_x86_msvc.h?type... I did this for consistency with the rest of this code, which is then likewise incorrect. See e.g. DeathData::RecordDurations. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:327: // Otherwise go around and try again. On 2017/05/03 19:40:33, gab wrote: > // Release_Load() semantics here ensure that the |byte_update_counter_| value > seen is at least as old as the |hi_word|/|lo_word| values seen above -- which > means that if it's still equal to |update_counter|, the read is consistent, > since the above MemoryBarrier() ensures they're at least as new as the > afore-obtained |update_counter|'s value. Done. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:334: void DeathData::SaturatingMemberAdd(const uint32_t addend, On 2017/05/03 19:40:33, gab wrote: > // static Done. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:359: #else On 2017/05/03 19:40:33, gab wrote: > For documentation: > > // This must only be called while the update counter is "locked" (i.e. odd). > DCHECK_EQ(base::subtle::NoBarrier_Load(&byte_update_counter_) & 1, 1); Done. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.h... base/tracked_objects.h:432: static int64_t CumulativeByteCountRead(const CumulativeByteCount* count); On 2017/05/03 19:40:33, gab wrote: > "UnsafeCumulativeByteCountRead" to explicitly document that this is dangerous > and should only be used where intended to. Done. https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.h... base/tracked_objects.h:440: void SaturatingByteCountMemberAdd(const uint32_t addend, On 2017/05/03 19:40:33, gab wrote: > Document this method Done.
lgtm, thanks! https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2859493002/diff/120001/base/tracked_objects.c... base/tracked_objects.cc:284: static_cast<uint32_t>(count->lo_word); On 2017/05/03 20:10:10, Sigurður Ásgeirsson wrote: > On 2017/05/03 19:40:33, gab wrote: > > Atomics should always be read using atomic ops (i.e. shouldn't assume > underlying > > typedef), please use NoBarrier_Load() for these 3 accesses (on x86 these are > > merely read the way you do it here [1] but reading directly isn't correct on > all > > architectures). > > > > [1] > > > https://cs.chromium.org/chromium/src/base/atomicops_internals_x86_msvc.h?type... > > I did this for consistency with the rest of this code, which is then likewise > incorrect. See e.g. DeathData::RecordDurations. Most of that methods uses atomic ops, I'm seeing if (count_ < INT_MAX) which is incorrect though, I may have missed a few others. But yes, those are incorrect and should be fixed too (ideally the type would prevent this, it's too easy to forget with the typedef...).
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2859493002/#ps140001 (title: "Address Gabs comments.")
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
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2859493002/#ps160001 (title: "Fix 64 bit compile, doofus!.")
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": 160001, "attempt_start_ts": 1493843988566030,
"parent_rev": "c1a8b5d06d97e880ac05822a26570a176330b57d", "commit_rev":
"f467a630a42f536db8a502d820b83266c4342f6b"}
Message was sent while issue was closed.
Description was changed from ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. Since Chrome churns heap memory at a prodigious rate, it only takes an hour or two for the 32 bit byte counts to saturate. After saturation occurs, the derived memory metrics described in https://sites.google.com/a/chromium.org/dev/developers/threaded-task-tracking run off the rails and become useless. BUG=644385 ========== to ========== Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation. Since Chrome churns heap memory at a prodigious rate, it only takes an hour or two for the 32 bit byte counts to saturate. After saturation occurs, the derived memory metrics described in https://sites.google.com/a/chromium.org/dev/developers/threaded-task-tracking run off the rails and become useless. BUG=644385 Review-Url: https://codereview.chromium.org/2859493002 Cr-Commit-Position: refs/heads/master@{#469145} Committed: https://chromium.googlesource.com/chromium/src/+/f467a630a42f536db8a502d820b8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f467a630a42f536db8a502d820b8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
