Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450953003:
Tracing macros perftests. (Closed)
|
Created:
4 years, 1 month ago by kraynov Modified:
4 years, 1 month ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, picksi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPerformance tests for tracing macros.
Cases introduced:
- Long-running TRACE_EVENT0
- Using TracedValue
- Tracing from 4 concurrent threads
BUG=656029
Committed: https://crrev.com/fe26f22a88e179d2edfa2f3d9ed37c108c6db405
Cr-Commit-Position: refs/heads/master@{#432051}
Patch Set 1 #
Total comments: 3
Patch Set 2 : concurrent #Patch Set 3 : style #Patch Set 4 : simplify #Patch Set 5 : little fix #
Total comments: 12
Patch Set 6 : nit #
Total comments: 1
Patch Set 7 : nit #Messages
Total messages: 21 (12 generated)
kraynov@chromium.org changed reviewers: + primiano@chromium.org
WIP
LGTM % having a good commit message and title and addressing some comments below https://codereview.chromium.org/2450953003/diff/1/components/tracing/test/tra... File components/tracing/test/trace_event_perftest.cc (right): https://codereview.chromium.org/2450953003/diff/1/components/tracing/test/tra... components/tracing/test/trace_event_perftest.cc:42: auto value = base::WrapUnique(new TracedValue); I think MakeUnique is the preferred one for new instances? https://codereview.chromium.org/2450953003/diff/1/components/tracing/test/tra... components/tracing/test/trace_event_perftest.cc:85: TRACE_EVENT0("test_category", "Outter event"); typo, extra t in outter https://codereview.chromium.org/2450953003/diff/1/components/tracing/test/tra... components/tracing/test/trace_event_perftest.cc:104: IterableStopwatch trace_sw("instant_events_with_value"); i'd just drop instant_ here to keep the metric more compact.
Description was changed from ========== Tracing macros perftests. NOT FOR REVIEW YET BUG=656029 ========== to ========== Performance tests for tracing macros. Cases introduced: - Long-running TRACE_EVENT0 - Using TracedValue - Tracing from 4 concurrent threads BUG=656029 ==========
Please review. More cases to be submitted in a separate CL.
Looks good, minor comments. thanks https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... File components/tracing/test/trace_event_perftest.cc (right): https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:6: #include "base/memory/ptr_util.h" what is this include for? https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:9: #include "base/task_runner_util.h" I don't think you need this anymore https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:74: static void SubmitTraceEventsAndSignal(WaitableEvent* complete) { variable/argument names should be nouns. Either s/complete/complete_event/ or just s/complete/completion/ https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:110: TEST_F(TraceEventPerfTest, UsingTracedValue) { would be nice if the test name here also had the 10000 in the name, so you can know that the time of create_trace_values is for creating 10k of them. Maybe Submit_10000_TRACE_EVENT_withTracedValue https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:114: ScopedStopwatch value_sw("create_traced_values"); I think this loop should be in a different TEST_F, Create_10000_TracedValue https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:133: const int num_threads = 4; s/num_threads/kNumThreads/ according to our C++ code style constant names should be kCamelCase
https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... File components/tracing/test/trace_event_perftest.cc (right): https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:6: #include "base/memory/ptr_util.h" On 2016/11/14 22:28:01, Primiano - slow(travelling) wrote: > what is this include for? WrapUnique, MakeUnique https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:9: #include "base/task_runner_util.h" On 2016/11/14 22:28:01, Primiano - slow(travelling) wrote: > I don't think you need this anymore Done. https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:74: static void SubmitTraceEventsAndSignal(WaitableEvent* complete) { On 2016/11/14 22:28:00, Primiano - slow(travelling) wrote: > variable/argument names should be nouns. > Either s/complete/complete_event/ or just s/complete/completion/ Done. https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:110: TEST_F(TraceEventPerfTest, UsingTracedValue) { On 2016/11/14 22:28:01, Primiano - slow(travelling) wrote: > would be nice if the test name here also had the 10000 in the name, so you can > know that the time of create_trace_values is for creating 10k of them. > Maybe Submit_10000_TRACE_EVENT_withTracedValue Done. https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:114: ScopedStopwatch value_sw("create_traced_values"); On 2016/11/14 22:28:00, Primiano - slow(travelling) wrote: > I think this loop should be in a different TEST_F, Create_10000_TracedValue Done. https://codereview.chromium.org/2450953003/diff/80001/components/tracing/test... components/tracing/test/trace_event_perftest.cc:133: const int num_threads = 4; On 2016/11/14 22:28:00, Primiano - slow(travelling) wrote: > s/num_threads/kNumThreads/ > according to our C++ code style constant names should be kCamelCase Done.
The CQ bit was checked by kraynov@chromium.org 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...
LGTM with 1 final comment. thanks https://codereview.chromium.org/2450953003/diff/100001/components/tracing/tes... File components/tracing/test/trace_event_perftest.cc (right): https://codereview.chromium.org/2450953003/diff/100001/components/tracing/tes... components/tracing/test/trace_event_perftest.cc:133: TEST_F(TraceEventPerfTest, Concurrent_10000_TRACE_EVENT0) { Oh just one thing here: can you cal lthis Submit_10000_TRACE_EVENT_multithread for consistency with the pattern above?
The CQ bit was checked by kraynov@chromium.org 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 kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2450953003/#ps120001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Performance tests for tracing macros. Cases introduced: - Long-running TRACE_EVENT0 - Using TracedValue - Tracing from 4 concurrent threads BUG=656029 ========== to ========== Performance tests for tracing macros. Cases introduced: - Long-running TRACE_EVENT0 - Using TracedValue - Tracing from 4 concurrent threads BUG=656029 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Performance tests for tracing macros. Cases introduced: - Long-running TRACE_EVENT0 - Using TracedValue - Tracing from 4 concurrent threads BUG=656029 ========== to ========== Performance tests for tracing macros. Cases introduced: - Long-running TRACE_EVENT0 - Using TracedValue - Tracing from 4 concurrent threads BUG=656029 Committed: https://crrev.com/fe26f22a88e179d2edfa2f3d9ed37c108c6db405 Cr-Commit-Position: refs/heads/master@{#432051} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fe26f22a88e179d2edfa2f3d9ed37c108c6db405 Cr-Commit-Position: refs/heads/master@{#432051} |
