Remove source code location information from Location in official builds. |
||||||||
Issue descriptionThe 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
,
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
,
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
,
Sep 6 2017
,
Oct 30 2017
+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?
,
Oct 30 2017
,
Oct 30 2017
,
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
,
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.
,
Nov 28 2017
Design doc is linked in bug summary.
,
Nov 29 2017
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.
,
Mar 20 2018
,
Mar 20 2018
,
Aug 1
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Sep 1 2017