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

Issue 1265393007: Reland 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

Reland of [tracing] Fix, simplify and speed up accounting of TraceEvent memory overhead (patchset #2 id:110001 of https://codereview.chromium.org/1258903003/ ) Reason for revert: Reverting did not solve crbug.com/516056 Original issue's 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} TBR=dsinclair@chromium.org,primiano@chromium.org,dyen@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=512383 Committed: https://crrev.com/20a0811a97a03cf3db4c98a96cefb5cad6070026 Cr-Commit-Position: refs/heads/master@{#342157}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -40 lines) Patch
M base/trace_event/trace_buffer.h View 1 chunk +1 line, -1 line 0 comments Download
M base/trace_event/trace_buffer.cc View 2 chunks +30 lines, -18 lines 0 comments Download
M base/trace_event/trace_event_impl.h View 1 chunk +0 lines, -1 line 0 comments Download
M base/trace_event/trace_event_impl.cc View 2 chunks +12 lines, -20 lines 0 comments Download
M base/trace_event/trace_event_memory_overhead.h View 1 chunk +3 lines, -0 lines 0 comments Download
M base/trace_event/trace_event_memory_overhead.cc View 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
oystein (OOO til 10th of July)
Created Reland of [tracing] Fix, simplify and speed up accounting of TraceEvent memory overhead
5 years, 4 months ago (2015-08-06 18:39:36 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265393007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265393007/1
5 years, 4 months ago (2015-08-06 18:40:22 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 4 months ago (2015-08-06 18:41:55 UTC) #3
commit-bot: I haz the power
5 years, 4 months ago (2015-08-06 18:42:38 UTC) #4
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/20a0811a97a03cf3db4c98a96cefb5cad6070026
Cr-Commit-Position: refs/heads/master@{#342157}

Powered by Google App Engine
This is Rietveld 408576698