Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(100)

Issue 1258903003: Revert of [tracing] Fix, simplify and speed up accounting of TraceEvent memory overhead (Closed)

Created:
5 years, 4 months ago by oystein (OOO til 10th of July)
Modified:
5 years, 4 months ago
CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of [tracing] Fix, simplify and speed up accounting of TraceEvent memory overhead (patchset #4 id:60001 of https://codereview.chromium.org/1251203003/ ) Reason for revert: Speculative revert for https://code.google.com/p/chromium/issues/detail?id=516056 Original issue's description: > [tracing] Fix, simplify and speed up accounting of TraceEvent memory overhead > > So far the accounting of TraceEvent memory overhead worked as follows: > - Each TraceEvent keeps a cache of its own overhead. > - The TraceBufferChunk (which is approx an array of 64 TraceEvent) > keeps a cache of the all 64 TraceEvent, but only when completelly > filled. > > This is sub-optimal because: > - There can be many TraceEvent(s), 10k-100k in a bulky 30s trace: > when memory-infra is enabled, this causes a cost of several MB to > keep the aforementioned per-TraceEvent caches. > - Every time we have to estimate the size of a non-completely-full > TraceBufferChunk, we still have to loop over their individual caches. > - Least but not last, turns out that the current accounting is just > wrong, as we ended up counting the sizeof(TraceEvent) both in > TraceBufferChunk and in the individual TraceEvent. > > This CL improves the situation as follows: > - The per-TraceEvent cache is dropped. Much less memory is used. > - The TraceBufferChunk still keeps a cache, but its cache is used in a > smarter way and not just when it is full. We simply remember how many > TraceEvent(s) we estimated in the previous pass and then estimate just > the missing ones. > - The double accounting issue is fixed, as now we count TraceEvent(s) > only in TraceBufferChunk. > > BUG=512383 > > Committed: https://crrev.com/b2ae97fb331cbad132a8e1aad9a25441fde75048 > Cr-Commit-Position: refs/heads/master@{#340005} TBR=dsinclair@chromium.org,primiano@chromium.org BUG=512383 Committed: https://crrev.com/8f329fc15f5fa64f64308244266a304c122a2868 Cr-Commit-Position: refs/heads/master@{#341910}

Patch Set 1 #

Patch Set 2 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -49 lines) Patch
M base/trace_event/trace_buffer.h View 1 1 chunk +1 line, -1 line 0 comments Download
M base/trace_event/trace_buffer.cc View 1 2 chunks +15 lines, -27 lines 0 comments Download
M base/trace_event/trace_event_impl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M base/trace_event/trace_event_impl.cc View 1 2 chunks +19 lines, -11 lines 0 comments Download
M base/trace_event/trace_event_memory_overhead.h View 1 1 chunk +0 lines, -3 lines 0 comments Download
M base/trace_event/trace_event_memory_overhead.cc View 1 1 chunk +0 lines, -7 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
oystein (OOO til 10th of July)
Created Revert of [tracing] Fix, simplify and speed up accounting of TraceEvent memory overhead
5 years, 4 months ago (2015-08-04 20:16:13 UTC) #1
David Yen
Looks like this fixed the flakiness, lets get this in and monitor the trybots more. ...
5 years, 4 months ago (2015-08-04 23:17:46 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258903003/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258903003/110001
5 years, 4 months ago (2015-08-04 23:19:17 UTC) #5
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/94951)
5 years, 4 months ago (2015-08-05 00:39:31 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258903003/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258903003/110001
5 years, 4 months ago (2015-08-05 16:05:37 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:110001)
5 years, 4 months ago (2015-08-05 16:11:03 UTC) #10
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/8f329fc15f5fa64f64308244266a304c122a2868 Cr-Commit-Position: refs/heads/master@{#341910}
5 years, 4 months ago (2015-08-05 16:12:03 UTC) #11
oystein (OOO til 10th of July)
5 years, 4 months ago (2015-08-06 18:39:36 UTC) #12
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:110001) has been created in
https://codereview.chromium.org/1265393007/ by oysteine@chromium.org.

The reason for reverting is: Reverting did not solve crbug.com/516056.

Powered by Google App Engine
This is Rietveld 408576698