Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2975033002:
[tracing] Optimize TracedValue::AppendAsTraceFormat(). (Closed)
|
Created:
3 years, 5 months ago by DmitrySkiba Modified:
3 years, 5 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, danakj+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Optimize TracedValue::AppendAsTraceFormat().
This CL optimizes TracedValue::AppendAsTraceFormat() by removing
intermediate base::Value conversion.
HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement
on macOS (1730ms -> 920ms).
TBR=jbauman@chromium.org
BUG=739378, 664350
Review-Url: https://codereview.chromium.org/2975033002
Cr-Commit-Position: refs/heads/master@{#487019}
Committed: https://chromium.googlesource.com/chromium/src/+/131e21f45f5d7b04874b636c1ffa642c13944f1e
Patch Set 1 #Patch Set 2 : Add include #Patch Set 3 : Remove include #
Total comments: 4
Patch Set 4 : Fix FilterOperationsTest #Patch Set 5 : Rebase #Patch Set 6 : Fix GraphicsMemoryDumpProviderTest #
Messages
Total messages: 30 (22 generated)
Description was changed from ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. BUG=739378 ========== to ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). BUG=739378 ==========
dskiba@chromium.org changed reviewers: + primiano@chromium.org
Description was changed from ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). BUG=739378 ========== to ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). BUG=739378, 664350 ==========
The CQ bit was checked by primiano@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...
Cool. LGTM if the tests are passing. Also I'd consider taking another look at using stringstream instead of std::string, it might speed up even further https://codereview.chromium.org/2975033002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event_argument.cc (right): https://codereview.chromium.org/2975033002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_argument.cc:460: struct State { isn't this a gcc extension? https://codereview.chromium.org/2975033002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_argument.cc:476: out->append("{"); I think all ths might be even faster if you build the json using a stringstream and only at the end assign the |out| string argument. these std::string operations will very likely hit on the heap all the time
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dskiba@chromium.org changed reviewers: + danakj@chromium.org
danakj@ PTAL to cc/base/filter_operations_unittest.cc - AppendAsTraceFormat() now appends elements in different order, so I had to reorder items in test JSON. https://codereview.chromium.org/2975033002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event_argument.cc (right): https://codereview.chromium.org/2975033002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_argument.cc:460: struct State { On 2017/07/12 15:26:23, Primiano Tucci wrote: > isn't this a gcc extension? Inner structs? I believe it's valid C++11. https://codereview.chromium.org/2975033002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_argument.cc:476: out->append("{"); On 2017/07/12 15:26:23, Primiano Tucci wrote: > I think all ths might be even faster if you build the json using a stringstream > and only at the end assign the |out| string argument. > these std::string operations will very likely hit on the heap all the time I don't think there will be a difference - stringstream just appends to a std::string inside. But after the dust settles, I do want to look closely at std::string used as to accumulate JSONs.
The CQ bit was checked by dskiba@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On Wed, Jul 12, 2017 at 7:48 PM, <dskiba@chromium.org> wrote: > danakj@ PTAL to cc/base/filter_operations_unittest.cc - > AppendAsTraceFormat() > now appends elements in different order, so I had to reorder items in test > JSON. > I'm not working on chromium this week, can you find another cc owner? Sorry. > > > > https://codereview.chromium.org/2975033002/diff/40001/ > base/trace_event/trace_event_argument.cc > File base/trace_event/trace_event_argument.cc (right): > > https://codereview.chromium.org/2975033002/diff/40001/ > base/trace_event/trace_event_argument.cc#newcode460 > base/trace_event/trace_event_argument.cc:460: struct State { > On 2017/07/12 15:26:23, Primiano Tucci wrote: > > isn't this a gcc extension? > > Inner structs? I believe it's valid C++11. > > https://codereview.chromium.org/2975033002/diff/40001/ > base/trace_event/trace_event_argument.cc#newcode476 > base/trace_event/trace_event_argument.cc:476: out->append("{"); > On 2017/07/12 15:26:23, Primiano Tucci wrote: > > I think all ths might be even faster if you build the json using a > stringstream > > and only at the end assign the |out| string argument. > > these std::string operations will very likely hit on the heap all the > time > > I don't think there will be a difference - stringstream just appends to > a std::string inside. But after the dust settles, I do want to look > closely at std::string used as to accumulate JSONs. > > https://codereview.chromium.org/2975033002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dskiba@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.
dskiba@chromium.org changed reviewers: + ericrk@chromium.org, jbauman@chromium.org
jbauman@, ericrk@: please rubber-stamp changes in corresponding unittests - AppendAsTraceFormat() now appends elements in different order, so I had to reorder items in test JSONs.
lgtm
Description was changed from ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). BUG=739378, 664350 ========== to ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). TBR=jbauman@chromium.org BUG=739378, 664350 ==========
The CQ bit was checked by dskiba@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/2975033002/#ps100001 (title: "Fix GraphicsMemoryDumpProviderTest")
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": 100001, "attempt_start_ts": 1500229685013310,
"parent_rev": "6da431cd105766b1eada18e3bdeef3a551e9c16f", "commit_rev":
"131e21f45f5d7b04874b636c1ffa642c13944f1e"}
Message was sent while issue was closed.
Description was changed from ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). TBR=jbauman@chromium.org BUG=739378, 664350 ========== to ========== [tracing] Optimize TracedValue::AppendAsTraceFormat(). This CL optimizes TracedValue::AppendAsTraceFormat() by removing intermediate base::Value conversion. HeapProfilerPerfTest.AppendStackFramesAsTraceFormat shows ~1.9x improvement on macOS (1730ms -> 920ms). TBR=jbauman@chromium.org BUG=739378, 664350 Review-Url: https://codereview.chromium.org/2975033002 Cr-Commit-Position: refs/heads/master@{#487019} Committed: https://chromium.googlesource.com/chromium/src/+/131e21f45f5d7b04874b636c1ffa... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/131e21f45f5d7b04874b636c1ffa... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
