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 2511093002: [counters] RuntimeStats: fix wrong bookkeeping when dynamically changing counters. (Closed)

Created:
4 years, 1 month ago by Camillo Bruni
Modified:
4 years ago
CC:
fmeawad, v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[counters] RuntimeStats: fix wrong bookkeeping when dynamically changing counters RuntimeTimerScopes always subtract their own time from the parent timer's counter to properly account for the own time. Once a scope is destructed it adds it own timer to the current active counter. However, if the current counter is changed with CorrectCurrentCounterId we will attribute all the subtimers to the previous counter, and add the own time to the new counter. This way it is possible to end up with negative times in certain counters but the overall would still be correct. BUG= Committed: https://crrev.com/244dd002c50dca663cb41686917e2006149e29e8 Cr-Commit-Position: refs/heads/master@{#41254}

Patch Set 1 #

Total comments: 4

Patch Set 2 : formatting #

Total comments: 8

Patch Set 3 : partly addressing nits and implementing unittests #

Patch Set 4 : fix windows #

Patch Set 5 : moar tests #

Total comments: 20

Patch Set 6 : addressing comments #

Patch Set 7 : adding more comments #

Patch Set 8 : more comments #

Total comments: 3

Patch Set 9 : fixing compilation #

Patch Set 10 : trying to fix win #

Patch Set 11 : rebasing for real #

Patch Set 12 : using the stopwatch principle to pause the outer timer #

Patch Set 13 : adding subtime_ field on RuntimeCallTimer #

Total comments: 3

Patch Set 14 : succumbing to igors will :D #

Patch Set 15 : reverting elapsed-timer.h #

Patch Set 16 : increase test epsilon to 20ms #

Patch Set 17 : merge with master #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+456 lines, -92 lines) Patch
M src/counters.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +62 lines, -73 lines 0 comments Download
M src/counters.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +30 lines, -16 lines 0 comments Download
M src/counters-inl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +51 lines, -0 lines 0 comments Download
M src/log.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M src/profiler/profile-generator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -2 lines 0 comments Download
M test/unittests/counters-unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +310 lines, -0 lines 1 comment Download

Messages

Total messages: 91 (60 generated)
Camillo Bruni
PTAL https://codereview.chromium.org/2511093002/diff/1/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2511093002/diff/1/src/counters-inl.h#newcode31 src/counters-inl.h:31: parent()->Subtract(delta); newly added Subtract https://codereview.chromium.org/2511093002/diff/1/src/counters.h File src/counters.h (left): ...
4 years, 1 month ago (2016-11-17 14:02:00 UTC) #2
Igor Sheludko
https://codereview.chromium.org/2511093002/diff/1/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2511093002/diff/1/src/counters-inl.h#newcode38 src/counters-inl.h:38: // from the current counter. This way we can ...
4 years, 1 month ago (2016-11-18 09:55:45 UTC) #9
Camillo Bruni
PTAL again https://codereview.chromium.org/2511093002/diff/20001/src/base/platform/elapsed-timer.h File src/base/platform/elapsed-timer.h (right): https://codereview.chromium.org/2511093002/diff/20001/src/base/platform/elapsed-timer.h#newcode79 src/base/platform/elapsed-timer.h:79: start_ticks_ -= delta; On 2016/11/18 at 09:55:45, ...
4 years, 1 month ago (2016-11-18 16:36:48 UTC) #16
Igor Sheludko
Almost there :) https://codereview.chromium.org/2511093002/diff/80001/src/base/platform/elapsed-timer.h File src/base/platform/elapsed-timer.h (right): https://codereview.chromium.org/2511093002/diff/80001/src/base/platform/elapsed-timer.h#newcode57 src/base/platform/elapsed-timer.h:57: TimeDelta Restart(TimeTicks ticks) { s/ticks/now/ to ...
4 years, 1 month ago (2016-11-18 18:56:28 UTC) #19
Igor Sheludko
https://codereview.chromium.org/2511093002/diff/80001/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2511093002/diff/80001/src/counters-inl.h#newcode47 src/counters-inl.h:47: base::TimeDelta delta; On 2016/11/18 18:56:27, Igor Sheludko wrote: > ...
4 years, 1 month ago (2016-11-20 23:47:16 UTC) #20
Camillo Bruni
PTAL again https://codereview.chromium.org/2511093002/diff/80001/src/base/platform/elapsed-timer.h File src/base/platform/elapsed-timer.h (right): https://codereview.chromium.org/2511093002/diff/80001/src/base/platform/elapsed-timer.h#newcode57 src/base/platform/elapsed-timer.h:57: TimeDelta Restart(TimeTicks ticks) { On 2016/11/18 at ...
4 years, 1 month ago (2016-11-21 10:50:03 UTC) #23
Igor Sheludko
lgtm modulo nits: https://codereview.chromium.org/2511093002/diff/140001/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2511093002/diff/140001/src/counters-inl.h#newcode47 src/counters-inl.h:47: base::TimeDelta delta = base::TimeDeltae::FromMicroseconds(0); s/TimeDeltae/TimeDelta/ https://codereview.chromium.org/2511093002/diff/140001/src/counters-inl.h#newcode50 ...
4 years, 1 month ago (2016-11-21 10:54:02 UTC) #26
Camillo Bruni
4 years, 1 month ago (2016-11-21 13:17:46 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2511093002/180001
4 years, 1 month ago (2016-11-21 13:18:06 UTC) #34
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 1 month ago (2016-11-21 13:45:10 UTC) #37
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/f6c74d964d9387df4bed3d8c1ded51eb9e8aa6e8 Cr-Commit-Position: refs/heads/master@{#41142}
4 years, 1 month ago (2016-11-21 13:45:36 UTC) #39
Camillo Bruni
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2519073002/ by cbruni@chromium.org. ...
4 years, 1 month ago (2016-11-21 16:00:22 UTC) #40
Camillo Bruni
PTAL again
4 years, 1 month ago (2016-11-22 13:21:58 UTC) #45
Camillo Bruni
PTAL again again :)
4 years, 1 month ago (2016-11-22 16:20:09 UTC) #50
lpy
LGTM, just have one concern. https://codereview.chromium.org/2511093002/diff/280001/src/counters.cc File src/counters.cc (right): https://codereview.chromium.org/2511093002/diff/280001/src/counters.cc#newcode301 src/counters.cc:301: delta = timer->Restart(now); I ...
4 years, 1 month ago (2016-11-22 22:25:44 UTC) #58
Igor Sheludko
https://codereview.chromium.org/2511093002/diff/280001/src/counters.cc File src/counters.cc (right): https://codereview.chromium.org/2511093002/diff/280001/src/counters.cc#newcode296 src/counters.cc:296: base::TimeDelta delta = base::TimeDelta(); Nit: you can drop = ...
4 years, 1 month ago (2016-11-22 22:53:59 UTC) #59
lpy
On 2016/11/22 22:53:59, Igor Sheludko wrote: > https://codereview.chromium.org/2511093002/diff/280001/src/counters.cc > File src/counters.cc (right): > > https://codereview.chromium.org/2511093002/diff/280001/src/counters.cc#newcode296 ...
4 years, 1 month ago (2016-11-22 23:02:04 UTC) #60
Igor Sheludko
lgtm
4 years, 1 month ago (2016-11-22 23:47:33 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2511093002/320001
4 years ago (2016-11-23 12:21:14 UTC) #68
commit-bot: I haz the power
Committed patchset #15 (id:320001)
4 years ago (2016-11-23 12:22:56 UTC) #70
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/491651792d7818aed04eaeffb9890b5a309b543e Cr-Commit-Position: refs/heads/master@{#41214}
4 years ago (2016-11-23 12:23:24 UTC) #72
Michael Achenbach
A revert of this CL (patchset #15 id:320001) has been created in https://codereview.chromium.org/2526843002/ by machenbach@chromium.org. ...
4 years ago (2016-11-23 15:27:02 UTC) #73
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2511093002/340001
4 years ago (2016-11-24 08:47:52 UTC) #76
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/builds/12737) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, ...
4 years ago (2016-11-24 08:50:05 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2511093002/360001
4 years ago (2016-11-24 09:37:53 UTC) #82
Michael Achenbach
https://codereview.chromium.org/2511093002/diff/360001/test/unittests/counters-unittest.cc File test/unittests/counters-unittest.cc (right): https://codereview.chromium.org/2511093002/diff/360001/test/unittests/counters-unittest.cc#newcode77 test/unittests/counters-unittest.cc:77: const uint32_t kEpsilonMs = 20; Hmm, is that rock ...
4 years ago (2016-11-24 09:48:46 UTC) #84
Camillo Bruni
On 2016/11/24 at 09:48:46, machenbach wrote: > https://codereview.chromium.org/2511093002/diff/360001/test/unittests/counters-unittest.cc > File test/unittests/counters-unittest.cc (right): > > https://codereview.chromium.org/2511093002/diff/360001/test/unittests/counters-unittest.cc#newcode77 ...
4 years ago (2016-11-24 09:59:38 UTC) #85
Michael Achenbach
On 2016/11/24 09:59:38, Camillo Bruni wrote: > On 2016/11/24 at 09:48:46, machenbach wrote: > > ...
4 years ago (2016-11-24 10:02:12 UTC) #86
commit-bot: I haz the power
Committed patchset #17 (id:360001)
4 years ago (2016-11-24 10:05:31 UTC) #88
commit-bot: I haz the power
Patchset 17 (id:??) landed as https://crrev.com/244dd002c50dca663cb41686917e2006149e29e8 Cr-Commit-Position: refs/heads/master@{#41254}
4 years ago (2016-11-24 10:05:48 UTC) #90
Nico
4 years ago (2016-11-28 22:37:23 UTC) #91
Message was sent while issue was closed.
This killed build performance, at least on Windows, see
https://bugs.chromium.org/p/chromium/issues/detail?id=668748

Powered by Google App Engine
This is Rietveld 408576698