Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714523006:
Refactoring: Remove DETAILED_MEMORY_INFRA (Closed)
|
Created:
4 years, 10 months ago by hajimehoshi Modified:
4 years, 10 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews, kinuko+watch, kouhei+heap_chromium.org, blink-reviews-wtf_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactoring: Remove DETAILED_MEMORY_INFRA
Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name,
let's remove the macro DETAILED_MEMORY_INFRA.
In addition, this CL removes className from GCInfo because we can
now get class lists at heap profiling.
BUG=n/a
TEST=n/a
Committed: https://crrev.com/89fb6454ae09c4390ea760428a67f82dd11a7790
Cr-Commit-Position: refs/heads/master@{#376713}
Patch Set 1 #Patch Set 2 : Surpress generating String objects #
Total comments: 5
Patch Set 3 : Remove className() #
Total comments: 5
Patch Set 4 : Remove spamming info #
Depends on Patchset: Messages
Total messages: 43 (10 generated)
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org, primiano@chromium.org
PTAL
LGTM
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/1714523006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714523006/20001
https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/GCInfo.h (right): https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* className() const { return m_className; } I thought you were removing this completely. If you want to keep it (fine), I think you want to return extractTypeNameFromFunctionName(m_className) here. Otherwise this CL is changing the semantic of className() Before: Foo Now: GcInfo::Blah<Foo> [T=Foo] https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { I wonder (for a next followup cl, not this one): if we know that we don't have class-names, is it worth spamming the regular dumps with stuff like this? https://imgur.com/n37JVaR
The CQ bit was unchecked by hajimehoshi@chromium.org
https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/GCInfo.h (right): https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* className() const { return m_className; } On 2016/02/19 10:04:51, Primiano Tucci wrote: > I thought you were removing this completely. > If you want to keep it (fine), I think you want to return > extractTypeNameFromFunctionName(m_className) here. > Otherwise this CL is changing the semantic of className() > > Before: Foo > Now: GcInfo::Blah<Foo> [T=Foo] My understanding was to remove the member variable m_className but keep className(). I failed to remove the member variable because class name can be determined when GCInfo is created. haraken-san, as primiano@ said, this CL changes the output. Is this allowable? https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { On 2016/02/19 10:04:51, Primiano Tucci wrote: > I wonder (for a next followup cl, not this one): if we know that we don't have > class-names, is it worth spamming the regular dumps with stuff like this? > > https://imgur.com/n37JVaR Hmm, this CL doesn't affect if the output is spammed, right? I don't think this CL changed the dump names (except for type names).
On 2016/02/19 10:32:03, hajimehoshi wrote: > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/GCInfo.h (right): > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* className() > const { return m_className; } > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > I thought you were removing this completely. > > If you want to keep it (fine), I think you want to return > > extractTypeNameFromFunctionName(m_className) here. > > Otherwise this CL is changing the semantic of className() > > > > Before: Foo > > Now: GcInfo::Blah<Foo> [T=Foo] > > My understanding was to remove the member variable m_className but keep > className(). I failed to remove the member variable because class name can be > determined when GCInfo is created. > > haraken-san, as primiano@ said, this CL changes the output. Is this allowable? I don't think we can remove GCInfo::m_className because it is used in ThreadState::takeSnapshot, where we don't have T. We need to store the class name in the GCInfo so that ThreadState::takeSnapshot can get the class name. > > extractTypeNameFromFunctionName(m_className) here. > > Otherwise this CL is changing the semantic of className() > > > > Before: Foo > > Now: GcInfo::Blah<Foo> [T=Foo] Good point -- let's use extractTypeNameFromFunctionName. > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > I wonder (for a next followup cl, not this one): if we know that we don't have > > class-names, is it worth spamming the regular dumps with stuff like this? > > > > https://imgur.com/n37JVaR > > Hmm, this CL doesn't affect if the output is spammed, right? I don't think this > CL changed the dump names (except for type names).
On 2016/02/19 10:40:00, haraken wrote: > On 2016/02/19 10:32:03, hajimehoshi wrote: > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/GCInfo.h (right): > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* className() > > const { return m_className; } > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > I thought you were removing this completely. > > > If you want to keep it (fine), I think you want to return > > > extractTypeNameFromFunctionName(m_className) here. > > > Otherwise this CL is changing the semantic of className() > > > > > > Before: Foo > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > My understanding was to remove the member variable m_className but keep > > className(). I failed to remove the member variable because class name can be > > determined when GCInfo is created. > > > > haraken-san, as primiano@ said, this CL changes the output. Is this allowable? > > I don't think we can remove GCInfo::m_className because it is used in > ThreadState::takeSnapshot, where we don't have T. We need to store the class > name in the GCInfo so that ThreadState::takeSnapshot can get the class name. > > > > extractTypeNameFromFunctionName(m_className) here. > > > Otherwise this CL is changing the semantic of className() > > > > > > Before: Foo > > > Now: GcInfo::Blah<Foo> [T=Foo] > > Good point -- let's use extractTypeNameFromFunctionName. Creating sub-strings for type names generates a lot of String objects and I don't think this is good. How about getting the type names in JavaScript? (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...) > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > I wonder (for a next followup cl, not this one): if we know that we don't > have > > > class-names, is it worth spamming the regular dumps with stuff like this? > > > > > > https://imgur.com/n37JVaR > > > > Hmm, this CL doesn't affect if the output is spammed, right? I don't think > this > > CL changed the dump names (except for type names).
On 2016/02/19 11:01:51, hajimehoshi wrote: > On 2016/02/19 10:40:00, haraken wrote: > > On 2016/02/19 10:32:03, hajimehoshi wrote: > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/GCInfo.h (right): > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* > className() > > > const { return m_className; } > > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > > I thought you were removing this completely. > > > > If you want to keep it (fine), I think you want to return > > > > extractTypeNameFromFunctionName(m_className) here. > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > Before: Foo > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > My understanding was to remove the member variable m_className but keep > > > className(). I failed to remove the member variable because class name can > be > > > determined when GCInfo is created. > > > > > > haraken-san, as primiano@ said, this CL changes the output. Is this > allowable? > > > > I don't think we can remove GCInfo::m_className because it is used in > > ThreadState::takeSnapshot, where we don't have T. We need to store the class > > name in the GCInfo so that ThreadState::takeSnapshot can get the class name. > > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > Before: Foo > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > Good point -- let's use extractTypeNameFromFunctionName. > > Creating sub-strings for type names generates a lot of String objects and I > don't think this is good. How about getting the type names in JavaScript? (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...) Since this is enabled only on the heap-profiling mode, I don't think we need to worry about the memory.
So, let me try to do a quick recap: who are the clients left for className() ? My understanding (but I might be terribly wrong) is that today we dump the oilpan class names in memory-infra twice: 1) In the standard dumps like this: https://imgur.com/n37JVaR. This path does use className() 2) When using --enable-heap-profiling. IIRC, as per crrev.com/1711643002 the heap profiler dumpers does NOT use className() but directly the WTF_HEAP_PROFILER_TYPE_NAME macro. Now my question is: are we keeping className() alive just for 1) ? Because if that is the case, I think we can drop that entirely. Unless you feel useful having the class names even when you do NOT use --enable-heap-profiling. My personal preference would be to drop 1) and keep just --enable-heap-profiling, as that would keep the trace size of the NON --enable-heap-profiling case smaller. But that depends more on your needs really. https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { On 2016/02/19 10:32:03, hajimehoshi wrote: > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > I wonder (for a next followup cl, not this one): if we know that we don't have > > class-names, is it worth spamming the regular dumps with stuff like this? > > > > https://imgur.com/n37JVaR > > Hmm, this CL doesn't affect if the output is spammed, right? I don't think this > CL changed the dump names (except for type names). Right. I am just saying that now that we have the heap profiler, having a second copy of the class list in the standard dump might be superfluous? WDYT? In any case, nothing that belongs to this CL.
On 2016/02/19 11:06:04, haraken wrote: > On 2016/02/19 11:01:51, hajimehoshi wrote: > > On 2016/02/19 10:40:00, haraken wrote: > > > On 2016/02/19 10:32:03, hajimehoshi wrote: > > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/platform/heap/GCInfo.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* > > className() > > > > const { return m_className; } > > > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > > > I thought you were removing this completely. > > > > > If you want to keep it (fine), I think you want to return > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > > > Before: Foo > > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > > > My understanding was to remove the member variable m_className but keep > > > > className(). I failed to remove the member variable because class name can > > be > > > > determined when GCInfo is created. > > > > > > > > haraken-san, as primiano@ said, this CL changes the output. Is this > > allowable? > > > > > > I don't think we can remove GCInfo::m_className because it is used in > > > ThreadState::takeSnapshot, where we don't have T. We need to store the class > > > name in the GCInfo so that ThreadState::takeSnapshot can get the class name. > > > > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > > > Before: Foo > > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > Good point -- let's use extractTypeNameFromFunctionName. > > > > Creating sub-strings for type names generates a lot of String objects and I > > don't think this is good. How about getting the type names in JavaScript? > (e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...) > > Since this is enabled only on the heap-profiling mode, I don't think we need to > worry about the memory. Those class names are used for memory-infra without --enable-heap-profiling.
On 2016/02/19 11:11:52, Primiano Tucci wrote: > So, let me try to do a quick recap: > who are the clients left for className() ? > My understanding (but I might be terribly wrong) is that today we dump the > oilpan class names in memory-infra twice: > 1) In the standard dumps like this: https://imgur.com/n37JVaR. > This path does use className() > 2) When using --enable-heap-profiling. IIRC, as per crrev.com/1711643002 the > heap profiler dumpers does NOT use className() but directly the > WTF_HEAP_PROFILER_TYPE_NAME macro. > > Now my question is: are we keeping className() alive just for 1) ? Because if > that is the case, I think we can drop that entirely. Unless you feel useful > having the class names even when you do NOT use --enable-heap-profiling. > My personal preference would be to drop 1) and keep just > --enable-heap-profiling, as that would keep the trace size of the NON > --enable-heap-profiling case smaller. But that depends more on your needs > really. Ah, nice catch! You're totally right. Our intention is to store the class name only when --enable-heap-profiling is enabled. So 2) should be enough. > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > On 2016/02/19 10:32:03, hajimehoshi wrote: > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > I wonder (for a next followup cl, not this one): if we know that we don't > have > > > class-names, is it worth spamming the regular dumps with stuff like this? > > > > > > https://imgur.com/n37JVaR > > > > Hmm, this CL doesn't affect if the output is spammed, right? I don't think > this > > CL changed the dump names (except for type names). > > Right. I am just saying that now that we have the heap profiler, having a second > copy of the class list in the standard dump might be superfluous? WDYT? > In any case, nothing that belongs to this CL.
On 2016/02/19 11:13:05, hajimehoshi wrote: > On 2016/02/19 11:06:04, haraken wrote: > > On 2016/02/19 11:01:51, hajimehoshi wrote: > > > On 2016/02/19 10:40:00, haraken wrote: > > > > On 2016/02/19 10:32:03, hajimehoshi wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/platform/heap/GCInfo.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* > > > className() > > > > > const { return m_className; } > > > > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > > > > I thought you were removing this completely. > > > > > > If you want to keep it (fine), I think you want to return > > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > > > > > Before: Foo > > > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > > > > > My understanding was to remove the member variable m_className but keep > > > > > className(). I failed to remove the member variable because class name > can > > > be > > > > > determined when GCInfo is created. > > > > > > > > > > haraken-san, as primiano@ said, this CL changes the output. Is this > > > allowable? > > > > > > > > I don't think we can remove GCInfo::m_className because it is used in > > > > ThreadState::takeSnapshot, where we don't have T. We need to store the > class > > > > name in the GCInfo so that ThreadState::takeSnapshot can get the class > name. > > > > > > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > > > > > Before: Foo > > > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > > > Good point -- let's use extractTypeNameFromFunctionName. > > > > > > Creating sub-strings for type names generates a lot of String objects and I > > > don't think this is good. How about getting the type names in JavaScript? > > (e.g. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...) > > > > Since this is enabled only on the heap-profiling mode, I don't think we need > to > > worry about the memory. > > Those class names are used for memory-infra without --enable-heap-profiling. We cannot do that anyway since the stored strings bloat the binary size (even if we remove temporary sub-strings etc). In conclusion, as primiano@ pointed out, a right fix is just to remove the m_className.
On 2016/02/19 11:16:22, haraken wrote: > On 2016/02/19 11:13:05, hajimehoshi wrote: > > On 2016/02/19 11:06:04, haraken wrote: > > > On 2016/02/19 11:01:51, hajimehoshi wrote: > > > > On 2016/02/19 10:40:00, haraken wrote: > > > > > On 2016/02/19 10:32:03, hajimehoshi wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/platform/heap/GCInfo.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/platform/heap/GCInfo.h:130: const char* > > > > className() > > > > > > const { return m_className; } > > > > > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > > > > > I thought you were removing this completely. > > > > > > > If you want to keep it (fine), I think you want to return > > > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > > > > > > > Before: Foo > > > > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > > > > > > > My understanding was to remove the member variable m_className but > keep > > > > > > className(). I failed to remove the member variable because class name > > can > > > > be > > > > > > determined when GCInfo is created. > > > > > > > > > > > > haraken-san, as primiano@ said, this CL changes the output. Is this > > > > allowable? > > > > > > > > > > I don't think we can remove GCInfo::m_className because it is used in > > > > > ThreadState::takeSnapshot, where we don't have T. We need to store the > > class > > > > > name in the GCInfo so that ThreadState::takeSnapshot can get the class > > name. > > > > > > > > > > > > extractTypeNameFromFunctionName(m_className) here. > > > > > > > Otherwise this CL is changing the semantic of className() > > > > > > > > > > > > > > Before: Foo > > > > > > > Now: GcInfo::Blah<Foo> [T=Foo] > > > > > > > > > > Good point -- let's use extractTypeNameFromFunctionName. > > > > > > > > Creating sub-strings for type names generates a lot of String objects and > I > > > > don't think this is good. How about getting the type names in JavaScript? > > > (e.g. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...) > > > > > > Since this is enabled only on the heap-profiling mode, I don't think we need > > to > > > worry about the memory. > > > > Those class names are used for memory-infra without --enable-heap-profiling. > > We cannot do that anyway since the stored strings bloat the binary size (even if > we remove temporary sub-strings etc). > > In conclusion, as primiano@ pointed out, a right fix is just to remove the > m_className. OK, I understood. Let me remove m_className.
On 2016/02/19 11:11:52, Primiano Tucci wrote: > So, let me try to do a quick recap: > who are the clients left for className() ? > My understanding (but I might be terribly wrong) is that today we dump the > oilpan class names in memory-infra twice: > 1) In the standard dumps like this: https://imgur.com/n37JVaR. > This path does use className() > 2) When using --enable-heap-profiling. IIRC, as per crrev.com/1711643002 the > heap profiler dumpers does NOT use className() but directly the > WTF_HEAP_PROFILER_TYPE_NAME macro. > > Now my question is: are we keeping className() alive just for 1) ? Because if > that is the case, I think we can drop that entirely. Unless you feel useful > having the class names even when you do NOT use --enable-heap-profiling. > My personal preference would be to drop 1) and keep just > --enable-heap-profiling, as that would keep the trace size of the NON > --enable-heap-profiling case smaller. But that depends more on your needs > really. > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1714523006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > On 2016/02/19 10:32:03, hajimehoshi wrote: > > On 2016/02/19 10:04:51, Primiano Tucci wrote: > > > I wonder (for a next followup cl, not this one): if we know that we don't > have > > > class-names, is it worth spamming the regular dumps with stuff like this? > > > > > > https://imgur.com/n37JVaR > > > > Hmm, this CL doesn't affect if the output is spammed, right? I don't think > this > > CL changed the dump names (except for type names). > > Right. I am just saying that now that we have the heap profiler, having a second > copy of the class list in the standard dump might be superfluous? WDYT? > In any case, nothing that belongs to this CL. Ah, I got it. I second your idea to remove the class list other than the heap profiler. BTW, can we say the same thing about partition alloc profiler?
Description was changed from ========== Refactoring: Remove DETAILED_MEMORY_INFRA Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name, let's remove the macro DETAILED_MEMORY_INFRA. BUG=n/a TEST=n/a ========== to ========== Refactoring: Remove DETAILED_MEMORY_INFRA Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name, let's remove the macro DETAILED_MEMORY_INFRA. In addition, this CL removes className from GCInfo because we can now get class lists at heap profiling. BUG=n/a TEST=n/a ==========
PTAL
LGTM, thanks!
https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { I think you want to remove this for loop entirely (line 1481-1494) and just keep the per-heap stats ? This is going to output a row for each class, but just print out numeric IDs for them like this: https://imgur.com/n37JVaR which I am not sure if they are really useful. What does tell you if if threadX/classes/101 is using X size?
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Visitor.h:45: #include "wtf/InstanceCounter.h" This now no longer needed.
Thank you! https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { On 2016/02/19 11:58:04, Primiano Tucci wrote: > I think you want to remove this for loop entirely (line 1481-1494) and just keep > the per-heap stats ? > > This is going to output a row for each class, but just print out numeric IDs for > them like this: https://imgur.com/n37JVaR > which I am not sure if they are really useful. > What does tell you if if threadX/classes/101 is using X size? I was thinking I'll remove this later, but you are right, I can remove this logic now. However, this loop is needed to calculate the total live/dead count/size. Done https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Visitor.h:45: #include "wtf/InstanceCounter.h" On 2016/02/19 12:00:49, sof wrote: > This now no longer needed. Oh, thanks! Done
https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { On 2016/02/19 11:58:04, Primiano Tucci wrote: > I think you want to remove this for loop entirely (line 1481-1494) and just keep > the per-heap stats ? > > This is going to output a row for each class, but just print out numeric IDs for > them like this: https://imgur.com/n37JVaR > which I am not sure if they are really useful. > What does tell you if if threadX/classes/101 is using X size? BTW, if we remove the loop, won't we lose the information about liveCount, deadCount, liveSize and deadSize per object type? Sorry for getting you back and forth, but I begin to think that we need the information per object type and thus we still need GCInfo::m_classname (to print a meaningful name here). GCInfo::m_classname is "" by default and stores a class name when --enable-heap-profiling is specified. (Correct me if I'm wrong.)
On 2016/02/19 12:19:40, haraken wrote: > https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > On 2016/02/19 11:58:04, Primiano Tucci wrote: > > I think you want to remove this for loop entirely (line 1481-1494) and just > keep > > the per-heap stats ? > > > > This is going to output a row for each class, but just print out numeric IDs > for > > them like this: https://imgur.com/n37JVaR > > which I am not sure if they are really useful. > > What does tell you if if threadX/classes/101 is using X size? > > BTW, if we remove the loop, won't we lose the information about liveCount, > deadCount, liveSize and deadSize per object type? Right, and I kept the logic. Don't worry! > > Sorry for getting you back and forth, but I begin to think that we need the > information per object type and thus we still need GCInfo::m_classname (to print > a meaningful name here). > > GCInfo::m_classname is "" by default and stores a class name when > --enable-heap-profiling is specified. > > (Correct me if I'm wrong.) IIUC, the same information is listed in the heap profiler (with --enable-heap-profiling), and primiano@ pointed out that we don't have to have two same lists.
On 2016/02/19 12:19:40, haraken wrote: > BTW, if we remove the loop, won't we lose the information about liveCount, > deadCount, liveSize and deadSize per object type? Correct, that's why I was asking if we need also that one. > GCInfo::m_classname is "" by default and stores a class name when > --enable-heap-profiling is specified. So, if we set m_className to WTF_HEAP_PROFILER_TYPE_NAME we would get a non-empty string in all non-official builds (regardless of --enable-heap-profiling) if we pass --enable-heap-profiling then we will obviously get ALSO the names in the heap profiler. So, if you want the full live-ojbects stats the question becomes: should we keep doing the string munging in chrome using extractTypeNameFromFunctionName? Or should we pass the long string to tracing and ask petrcermak to replicate the munging logic also for the BlinkGC heap stats (i.e. NOT just the heap profiler one?) Probably the best answer would be do first using extractTypeNameFromFunctionName() in className() getter, which adds some runtime penalty and memory for Strings (But only when using tracing+memory-infra) and improve it later? Does it make sense?
On 2016/02/19 12:27:17, hajimehoshi wrote: > On 2016/02/19 12:19:40, haraken wrote: > > > https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > > On 2016/02/19 11:58:04, Primiano Tucci wrote: > > > I think you want to remove this for loop entirely (line 1481-1494) and just > > keep > > > the per-heap stats ? > > > > > > This is going to output a row for each class, but just print out numeric IDs > > for > > > them like this: https://imgur.com/n37JVaR > > > which I am not sure if they are really useful. > > > What does tell you if if threadX/classes/101 is using X size? > > > > BTW, if we remove the loop, won't we lose the information about liveCount, > > deadCount, liveSize and deadSize per object type? > > Right, and I kept the logic. Don't worry! > > > > > Sorry for getting you back and forth, but I begin to think that we need the > > information per object type and thus we still need GCInfo::m_classname (to > print > > a meaningful name here). > > > > GCInfo::m_classname is "" by default and stores a class name when > > --enable-heap-profiling is specified. > > > > (Correct me if I'm wrong.) > > IIUC, the same information is listed in the heap profiler (with > --enable-heap-profiling), and primiano@ pointed out that we don't have to have > two same lists. If the heap profiler already has that info, I'm fine with removing it (i.e., PS4 looks good). (Sorry, I don't have an env to check memory-infra on ToT.)
hajimehoshi@chromium.org changed reviewers: + tasak@google.com
On 2016/02/19 12:32:46, haraken wrote: > On 2016/02/19 12:27:17, hajimehoshi wrote: > > On 2016/02/19 12:19:40, haraken wrote: > > > > > > https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > https://codereview.chromium.org/1714523006/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:1481: for (size_t > > > gcInfoIndex = 1; gcInfoIndex <= GCInfoTable::gcInfoIndex(); ++gcInfoIndex) { > > > On 2016/02/19 11:58:04, Primiano Tucci wrote: > > > > I think you want to remove this for loop entirely (line 1481-1494) and > just > > > keep > > > > the per-heap stats ? > > > > > > > > This is going to output a row for each class, but just print out numeric > IDs > > > for > > > > them like this: https://imgur.com/n37JVaR > > > > which I am not sure if they are really useful. > > > > What does tell you if if threadX/classes/101 is using X size? > > > > > > BTW, if we remove the loop, won't we lose the information about liveCount, > > > deadCount, liveSize and deadSize per object type? > > > > Right, and I kept the logic. Don't worry! > > > > > > > > Sorry for getting you back and forth, but I begin to think that we need the > > > information per object type and thus we still need GCInfo::m_classname (to > > print > > > a meaningful name here). > > > > > > GCInfo::m_classname is "" by default and stores a class name when > > > --enable-heap-profiling is specified. > > > > > > (Correct me if I'm wrong.) > > > > IIUC, the same information is listed in the heap profiler (with > > --enable-heap-profiling), and primiano@ pointed out that we don't have to have > > two same lists. > > If the heap profiler already has that info, I'm fine with removing it (i.e., PS4 > looks good). > > (Sorry, I don't have an env to check memory-infra on ToT.) +tasak, who is a heavy user of memory-infra.
haraken and I had a chat offline looking at a trace. I think the agreement is that since we have per-heap stats live/dead object count AND the new heap profiler, should be fine to drop the per-class-id live/dead stats. Concretely this means that we can remove most of the things in that for-loop (the per-object dumping part) Let's wait for tasak@ to confirm.
On 2016/02/19 15:22:32, Primiano Tucci wrote: > haraken and I had a chat offline looking at a trace. > I think the agreement is that since we have per-heap stats live/dead object > count AND the new heap profiler, should be fine to drop the per-class-id > live/dead stats. > Concretely this means that we can remove most of the things in that for-loop > (the per-object dumping part) > Let's wait for tasak@ to confirm. No problem. I agree with you.
On 2016/02/22 09:23:11, tasak wrote: > On 2016/02/19 15:22:32, Primiano Tucci wrote: > > haraken and I had a chat offline looking at a trace. > > I think the agreement is that since we have per-heap stats live/dead object > > count AND the new heap profiler, should be fine to drop the per-class-id > > live/dead stats. > > Concretely this means that we can remove most of the things in that for-loop > > (the per-object dumping part) > > Let's wait for tasak@ to confirm. > > No problem. I agree with you. Thank you! PTAL
On 2016/02/22 09:47:16, hajimehoshi wrote: > On 2016/02/22 09:23:11, tasak wrote: > > On 2016/02/19 15:22:32, Primiano Tucci wrote: > > > haraken and I had a chat offline looking at a trace. > > > I think the agreement is that since we have per-heap stats live/dead object > > > count AND the new heap profiler, should be fine to drop the per-class-id > > > live/dead stats. > > > Concretely this means that we can remove most of the things in that for-loop > > > (the per-object dumping part) Ah, sorry but does this mean PS4 is fine?
On 2016/02/22 09:51:52, hajimehoshi wrote: > On 2016/02/22 09:47:16, hajimehoshi wrote: > > On 2016/02/22 09:23:11, tasak wrote: > > > On 2016/02/19 15:22:32, Primiano Tucci wrote: > > > > haraken and I had a chat offline looking at a trace. > > > > I think the agreement is that since we have per-heap stats live/dead > object > > > > count AND the new heap profiler, should be fine to drop the per-class-id > > > > live/dead stats. > > > > Concretely this means that we can remove most of the things in that > for-loop > > > > (the per-object dumping part) > > Ah, sorry but does this mean PS4 is fine? Yes, PS4 LGTM. You cannot remove the for loop.
Patchset #5 (id:80001) has been deleted
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/1714523006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714523006/60001
Message was sent while issue was closed.
Description was changed from ========== Refactoring: Remove DETAILED_MEMORY_INFRA Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name, let's remove the macro DETAILED_MEMORY_INFRA. In addition, this CL removes className from GCInfo because we can now get class lists at heap profiling. BUG=n/a TEST=n/a ========== to ========== Refactoring: Remove DETAILED_MEMORY_INFRA Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name, let's remove the macro DETAILED_MEMORY_INFRA. In addition, this CL removes className from GCInfo because we can now get class lists at heap profiling. BUG=n/a TEST=n/a ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactoring: Remove DETAILED_MEMORY_INFRA Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name, let's remove the macro DETAILED_MEMORY_INFRA. In addition, this CL removes className from GCInfo because we can now get class lists at heap profiling. BUG=n/a TEST=n/a ========== to ========== Refactoring: Remove DETAILED_MEMORY_INFRA Now that we can use WTF_HEAP_PROFILER_TYPE_NAME to get a type name, let's remove the macro DETAILED_MEMORY_INFRA. In addition, this CL removes className from GCInfo because we can now get class lists at heap profiling. BUG=n/a TEST=n/a Committed: https://crrev.com/89fb6454ae09c4390ea760428a67f82dd11a7790 Cr-Commit-Position: refs/heads/master@{#376713} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/89fb6454ae09c4390ea760428a67f82dd11a7790 Cr-Commit-Position: refs/heads/master@{#376713}
Message was sent while issue was closed.
On 2016/02/22 11:21:20, commit-bot: I haz the power wrote: > Committed patchset #4 (id:60001) Yup. Sorry for the back and forths it took to converge. LGTM. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
