Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1676453002:
Change initialization strategy of AllocationRegister back (Closed)
|
Created:
4 years, 10 months ago by hajimehoshi Modified:
4 years, 10 months ago CC:
blink-reviews, chromium-reviews, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange initialization strategy of AllocationRegister back
AllocationRegister in PartitionAllocMemoryDumpProvider should be
initialized only when necessary to avoid waste of memory.
This initialization strategy was changed at crrev.com/1642023007
accidentally, and this CL changes this back.
BUG=584611
TEST=n/a
Committed: https://crrev.com/d7036a2197e53a8d5a33b11a0c4967046431257f
Cr-Commit-Position: refs/heads/master@{#373802}
Patch Set 1 #
Total comments: 4
Depends on Patchset: Messages
Total messages: 15 (4 generated)
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org, primiano@chromium.org, tasak@google.com
PTAL
LGTM https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:172: MutexLocker locker(m_allocationRegisterMutex); Nit: It would be heavy to use MutexLocker in insert/remove. You could use releaseStore/acquireLoad. You don't need to address it in this CL.
LGTM thanks for the prompt fix! https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:172: MutexLocker locker(m_allocationRegisterMutex); On 2016/02/05 11:20:59, haraken wrote: > > Nit: It would be heavy to use MutexLocker in insert/remove. You could use > releaseStore/acquireLoad. You don't need to address it in this CL. > Note that here we have also to protect against concurrent accesses to the allocation register (Which is not thread safe) is releaseStore/acquireLoad going to protect this?
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1676453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1676453002/1
> https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp > (right): > > https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:172: > MutexLocker locker(m_allocationRegisterMutex); > On 2016/02/05 11:20:59, haraken wrote: > > > > Nit: It would be heavy to use MutexLocker in insert/remove. You could use > > releaseStore/acquireLoad. You don't need to address it in this CL. > > > > Note that here we have also to protect against concurrent accesses to the > allocation register (Which is not thread safe) > is releaseStore/acquireLoad going to protect this? Ah, MutexLocker makes sense.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Change initialization strategy of AllocationRegister back AllocationRegister in PartitionAllocMemoryDumpProvider should be initialized only when necessary to avoid waste of memory. This initialization strategy was changed at crrev.com/1642023007 accidentally, and this CL changes this back. BUG=584611 TEST=n/a ========== to ========== Change initialization strategy of AllocationRegister back AllocationRegister in PartitionAllocMemoryDumpProvider should be initialized only when necessary to avoid waste of memory. This initialization strategy was changed at crrev.com/1642023007 accidentally, and this CL changes this back. BUG=584611 TEST=n/a Committed: https://crrev.com/d7036a2197e53a8d5a33b11a0c4967046431257f Cr-Commit-Position: refs/heads/master@{#373802} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/d7036a2197e53a8d5a33b11a0c4967046431257f Cr-Commit-Position: refs/heads/master@{#373802}
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:163: PartitionAllocHooks::setFreeHook(nullptr); should we clear m_allocationRegister here? What eventually cleans up m_allocationRegister?
Message was sent while issue was closed.
https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:163: PartitionAllocHooks::setFreeHook(nullptr); On 2016/02/09 07:07:13, esprehn wrote: > should we clear m_allocationRegister here? What eventually cleans up > m_allocationRegister? That's a good point, missed it. So, right now there is nothing ever tearing down heap profiler once its started (you only see the "enabled=true" part in this function) but that is a bug I'm going to fix soon. FWIW, I can guarantee you that we are never going to support re-enabling heap profiling (enable -> disable -> enable). The reason is that it is conceptually wrong. If you interrupt heap profiling you lose data and the result is meaningless. But not sure whether this is going to make a difference in this code at all.
Message was sent while issue was closed.
On 2016/02/09 at 11:00:24, primiano wrote: > https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): > > https://codereview.chromium.org/1676453002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:163: PartitionAllocHooks::setFreeHook(nullptr); > On 2016/02/09 07:07:13, esprehn wrote: > > should we clear m_allocationRegister here? What eventually cleans up > > m_allocationRegister? > > That's a good point, missed it. > So, right now there is nothing ever tearing down heap profiler once its started (you only see the "enabled=true" part in this function) but that is a bug I'm going to fix soon. > > FWIW, I can guarantee you that we are never going to support re-enabling heap profiling (enable -> disable -> enable). > The reason is that it is conceptually wrong. If you interrupt heap profiling you lose data and the result is meaningless. > But not sure whether this is going to make a difference in this code at all. Does that mean if I open about:tracing, record a memory profile, close tracing, I can never record another memory profile? Or that I'm stuck with a permanent memory increase after recording the memory profile?
Message was sent while issue was closed.
On 2016/02/09 11:43:40, esprehn wrote: > Does that mean if I open about:tracing, record a memory profile, close tracing, > I can never record another memory profile? Or that I'm stuck with a permanent > memory increase after recording the memory profile? Nope that will work as you expect. The "only once" exception applies only to the special case of: - you start chrome with --enable-heap-profiling - you start tracing with memory-infra (which will give you the extra heap details). In this scenario: Today: you are stuck with the extra memory increase of the register even after you stop tracing. As I said this is a bug I am going to fix soon (just didn't bother prioritizing that). In the near future: Once you stop tracing, memory usage will go back as expected. If you restart tracing, memory infra will work as expected w.r.t. the standard probes that we have. However, the pane about the heap details will show some red warning (inconsistent data or similar) The reason is: if you stop profiling, you stop tracking malloc/partalloc allocations, you lose data that you are never going to be able to catch again. You could argue that you might still be able to get the data for the allocations happened in the timeframe [when you re-enabled tracing the 2nd time, when you stopped tracing the 2nd time] that will work, but that data is going to be misleading, as you dropped on the floor all the allocations that happened before, and that might be still outstanding. That's why I said above that, for detailed heap profiling, I intend to support only the case of: you trace once, since the beginning, and once you stop you are done. Technically we could do that, but the final result would be so hard to explain (due to the reason above) that I prefer saying "you can't do this, period". Does it make sense? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
