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

Issue 766396 link

Starred by 1 user
Status: Fixed
Owner:
█ brettw@chromium.org
Last visit 25 days ago
Closed: Oct 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 758739



Sign in to add a comment

Flush memlog pipes before taking a heap dump.

Project Member Reported by erikc...@chromium.org, Sep 19 2017 
We'll need to do the following:

1) Modify memlog_client.mojom to add a Flush() function.
2) Implement Flush() by iterating through all MemlogSenderPipe shards and flushing them.
3) Call the Flush() function from ProfilingProcessHost, wait for the callback, and then send the Dump command to the profiling process.

Comment 1 by erikc...@chromium.org, Sep 19 2017 

This also allows us to:
1) Get rid of the 68,000 flush in the memlog_browsertest.
2) Allow us to turn on some basic monitoring for renderer profiling.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 6 2017 

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

commit 1ca0e5fc031b33fc99f8e78bd367986398b06fb3
Author: erikchen <erikchen@chromium.org>
Date: Fri Oct 06 22:06:14 2017

Synchronize on the memlog pipe when doing a dump.

Fixes data races on dumping as the AllocationTracker was being accessed
from another thread than it is modifying the data on. The dumping function
locks all the parsers which will prevent most data races since no new
parsed data will be sent, but it's not guaranteed that the allocation
tracker is not processing previously parsed data at the time the
snapshot is taken.

Removes the "hop to thread" flag on dumping, since it will now synchronize
all the way to the instrumented process.

Bug:  766396 
Change-Id: I572f558a585a4316c612c6ac78665f52facfc10c
Reviewed-on: https://chromium-review.googlesource.com/675809
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507188}
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/browser/profiling_host/profiling_process_host.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog.mojom
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog_allocator_shim.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog_allocator_shim.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog_client.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog_client.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog_client.mojom
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/common/profiling/memlog_stream.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/README.md
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/allocation_tracker.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/allocation_tracker.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/backtrace_storage.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/backtrace_storage.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/json_exporter.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/json_exporter.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/json_exporter_unittest.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_connection_manager.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_connection_manager.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_impl.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_impl.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver_pipe.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver_pipe.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver_pipe_posix.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver_pipe_posix.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver_pipe_win.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_receiver_pipe_win.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_stream_parser.cc
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_stream_parser.h
[modify] https://crrev.com/1ca0e5fc031b33fc99f8e78bd367986398b06fb3/chrome/profiling/memlog_stream_parser_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 6 2017 

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

commit 22f66c6dca7a95233979a78bd65642d4b498f905
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Oct 06 23:48:50 2017

Revert "Synchronize on the memlog pipe when doing a dump."

This reverts commit 1ca0e5fc031b33fc99f8e78bd367986398b06fb3.

Reason for revert: causes crash when attempting to take a memory-infra dump with --memlog=all.

Original change's description:
> Synchronize on the memlog pipe when doing a dump.
> 
> Fixes data races on dumping as the AllocationTracker was being accessed
> from another thread than it is modifying the data on. The dumping function
> locks all the parsers which will prevent most data races since no new
> parsed data will be sent, but it's not guaranteed that the allocation
> tracker is not processing previously parsed data at the time the
> snapshot is taken.
> 
> Removes the "hop to thread" flag on dumping, since it will now synchronize
> all the way to the instrumented process.
> 
> Bug:  766396 
> Change-Id: I572f558a585a4316c612c6ac78665f52facfc10c
> Reviewed-on: https://chromium-review.googlesource.com/675809
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#507188}

TBR=dcheng@chromium.org,brettw@chromium.org,erikchen@chromium.org

Change-Id: I13f13dcbf7097b9e9bf48f685fd74c5d361b2ade
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  766396 
Reviewed-on: https://chromium-review.googlesource.com/706237
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507231}
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/browser/profiling_host/profiling_process_host.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog.mojom
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog_allocator_shim.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog_allocator_shim.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog_client.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog_client.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog_client.mojom
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/common/profiling/memlog_stream.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/README.md
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/allocation_tracker.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/allocation_tracker.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/backtrace_storage.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/backtrace_storage.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/json_exporter.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/json_exporter.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/json_exporter_unittest.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_connection_manager.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_connection_manager.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_impl.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_impl.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver_pipe.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver_pipe.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver_pipe_posix.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver_pipe_posix.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver_pipe_win.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_receiver_pipe_win.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_stream_parser.cc
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_stream_parser.h
[modify] https://crrev.com/22f66c6dca7a95233979a78bd65642d4b498f905/chrome/profiling/memlog_stream_parser_unittest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 7 2017 

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

commit e2d0644720e2432156e4ec88da4859b8384beb6e
Author: Erik Chen <erikchen@chromium.org>
Date: Sat Oct 07 03:34:07 2017

[Reland #1] "Synchronize on the memlog pipe when doing a dump."

Original author: brettw@

While brettw@ was out, I fixed some [unrelated] race conditions and added the
fixes to Brett's patch. During code review, I accidentally added an incorrect
std::move. The tests that would have caught this were themselves reverted for
unrelated reasons. See
https://bugs.chromium.org/p/chromium/issues/detail?id=753218#c15.

This is a reland of 1ca0e5fc031b33fc99f8e78bd367986398b06fb3
Original change's description:
> Synchronize on the memlog pipe when doing a dump.
>
> Fixes data races on dumping as the AllocationTracker was being accessed
> from another thread than it is modifying the data on. The dumping function
> locks all the parsers which will prevent most data races since no new
> parsed data will be sent, but it's not guaranteed that the allocation
> tracker is not processing previously parsed data at the time the
> snapshot is taken.
>
> Removes the "hop to thread" flag on dumping, since it will now synchronize
> all the way to the instrumented process.
>
> Bug:  766396 
> Change-Id: I572f558a585a4316c612c6ac78665f52facfc10c
> Reviewed-on: https://chromium-review.googlesource.com/675809
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#507188}

Bug:  766396 
Change-Id: I822bd57b5ba247d4bed9a784b99d7d04cca5370e
TBR: dcheng@chromium.org, brettw@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/706541
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507273}
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/browser/profiling_host/memlog_browsertest.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/browser/profiling_host/profiling_process_host.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/browser/profiling_host/profiling_process_host.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog.mojom
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog_allocator_shim.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog_allocator_shim.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog_client.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog_client.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog_client.mojom
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/common/profiling/memlog_stream.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/README.md
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/allocation_tracker.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/allocation_tracker.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/backtrace_storage.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/backtrace_storage.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/json_exporter.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/json_exporter.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/json_exporter_unittest.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_connection_manager.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_connection_manager.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_impl.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_impl.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver_pipe.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver_pipe.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver_pipe_posix.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver_pipe_posix.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver_pipe_win.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_receiver_pipe_win.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_stream_parser.cc
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_stream_parser.h
[modify] https://crrev.com/e2d0644720e2432156e4ec88da4859b8384beb6e/chrome/profiling/memlog_stream_parser_unittest.cc

Comment 5 by brettw@chromium.org, Oct 9 2017

Owner: brettw@chromium.org
Status: Fixed (was: Untriaged)
►

Sign in to add a comment