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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1287073006: Correct inclusive time and start tracking maximum exclusive time (Closed)

Created:
5 years, 4 months ago by Cutch
Modified:
5 years, 4 months ago
Reviewers:
rmacnak, srdjan
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Base URL:
git@github.com:dart-lang/sdk.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Correct inclusive time and start tracking maximum exclusive time BUG= R=rmacnak@google.com Committed: https://github.com/dart-lang/sdk/commit/c0b6625d20eb3b4f5e8cef08ff9bb2c647ca745a

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -78 lines) Patch
M runtime/vm/timeline_analysis.h View 1 3 chunks +27 lines, -12 lines 0 comments Download
M runtime/vm/timeline_analysis.cc View 1 8 chunks +78 lines, -48 lines 6 comments Download
M runtime/vm/timeline_test.cc View 7 chunks +35 lines, -18 lines 0 comments Download

Messages

Total messages: 9 (2 generated)
Cutch
5 years, 4 months ago (2015-08-14 20:58:20 UTC) #2
rmacnak
lgtm https://chromiumcodereview.appspot.com/1287073006/diff/1/runtime/vm/timeline_analysis.h File runtime/vm/timeline_analysis.h (right): https://chromiumcodereview.appspot.com/1287073006/diff/1/runtime/vm/timeline_analysis.h#newcode191 runtime/vm/timeline_analysis.h:191: bool LabelOnStack(const char* label); IsLabelOnStack
5 years, 4 months ago (2015-08-14 21:24:02 UTC) #3
rmacnak
lgtm lgtm
5 years, 4 months ago (2015-08-14 21:24:03 UTC) #4
Cutch
Committed patchset #2 (id:20001) manually as c0b6625d20eb3b4f5e8cef08ff9bb2c647ca745a (presubmit successful).
5 years, 4 months ago (2015-08-14 21:47:33 UTC) #5
Cutch
https://codereview.chromium.org/1287073006/diff/1/runtime/vm/timeline_analysis.h File runtime/vm/timeline_analysis.h (right): https://codereview.chromium.org/1287073006/diff/1/runtime/vm/timeline_analysis.h#newcode191 runtime/vm/timeline_analysis.h:191: bool LabelOnStack(const char* label); On 2015/08/14 21:24:02, rmacnak wrote: ...
5 years, 4 months ago (2015-08-17 16:52:25 UTC) #6
srdjan
DBC https://codereview.chromium.org/1287073006/diff/20001/runtime/vm/timeline_analysis.cc File runtime/vm/timeline_analysis.cc (right): https://codereview.chromium.org/1287073006/diff/20001/runtime/vm/timeline_analysis.cc#newcode374 runtime/vm/timeline_analysis.cc:374: pause_info->OnPush(event->TimeDuration(), IsLabelOnStack(event->label())); Is it better to do: if ...
5 years, 4 months ago (2015-08-17 17:37:11 UTC) #8
Cutch
5 years, 4 months ago (2015-08-17 17:51:12 UTC) #9
Message was sent while issue was closed. 
https://codereview.chromium.org/1287073006/diff/20001/runtime/vm/timeline_ana...
File runtime/vm/timeline_analysis.cc (right):

https://codereview.chromium.org/1287073006/diff/20001/runtime/vm/timeline_ana...
runtime/vm/timeline_analysis.cc:374: pause_info->OnPush(event->TimeDuration(),
IsLabelOnStack(event->label()));
On 2015/08/17 17:37:11, srdjan wrote:
> Is it better to do:
> if (!isLabelOnStack(..)) {
>   pause_info->OnPush(event->TimeDuration())
> }
> 
> or is it better to pass an extra argument?

At one point we needed the extra argument, right now we don't but I'd like to
keep providing the bit of information to ::OnPush in case I need it again.

https://codereview.chromium.org/1287073006/diff/20001/runtime/vm/timeline_ana...
runtime/vm/timeline_analysis.cc:389: bool TimelinePauses::IsLabelOnStack(const
char* label) {
On 2015/08/17 17:37:11, srdjan wrote:
> const

Done (in a CL I'm working on now).

https://codereview.chromium.org/1287073006/diff/20001/runtime/vm/timeline_ana...
runtime/vm/timeline_analysis.cc:406: TimelinePauses::StackItem&
TimelinePauses::GetStackTop() {
On 2015/08/17 17:37:11, srdjan wrote:
> const

Cannot be done because StackItem& is modified by the caller.

Powered by Google App Engine
This is Rietveld 408576698