New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 760702 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocking:
issue 778067



Sign in to add a comment

Remove source code location information from Location in official builds.

Project Member Reported by brettw@chromium.org, Aug 30 2017

Issue description

The FROM_HERE macro generates Location objects which refer to the source file name and function name. This takes up space and is only used by developers. We can use the program counter instead.

Design doc:
https://docs.google.com/document/d/1cibE5nAG9sdjVMfs-MkvrMtp43i9MLGqM_uNusLqiBY
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135

commit 5b5b7cbebc0dece3d9d0691eee96a2b9c8067135
Author: Brett Wilson <brettw@chromium.org>
Date: Fri Sep 01 04:40:02 2017

Add a flag to remove source code info from Location.

Source code information is still saved by default with this patch, developers
will have to set the flag to false manually to remove this information.

The current implementation removes only the function and line numbers from
the FROM_HERE macro when the flag is turned off. The file name will be
removed in a later pass when the heap profiler is updated.

The hash and comparison functions have been updated to compare only the PC
and not the file and line number since the source information will not
necessarily be available. This should be unique and is also faster to
compare. The tracked object unittests that relied on this were updated to
use non-null addresses.

Minor updates to Location to convert the explicit hash functor to a C++11
compatible hash function.

Design doc: https://docs.google.com/document/d/1cibE5nAG9sdjVMfs-MkvrMtp43i9MLGqM_uNusLqiBY

Bug: 760702
Change-Id: I835f13fd194bfe68951108a1f4a8fffd8165b04b
Reviewed-on: https://chromium-review.googlesource.com/641913
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499125}
[modify] https://crrev.com/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135/base/BUILD.gn
[modify] https://crrev.com/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135/base/location.cc
[modify] https://crrev.com/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135/base/location.h
[modify] https://crrev.com/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135/base/trace_event/trace_event.h
[modify] https://crrev.com/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135/base/tracked_objects.h
[modify] https://crrev.com/5b5b7cbebc0dece3d9d0691eee96a2b9c8067135/base/tracked_objects_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a2884415d3a0119bd0949d4adc325a493474046c

commit a2884415d3a0119bd0949d4adc325a493474046c
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Fri Sep 01 04:55:15 2017

Revert "Add a flag to remove source code info from Location."

This reverts commit 5b5b7cbebc0dece3d9d0691eee96a2b9c8067135.

Reason for revert: Broke Linux builder dbg 32: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_Builder__dbg__32_%2F71975%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

Original change's description:
> Add a flag to remove source code info from Location.
> 
> Source code information is still saved by default with this patch, developers
> will have to set the flag to false manually to remove this information.
> 
> The current implementation removes only the function and line numbers from
> the FROM_HERE macro when the flag is turned off. The file name will be
> removed in a later pass when the heap profiler is updated.
> 
> The hash and comparison functions have been updated to compare only the PC
> and not the file and line number since the source information will not
> necessarily be available. This should be unique and is also faster to
> compare. The tracked object unittests that relied on this were updated to
> use non-null addresses.
> 
> Minor updates to Location to convert the explicit hash functor to a C++11
> compatible hash function.
> 
> Design doc: https://docs.google.com/document/d/1cibE5nAG9sdjVMfs-MkvrMtp43i9MLGqM_uNusLqiBY
> 
> Bug: 760702
> Change-Id: I835f13fd194bfe68951108a1f4a8fffd8165b04b
> Reviewed-on: https://chromium-review.googlesource.com/641913
> Commit-Queue: Nico Weber <thakis@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#499125}

TBR=thakis@chromium.org,brettw@chromium.org

Change-Id: I697d0c249aa8fa1d498cc053bb02bae921fc5201
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 760702
Reviewed-on: https://chromium-review.googlesource.com/646434
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499127}
[modify] https://crrev.com/a2884415d3a0119bd0949d4adc325a493474046c/base/BUILD.gn
[modify] https://crrev.com/a2884415d3a0119bd0949d4adc325a493474046c/base/location.cc
[modify] https://crrev.com/a2884415d3a0119bd0949d4adc325a493474046c/base/location.h
[modify] https://crrev.com/a2884415d3a0119bd0949d4adc325a493474046c/base/trace_event/trace_event.h
[modify] https://crrev.com/a2884415d3a0119bd0949d4adc325a493474046c/base/tracked_objects.h
[modify] https://crrev.com/a2884415d3a0119bd0949d4adc325a493474046c/base/tracked_objects_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c794cf6057400281fad24cdb86a990ef224cfa25

commit c794cf6057400281fad24cdb86a990ef224cfa25
Author: Brett Wilson <brettw@chromium.org>
Date: Fri Sep 01 20:14:33 2017

Add a flag to remove source code info from Location.

Source code information is still saved by default with this patch, developers
will have to set the flag to false manually to remove this information.

The current implementation removes only the function and line numbers from
the FROM_HERE macro when the flag is turned off. The file name will be
removed in a later pass when the heap profiler is updated.

The hash and comparison functions have been updated to compare only the PC
and not the file and line number since the source information will not
necessarily be available. This should be unique and is also faster to
compare. The tracked object unittests that relied on this were updated to
use non-null addresses.

Minor updates to Location to convert the explicit hash functor to a C++11
compatible hash function.

Design doc: https://docs.google.com/document/d/1cibE5nAG9sdjVMfs-MkvrMtp43i9MLGqM_uNusLqiBY

Some missing PPAPI deps on base were added that caused location.h to be
included without generating the required buildflag header.

TBR=thakis (reland with trivial fixes)
Reland of https://chromium-review.googlesource.com/c/chromium/src/+/641913

Bug: 760702
Change-Id: I54de9af59cbd12283073d419516c8420a6d3797c
Reviewed-on: https://chromium-review.googlesource.com/647333
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499273}
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/base/BUILD.gn
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/base/location.cc
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/base/location.h
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/base/trace_event/trace_event.h
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/base/tracked_objects.h
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/base/tracked_objects_unittest.cc
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/ppapi/shared_impl/BUILD.gn
[modify] https://crrev.com/c794cf6057400281fad24cdb86a990ef224cfa25/ppapi/thunk/BUILD.gn

Comment 4 by ssid@chromium.org, Sep 6 2017

Cc: ssid@chromium.org
Cc: deadbeef@chromium.org
+deadbeef, who added location strings to webrtc via RTC_FROM_HERE. I'm wondering if we can / should similarly remove the strings from this macro?
Labels: Performance-Size
Blocking: 778067
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0e776e8b898772ca22837b3d6b109e680f2651a

commit a0e776e8b898772ca22837b3d6b109e680f2651a
Author: Siddhartha <ssid@chromium.org>
Date: Mon Oct 30 20:51:41 2017

Introduce mixed stack mode in AllocationContextTracker

AllocationContextTracker tracks PCs just like pseudo stack when native
stack is not available. Also remove NO_STACK mode.
Remove behavior of adding type_name from trace category.

BUG=760702

Change-Id: I884ae9f8c853ecdea5d531f8ebe6c871901b8255
Reviewed-on: https://chromium-review.googlesource.com/729548
Commit-Queue: Siddhartha S <ssid@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512619}
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/heap_profiler.h
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/heap_profiler_allocation_context_tracker.cc
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/heap_profiler_allocation_context_tracker.h
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/heap_profiler_event_filter.cc
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/memory_dump_manager_unittest.cc
[modify] https://crrev.com/a0e776e8b898772ca22837b3d6b109e680f2651a/base/trace_event/trace_event.h

Comment 9 by deadbeef@webrtc.org, Nov 28 2017

agrieve, can you summarize the reasoning behind this? I think the RTC_FROM_HERE tracking has been useful; I wouldn't want to remove it unless we have a good reason and an alternative.

I think one benefit over the program counter is that you don't need to have symbol files to get information out of it. For example, we log "slow" dispatches (https://cs.chromium.org/chromium/src/third_party/webrtc/rtc_base/messagequeue.cc?sq=package:chromium&l=537) which has helped debug some issues given the log file alone.
Design doc is linked in bug summary.
Sorry I didn't look that over first.

As long as the location information is kept in unofficial builds, that sounds acceptable to me. It would cover the main use cases I have in mind.
Owner: ----
Status: Available (was: Started)

Comment 13 by ssid@chromium.org, Mar 20 2018

Owner: ssid@chromium.org
Status: Assigned (was: Available)

Sign in to add a comment