Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537143005:
Only create network_instrumentation TracedValue if tracing enabled (Closed)
DescriptionOnly create network_instrumentation TracedValue if tracing enabled
Currently the TracedValues were being created even when the tracing category was
disabled. This CL moves the creation of the TracedValues into a function, which
is called only when the TRACE macro arguments are evaluated, which, in turn,
only happens when the tracing category is enabled.
Measuring on a linux workstation using the linux perf tools, the instrumentation
overhead with the tracing category disabled is now below 2% of the cost of the
ResourceFetcher::requestResource function, as opposed to the 12% measured
before.
Committed: https://crrev.com/02efb805d9768ca2cead9ae25e34672b9d99974b
Cr-Commit-Position: refs/heads/master@{#437657}
Patch Set 1 #Patch Set 2 : typo #
Total comments: 1
Patch Set 3 : Use anonymous namespace #Messages
Total messages: 26 (12 generated)
Description was changed from ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. BUG=669666 ========== to ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Only a linux workstation, using the linux perf tools, the instrumentation overhead is now the category is disabled is now below 2% of the cost of ResourceFetcher::requestResource function, as opposed to the 12% measured before. BUG=669666 ==========
dproy@chromium.org changed reviewers: + caseq@chromium.org, chiniforooshan@chromium.org, csharrison@chromium.org
Description was changed from ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Only a linux workstation, using the linux perf tools, the instrumentation overhead is now the category is disabled is now below 2% of the cost of ResourceFetcher::requestResource function, as opposed to the 12% measured before. BUG=669666 ========== to ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. ==========
PTAL
Looks good, but I am still surprised this has meaningful overhead if tracing is turned off. I've never seen this for trace events in the past.
One part of the overhead is creating the scoped tracker, which is still happening even when the category is turned off. If we get rid of that, that will probably decrease 0.5-1 more percent of the cost. If we call the tracing macros directly (or turn the network instrumentation functions into macros) we *may* be able to save even more cost - whatever the overhead of calling a function is - but we will lose the nice strongly typed interface. It's also possible that we usually don't see the cost of tracing macros because if the category is disabled no function gets called, and nothing shows up on any stack or flamegraph. Some cost is still there; it's just not easily visible.
Ah right, creating the ScopedTracker explains most of it. LGTM from me but let's wait for tracing experts to weight in.
lgtm https://codereview.chromium.org/2537143005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2537143005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:39: std::unique_ptr<TracedValue> scopedResourceTrackerBeginData( All three could be in anonymous namespace -- unless you're going to make the call-sites inline.
dproy can you check to see if the regression made in to M56?
On 2016/12/05 15:00:07, Charlie Harrison wrote: > dproy can you check to see if the regression made in to M56? The regression has indeed made it to M56. I'll land this and create a merge request for this CL.
dproy@chromium.org changed reviewers: + jbroman@chromium.org
+jbroman@ for owner review.
lgtm
The CQ bit was checked by dproy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2537143005/#ps40001 (title: "Use anonymous namespace")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dproy@chromium.org
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": 40001, "attempt_start_ts": 1481310672010100,
"parent_rev": "c0263ec2327178b13fc82d52a883dce85007ae04", "commit_rev":
"aeb2dfc6af5da9fb59645de7a4c6b033714bb34d"}
Message was sent while issue was closed.
Description was changed from ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. ========== to ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. Review-Url: https://codereview.chromium.org/2537143005 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. Review-Url: https://codereview.chromium.org/2537143005 ========== to ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. BUG=669666 Review-Url: https://codereview.chromium.org/2537143005 ==========
Message was sent while issue was closed.
Description was changed from ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. BUG=669666 Review-Url: https://codereview.chromium.org/2537143005 ========== to ========== Only create network_instrumentation TracedValue if tracing enabled Currently the TracedValues were being created even when the tracing category was disabled. This CL moves the creation of the TracedValues into a function, which is called only when the TRACE macro arguments are evaluated, which, in turn, only happens when the tracing category is enabled. Measuring on a linux workstation using the linux perf tools, the instrumentation overhead with the tracing category disabled is now below 2% of the cost of the ResourceFetcher::requestResource function, as opposed to the 12% measured before. Committed: https://crrev.com/02efb805d9768ca2cead9ae25e34672b9d99974b Cr-Commit-Position: refs/heads/master@{#437657} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/02efb805d9768ca2cead9ae25e34672b9d99974b Cr-Commit-Position: refs/heads/master@{#437657} |
