Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716023002:
Add some changes to TraceAnalyzer to improve analysis (Closed)
DescriptionAdd some changes to TraceAnalyzer to improve analysis
* Tests which rely on being able to query for the end event in a pair
were not possible, partly because end events tend to be missing
information, and also because they did not reference their start
events
* Async event association can optionally be allowed for events which
span across process boundaries
BUG=None
Review-Url: https://codereview.chromium.org/2716023002
Cr-Commit-Position: refs/heads/master@{#455132}
Committed: https://chromium.googlesource.com/chromium/src/+/83621ef58f633413145bd7093f16d56eb2c88103
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add some changes to TraceAnalyzer to improve analysis #
Messages
Total messages: 42 (25 generated)
Description was changed from ========== Add some changes to TraceAnalyzer to improve analysis * Tests which rely on being able to query for the end event in a pair were not possible, partly because end events tend to be missing information, and also because they did not reference their start events * Async event association can optionally be allowed for events which span across process boundaries BUG=None ========== to ========== Add some changes to TraceAnalyzer to improve analysis * Tests which rely on being able to query for the end event in a pair were not possible, partly because end events tend to be missing information, and also because they did not reference their start events * Async event association can optionally be allowed for events which span across process boundaries BUG=None ==========
skiazyk@google.com changed reviewers: + phajdan.jr@chromium.org
skiazyk@google.com changed required reviewers: + phajdan.jr@chromium.org
phajdan.jr@chromium.org changed reviewers: + primiano@chromium.org
Please get a review from someone more familiar with tracing first. Maybe Primiano (added)?
primiano@chromium.org changed reviewers: + fmeawad@chromium.org
On 2017/02/27 16:06:02, Paweł Hajdan Jr. wrote: > Please get a review from someone more familiar with tracing first. Maybe > Primiano (added)? sorry super swamped. can review this but more towards the end of the week. Also, the fact that this CL does not have a BUG= nor a description on what problem is trying to solve, doesn't help me to prioritize this. maybe +fmeawad@ can help finding another reviewer if this is really urgent?
On 2017/02/27 16:51:29, Primiano Tucci wrote: > On 2017/02/27 16:06:02, Paweł Hajdan Jr. wrote: > > Please get a review from someone more familiar with tracing first. Maybe > > Primiano (added)? > > sorry super swamped. can review this but more towards the end of the week. > Also, the fact that this CL does not have a BUG= nor a description on what > problem is trying to solve, doesn't help me to prioritize this. > > maybe +fmeawad@ can help finding another reviewer if this is really urgent? This is a modification I did on development branch of Android tree that I use in some performance testing scripts. We are currently trying to get the whole project upstreamed to Android master, however I was told that they generally only cherry-pick from chromium, so this is one of the repositories left that we are still on our own branch. If it helps, its already been reviewed by a number of former chrome people?
On 2017/02/27 17:25:44, skiazyk wrote: > If it helps, its already been reviewed by a number > of former chrome people? It may help if you point exactly to such reviews then, and that they'd be the right people to review your CL. Maybe you could also add them to this review, which should be easy for them to approve.
skiazyk@google.com changed reviewers: + avakulenko@google.com, hendrikw@google.com, jbates@google.com
skiazyk@google.com changed reviewers: + jbates@chromium.org - avakulenko@google.com, hendrikw@google.com, jbates@google.com
jbates@google.com changed reviewers: + jbates@google.com
LGTM. I reviewed this for another project.
The CQ bit was checked by jbates@google.com
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
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
skiazyk@google.com changed required reviewers: + jbates@google.com - phajdan.jr@chromium.org
The CQ bit was checked by skiazyk@google.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
skiazyk@google.com changed required reviewers: + phajdan.jr@chromium.org
hendrikw@chromium.org changed reviewers: + hendrikw@chromium.org
https://codereview.chromium.org/2716023002/diff/1/base/test/trace_event_analy... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/2716023002/diff/1/base/test/trace_event_analy... base/test/trace_event_analyzer.cc:532: *num = event.prev_event ? 1.0 : 0.0; Are you missing a return true?
https://codereview.chromium.org/2716023002/diff/1/base/test/trace_event_analy... File base/test/trace_event_analyzer.cc (right): https://codereview.chromium.org/2716023002/diff/1/base/test/trace_event_analy... base/test/trace_event_analyzer.cc:532: *num = event.prev_event ? 1.0 : 0.0; On 2017/03/06 17:33:38, hendrikw wrote: > Are you missing a return true? Indeed I am, resolved in subsequent patch set.
On 2017/03/06 17:46:54, skiazyk wrote: > https://codereview.chromium.org/2716023002/diff/1/base/test/trace_event_analy... > File base/test/trace_event_analyzer.cc (right): > > https://codereview.chromium.org/2716023002/diff/1/base/test/trace_event_analy... > base/test/trace_event_analyzer.cc:532: *num = event.prev_event ? 1.0 : 0.0; > On 2017/03/06 17:33:38, hendrikw wrote: > > Are you missing a return true? > > Indeed I am, resolved in subsequent patch set. LGTM
The CQ bit was checked by skiazyk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbates@google.com Link to the patchset: https://codereview.chromium.org/2716023002/#ps20001 (title: "Add some changes to TraceAnalyzer to improve analysis")
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
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
LGTM
The CQ bit was checked by skiazyk@google.com 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 CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skiazyk@google.com
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": 20001, "attempt_start_ts": 1488908948682380,
"parent_rev": "049ff5112cc49fdf06e27821a513c7ec4ca5403f", "commit_rev":
"83621ef58f633413145bd7093f16d56eb2c88103"}
Message was sent while issue was closed.
Description was changed from ========== Add some changes to TraceAnalyzer to improve analysis * Tests which rely on being able to query for the end event in a pair were not possible, partly because end events tend to be missing information, and also because they did not reference their start events * Async event association can optionally be allowed for events which span across process boundaries BUG=None ========== to ========== Add some changes to TraceAnalyzer to improve analysis * Tests which rely on being able to query for the end event in a pair were not possible, partly because end events tend to be missing information, and also because they did not reference their start events * Async event association can optionally be allowed for events which span across process boundaries BUG=None Review-Url: https://codereview.chromium.org/2716023002 Cr-Commit-Position: refs/heads/master@{#455132} Committed: https://chromium.googlesource.com/chromium/src/+/83621ef58f633413145bd7093f16... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/83621ef58f633413145bd7093f16... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
