Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912723002:
Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" (Closed)
|
Created:
3 years, 6 months ago by hajimehoshi Modified:
3 years, 6 months ago CC:
chromium-reviews, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRevert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong"
This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e.
The previous change removes 'shared' global dumps when the backend is
not base::SharedMemory, but actually the 'shared' global dumps are
needed regardless of the backend. 'Shared' in this context means not only
base::SharedMemory but also wider things.
BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2912723002
Cr-Commit-Position: refs/heads/master@{#477197}
Committed: https://chromium.googlesource.com/chromium/src/+/3e863242981adb61aa416fda0607470df3ca0eb6
Patch Set 1 #Patch Set 2 : (rebase) #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context doesn't mean base::SharedMemory. For example, the buffer can be allocated in GPU and might be shared between processes or components. BUG=604726 ========== to ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context doesn't mean base::SharedMemory. For example, the buffer can be allocated in GPU and might be shared between processes or components. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hajimehoshi@chromium.org changed reviewers: + ericrk@chromium.org, jbauman@chromium.org, ssid@chromium.org
PTAL
Description was changed from ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context doesn't mean base::SharedMemory. For example, the buffer can be allocated in GPU and might be shared between processes or components. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context means not only base::SharedMemory but also wider things like GPU. For example, the GPU buffer can be allocated in GPU and might be shared between processes or components. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/29 09:05:10, hajimehoshi wrote: > PTAL lgtm thanks!
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ping ericrk@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
On 2017/06/05 23:28:34, hajimehoshi wrote: > ping Sorry, I'm not sure if I understand why we're trying to do this. Why would we want to count a MemoryBufferBacking as shared memory, given that it's allocated with "new" and can't be shared cross-process?
On 2017/06/05 23:36:25, jbauman wrote: > On 2017/06/05 23:28:34, hajimehoshi wrote: > > ping > > Sorry, I'm not sure if I understand why we're trying to do this. Why would we > want to count a MemoryBufferBacking as shared memory, given that it's allocated > with "new" and can't be shared cross-process? The naming is confusing, but a global 'shared' dump created by CreateSharedGlobalAllocatorDump is used not only for base::SharedMemory shared by multiple processes but also GPU memory.
On 2017/06/06 00:21:57, hajimehoshi wrote: > On 2017/06/05 23:36:25, jbauman wrote: > > On 2017/06/05 23:28:34, hajimehoshi wrote: > > > ping > > > > Sorry, I'm not sure if I understand why we're trying to do this. Why would we > > want to count a MemoryBufferBacking as shared memory, given that it's > allocated > > with "new" and can't be shared cross-process? > > The naming is confusing, but a global 'shared' dump created by > CreateSharedGlobalAllocatorDump is used not only for base::SharedMemory shared > by multiple processes but also GPU memory. Do you mean system memory allocated by the GPU process for any purpose, or actual GPU memory that the physical GPU has access to?
On 2017/06/06 02:47:02, jbauman wrote: > On 2017/06/06 00:21:57, hajimehoshi wrote: > > On 2017/06/05 23:36:25, jbauman wrote: > > > On 2017/06/05 23:28:34, hajimehoshi wrote: > > > > ping > > > > > > Sorry, I'm not sure if I understand why we're trying to do this. Why would > we > > > want to count a MemoryBufferBacking as shared memory, given that it's > > allocated > > > with "new" and can't be shared cross-process? > > > > The naming is confusing, but a global 'shared' dump created by > > CreateSharedGlobalAllocatorDump is used not only for base::SharedMemory shared > > by multiple processes but also GPU memory. > > Do you mean system memory allocated by the GPU process for any purpose, or > actual GPU memory that the physical GPU has access to? Ah sorry, in the case of BufferBacking, GPU is not used. GPU is not related in this case. However, the global shared ownership edges are still needed even if they are actually not shared by multiple processes... https://codereview.chromium.org/2866973003/#msg27 ericrk@ explained when we need the global shared ownership edges.
IIUC, all this CL should do is revert this change: https://codereview.chromium.org/2575803002 This is because it removes ownership edges for the memory dumps that are not created using base::SharedMemory. The SharedOwnershipEdge method is to avoid double counting of memory. Now since this change tracing double counts the memory that is not backed by base::SharedMemory. So, this was an error and we are reverting the change.
Thanks for the explanations, lgtm. On 2017/06/06 02:51:18, hajimehoshi wrote: > On 2017/06/06 02:47:02, jbauman wrote: > > On 2017/06/06 00:21:57, hajimehoshi wrote: > > > On 2017/06/05 23:36:25, jbauman wrote: > > > > On 2017/06/05 23:28:34, hajimehoshi wrote: > > > > > ping > > > > > > > > Sorry, I'm not sure if I understand why we're trying to do this. Why would > > we > > > > want to count a MemoryBufferBacking as shared memory, given that it's > > > allocated > > > > with "new" and can't be shared cross-process? > > > > > > The naming is confusing, but a global 'shared' dump created by > > > CreateSharedGlobalAllocatorDump is used not only for base::SharedMemory > shared > > > by multiple processes but also GPU memory. > > > > Do you mean system memory allocated by the GPU process for any purpose, or > > actual GPU memory that the physical GPU has access to? > > Ah sorry, in the case of BufferBacking, GPU is not used. GPU is not related in > this case. Ok, could you remove that from the description? > > However, the global shared ownership edges are still needed even if they are > actually not shared by multiple processes... > > https://codereview.chromium.org/2866973003/#msg27 ericrk@ explained when we need > the global shared ownership edges.
Description was changed from ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context means not only base::SharedMemory but also wider things like GPU. For example, the GPU buffer can be allocated in GPU and might be shared between processes or components. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context means not only base::SharedMemory but also wider things. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Thank you! On 2017/06/06 03:07:11, jbauman wrote: > Thanks for the explanations, lgtm. > > On 2017/06/06 02:51:18, hajimehoshi wrote: > > On 2017/06/06 02:47:02, jbauman wrote: > > > On 2017/06/06 00:21:57, hajimehoshi wrote: > > > > On 2017/06/05 23:36:25, jbauman wrote: > > > > > On 2017/06/05 23:28:34, hajimehoshi wrote: > > > > > > ping > > > > > > > > > > Sorry, I'm not sure if I understand why we're trying to do this. Why > would > > > we > > > > > want to count a MemoryBufferBacking as shared memory, given that it's > > > > allocated > > > > > with "new" and can't be shared cross-process? > > > > > > > > The naming is confusing, but a global 'shared' dump created by > > > > CreateSharedGlobalAllocatorDump is used not only for base::SharedMemory > > shared > > > > by multiple processes but also GPU memory. > > > > > > Do you mean system memory allocated by the GPU process for any purpose, or > > > actual GPU memory that the physical GPU has access to? > > > > Ah sorry, in the case of BufferBacking, GPU is not used. GPU is not related in > > this case. > > Ok, could you remove that from the description? Removed the GPU part. > > > > However, the global shared ownership edges are still needed even if they are > > actually not shared by multiple processes... > > > > https://codereview.chromium.org/2866973003/#msg27 ericrk@ explained when we > need > > the global shared ownership edges.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2912723002/#ps20001 (title: "(rebase)")
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": 20001, "attempt_start_ts": 1496718749412840,
"parent_rev": "d1c316847a808e70590eb0af0486efbd9a92152e", "commit_rev":
"3e863242981adb61aa416fda0607470df3ca0eb6"}
Message was sent while issue was closed.
Description was changed from ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context means not only base::SharedMemory but also wider things. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Revert "Bug fix: TranferBufferManager's SharedMemory memory usage reporting was wrong" This reverts commit 6c8897721e2df9a0ec3677d0a13d677c457e5e1e. The previous change removes 'shared' global dumps when the backend is not base::SharedMemory, but actually the 'shared' global dumps are needed regardless of the backend. 'Shared' in this context means not only base::SharedMemory but also wider things. BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2912723002 Cr-Commit-Position: refs/heads/master@{#477197} Committed: https://chromium.googlesource.com/chromium/src/+/3e863242981adb61aa416fda0607... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3e863242981adb61aa416fda0607... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
