Issue metadata
Sign in to add a comment
|
Memory allocator dumps problems in SharedMemoryTracker::OnMemoryDump |
||||||||||||||||||||||||
Issue descriptionNow memory allocator dumps and edges are created at SharedMemoryTracker::OnMemoryDump but there are some problems. ProcessMemoryDump::CreateSharedMemoryOwnershipEdge might create the same dump but not setting its size. Let's assume that PMD::CreateSharedMemoryOwnershipEdge is called ahead of SMT::OnMemoryDump. SMT::OnMemoryDump needs to set the size to the dump, but in the current implementation, the size is not set because it does nothing after checking the dump existence. In this case, an edge between the local dump and the global dump should be created if and only if it is necessary. It's because PMD doesn't create not all necessary edges, and we cannot create duplicated edges (edges created by PMD are not overrideable). In single process mode, the problem is more complicated: the dump we are trying to create might already exist because the other renderer created that, OR because PMD created that. We should skip creating dumps or edges in the former case, and should add sizes to the dump in the latter case. I've created https://chromium-review.googlesource.com/c/544638/1 but the problem in this CL is that calling AddScalar twice might cause undetermined behavior. As primiano@ and I discussed offline, we came up with some ideas to solve this: 1) Adding is_empty() { return attributes_.empty() } to MemoryAllocatorDump and check this when trying to create an edge. 2) Exposing GUID generating logic and using this to get GUID instead of creating a dump I prefer 2) because we could avoid such side effect. ssid@, what do you think?
,
Jun 22 2017
I'd also prefer 2). ssid any thoughts/objections?
,
Jun 22 2017
(2) s.g.
,
Jun 23 2017
Created https://chromium-review.googlesource.com/c/544482/ based on 2).
,
Jun 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de0c27b10c909960c3797ffc42159fb81d8c4bbd commit de0c27b10c909960c3797ffc42159fb81d8c4bbd Author: Hajime Hoshi <hajimehoshi@chromium.org> Date: Tue Jun 27 08:11:24 2017 Bug fix: Avoid creating dumps when getting MemoryAllocatorDump guids in ProcessMemoryDump SharedMemoryTracker::OnMemoryDump creates dumps with their sizes and edges between dumps for base::SharedMemory. The problem is that same dumps might be created at ProcessMemoryDump:: CreateSharedMemoryOwnershipEdge just to retrieve GUID, and SMT:: OnMemoryDump needs to consider the state whether dumps are already created or not. There are bugs in SMT::OnMemoryDump. This needs to set sizes to dumps and edges if and only if necessary, but these are not implemented yet. As long as PMD creates dumps, the fix would be complicated. To make matters worse, in single process mode, dumps can be duplicated and the fix would need to consider this condition. This CL fixes these bugs by avoiding create dumps in ProcessMemoryDump:: CreateSharedMemoryOwnershipEdge and exposing generating GUID logic instead. After that, PMD no longer creates dumps and SMT can assume that if the dump already exists, this is called in single process mode. Bug: 604726 , 735892 Change-Id: I34e3e2d87e238258caf692db62f2a05dd5f2b545 Reviewed-on: https://chromium-review.googlesource.com/544482 Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: siddhartha sivakumar <ssid@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#482572} [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/memory/shared_memory_tracker.cc [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/memory/shared_memory_tracker.h [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/trace_event/memory_allocator_dump.cc [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/trace_event/memory_allocator_dump.h [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/trace_event/memory_allocator_dump_unittest.cc [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/trace_event/process_memory_dump.cc [modify] https://crrev.com/de0c27b10c909960c3797ffc42159fb81d8c4bbd/base/trace_event/process_memory_dump_unittest.cc
,
Jun 30 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by hajimehoshi@chromium.org
, Jun 22 2017