Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1721333002:
Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing (Closed)
|
Created:
4 years, 10 months ago by hajimehoshi Modified:
4 years, 9 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews-css, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, kinuko+watch, kouhei+heap_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing
BlinkGC allocation hooks for the memory infra were added at
crrev.com/1676973002, but those were not enough. This CL adds missing
hooks.
BUG=588123
Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306
Cr-Commit-Position: refs/heads/master@{#379252}
Committed: https://crrev.com/d7c89d879b56ebad4e04d2bbc0452d35c1fa1f25
Cr-Commit-Position: refs/heads/master@{#379280}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Hook in allocateOnHeapIndex #
Total comments: 15
Patch Set 3 : Fix type names #Patch Set 4 : Use 'using' #Patch Set 5 : (rebasing) #Patch Set 6 : (rebasing) #Patch Set 7 : Use COMMA #
Depends on Patchset: Messages
Total messages: 59 (16 generated)
hajimehoshi@chromium.org changed reviewers: + haraken@chromium.org, primiano@chromium.org
PTAL
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); Can you guarantee that this doesn't add a single extra cycle to the object allocation path in official builds?
https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); On 2016/02/23 09:13:02, sof wrote: > Can you guarantee that this doesn't add a single extra cycle to the object > allocation path in official builds? Yes. The actual hooks doesn't allocate any objects from BlinkGC. https://codereview.chromium.org/1701813003/diff/40001/third_party/WebKit/Sour...
On 2016/02/23 09:27:17, hajimehoshi wrote: > https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): > > https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: > HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); > On 2016/02/23 09:13:02, sof wrote: > > Can you guarantee that this doesn't add a single extra cycle to the object > > allocation path in official builds? > > Yes. The actual hooks doesn't allocate any objects from BlinkGC. > https://codereview.chromium.org/1701813003/diff/40001/third_party/WebKit/Sour... I wouldn't expect them to, but I don't want the overhead of calling a no-op for every backing store allocation (or have its code inlined at allocation sites). Or any other heap allocation, are we sure that what http://crrev.com/1676973002 added doesn't cause any perf regressions?
On 2016/02/23 09:35:29, sof wrote: > On 2016/02/23 09:27:17, hajimehoshi wrote: > > > https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): > > > > > https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: > > HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); > > On 2016/02/23 09:13:02, sof wrote: > > > Can you guarantee that this doesn't add a single extra cycle to the object > > > allocation path in official builds? > > > > Yes. The actual hooks doesn't allocate any objects from BlinkGC. > > > https://codereview.chromium.org/1701813003/diff/40001/third_party/WebKit/Sour... > > I wouldn't expect them to, but I don't want the overhead of calling a no-op for > every backing store allocation (or have its code inlined at allocation sites). > > Or any other heap allocation, are we sure that what http://crrev.com/1676973002 > added doesn't cause any perf regressions? iow, one thing I think would be worth doing here is to have this hooking of allocations be packaged up in a macro that expands to nothing for builds that don't support it. Perhaps even better would be to push the allocationHookIfEnabled() call into Heap::allocateOnHeapIndex(), as all(?) call sites will now use this hook.
https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); Is there any way we can make this future proof such that if somebody adds a new call to allocateOnHeapIndex somehwere else we don't forget to add a call to the hook? Maybe move the hook inside Heap::allocateOnHeapIndex and pass the type as const char*?
https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); On 2016/02/25 06:55:51, Primiano (throttled til Mar 4) wrote: > Is there any way we can make this future proof such that if somebody adds a new > call to allocateOnHeapIndex somehwere else we don't forget to add a call to the > hook? > Maybe move the hook inside Heap::allocateOnHeapIndex and pass the type as const > char*? Good point. Such hooks should be in allocateOnHeapIndex. I found the missing hooks just by searching allocateOnHeapIndex. BTW, as I said at the issue, the hooks added by this CL might cause performance regression. I suggested using a build macro, but if we move these hooks in allocateOnHeapIndex, can't we see the Oilpan heap profiler without the build macro?
On 2016/02/25 07:06:01, hajimehoshi wrote: > https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): > > https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: > HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); > On 2016/02/25 06:55:51, Primiano (throttled til Mar 4) wrote: > > Is there any way we can make this future proof such that if somebody adds a > new > > call to allocateOnHeapIndex somehwere else we don't forget to add a call to > the > > hook? > > Maybe move the hook inside Heap::allocateOnHeapIndex and pass the type as > const > > char*? > > Good point. Such hooks should be in allocateOnHeapIndex. I found the missing > hooks just by searching allocateOnHeapIndex. > > BTW, as I said at the issue, the hooks added by this CL might cause performance > regression. I suggested using a build macro, but if we move these hooks in > allocateOnHeapIndex, can't we see the Oilpan heap profiler without the build > macro? Answered on the bug. Let's keep the discussion there
https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/HeapAllocator.h:67: HeapAllocHooks::allocationHookIfEnabled(address, size, typeName); On 2016/02/25 06:55:51, Primiano (throttled til Mar 4) wrote: > Is there any way we can make this future proof such that if somebody adds a new > call to allocateOnHeapIndex somehwere else we don't forget to add a call to the > hook? > Maybe move the hook inside Heap::allocateOnHeapIndex and pass the type as const > char*? Done.
LGTM with some suggestions. IIRC the point of using WTF_HEAP_PROFILER_TYPE_NAME is: - If you need the <T> expansion (which doesn't seem the case here) - If you want to save the binary size for official builds. In these cases feels like that if you add the actual string, you pay few bytes of binary size but you'll get this data in official builds. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSValue.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSValue.h:47: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(CSSValue); Considering that IIUC this is a pretty heavy user and that the string "CSSValue" is just 9 bytes, should this be just const char typeName[] = "CSSValue", so we get this also in official builds? You don't really need the <T> expansion here, right? https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:187: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(Node); ditto here https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapTest.cpp:778: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(IntNode); don't know how frequent this is, if is significant, 8 extra bytes for "IntNode" won't be that bad :)
https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.h:482: Address address = heap->allocateObject(allocationSizeFromSize(size), gcInfoIndex); Given that allocateOnHeapIndex() is also inlined, I'd expect the performance of this to be equal to what it was in the previous patchset. As this is the only call site for allocationHookIfEnabled(), I suggest you replace this with a macro (or equivalent #if-ery) that expands to nothing if the build doesn't support allocation hooks.
https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.h:482: Address address = heap->allocateObject(allocationSizeFromSize(size), gcInfoIndex); On 2016/02/29 18:36:59, sof wrote: > As this is the only call site for allocationHookIfEnabled(), I suggest you > replace this with a macro (or equivalent #if-ery) that expands to nothing if the > build doesn't support allocation hooks. All the builds support allocation hooks, the macro is only about which string make it to production and which not (for the sake of binary size)
On 2016/02/29 18:38:45, Primiano (throttled til Mar 4) wrote: > https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/Heap.h:482: Address address = > heap->allocateObject(allocationSizeFromSize(size), gcInfoIndex); > On 2016/02/29 18:36:59, sof wrote: > > As this is the only call site for allocationHookIfEnabled(), I suggest you > > replace this with a macro (or equivalent #if-ery) that expands to nothing if > the > > build doesn't support allocation hooks. > > All the builds support allocation hooks, the macro is only about which string > make it to production and which not (for the sake of binary size) If so, why the UNLIKELY() null check in allocationHookIfEnabled() ?
On 2016/02/29 18:44:20, sof wrote: > On 2016/02/29 18:38:45, Primiano (throttled til Mar 4) wrote: > > > https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > > > > https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/Heap.h:482: Address address = > > heap->allocateObject(allocationSizeFromSize(size), gcInfoIndex); > > On 2016/02/29 18:36:59, sof wrote: > > > As this is the only call site for allocationHookIfEnabled(), I suggest you > > > replace this with a macro (or equivalent #if-ery) that expands to nothing if > > the > > > build doesn't support allocation hooks. > > > > All the builds support allocation hooks, the macro is only about which string > > make it to production and which not (for the sake of binary size) > > If so, why the UNLIKELY() null check in allocationHookIfEnabled() ? That is a run-time check, not build time. Let me rephrase my sentence: all the builds support (w.r.t. build time config) heap profiling. But heap profiling needs to be enabled at runtime (--enable-heap-profiling), otherwise all this is a LIKELY-noop.
On 2016/02/29 18:45:49, Primiano (throttled til Mar 4) wrote: > On 2016/02/29 18:44:20, sof wrote: > > On 2016/02/29 18:38:45, Primiano (throttled til Mar 4) wrote: > > > > > > https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > > > > > > > > https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/Heap.h:482: Address address = > > > heap->allocateObject(allocationSizeFromSize(size), gcInfoIndex); > > > On 2016/02/29 18:36:59, sof wrote: > > > > As this is the only call site for allocationHookIfEnabled(), I suggest you > > > > replace this with a macro (or equivalent #if-ery) that expands to nothing > if > > > the > > > > build doesn't support allocation hooks. > > > > > > All the builds support allocation hooks, the macro is only about which > string > > > make it to production and which not (for the sake of binary size) > > > > If so, why the UNLIKELY() null check in allocationHookIfEnabled() ? > > That is a run-time check, not build time. > Let me rephrase my sentence: all the builds support (w.r.t. build time config) > heap profiling. But heap profiling needs to be enabled at runtime > (--enable-heap-profiling), otherwise all this is a LIKELY-noop. If you don't have a type name string (i.e., official builds), then it brings sufficient value to register each allocation rather than just using the overall heap stats?
On 2016/02/29 18:56:43, sof wrote: > If you don't have a type name string (i.e., official builds), then it brings > sufficient value to register each allocation rather than just using the overall > heap stats? The deal is the following: if you don't have the type name, it still works in production builds, but you can only see the stack-trace of where the allocation came from, but not its type. If you have the type name you can see also the type, see the bottom of https://chromium.googlesource.com/chromium/src/+/master/components/tracing/do... as an example. The deal about typenames is: we cannot add blindly to all templated class, as that would increase too mnuch the binary size. But for the key classes, we annotated them to have the string even in production build, as that still gives useful data (the type of an allocation) paying few bytes of binary.
On 2016/02/29 19:00:17, Primiano (throttled til Mar 4) wrote: > On 2016/02/29 18:56:43, sof wrote: > > If you don't have a type name string (i.e., official builds), then it brings > > sufficient value to register each allocation rather than just using the > overall > > heap stats? > The deal is the following: > if you don't have the type name, it still works in production builds, but you > can only see the stack-trace of where the allocation came from, but not its > type. > If you have the type name you can see also the type, see the bottom of > https://chromium.googlesource.com/chromium/src/+/master/components/tracing/do... > as an example. > > The deal about typenames is: we cannot add blindly to all templated class, as > that would increase too mnuch the binary size. But for the key classes, we > annotated them to have the string even in production build, as that still gives > useful data (the type of an allocation) paying few bytes of binary. If this refers to CSSValue and Node, we have separate heaps for those that would report overall allocations, so what's the added value beyond that to call a hook?
On 2016/02/29 19:10:47, sof wrote: > If this refers to CSSValue and Node, we have separate heaps for those that would > report overall allocations, so what's the added value beyond that to call a > hook? Oh I see the question. Those are two slighly different reporting mechanisms: one (the one we already have and you mentioned) gives the stats about the heaps. That is cheap / less intrusive but doesn't know anything about object inside those heaps. This CL is about the heap profiler which, when enabled via the run time flag, is more performance-intrusive but gives information about each individual allocation, where did they happen and which type they are. I don't think there is any code path which combines the two. In other words, without this you can either see that: - A given heap has X MB and # objects, but don't know when and where the object were allocated. - A given object was allocated at a given time, but you cannot know the type of that object. The type strings for the heap allocator are the missing link to get the full data for each allocation when heap profiling.
On 2016/02/29 19:16:32, Primiano (throttled til Mar 4) wrote: > On 2016/02/29 19:10:47, sof wrote: > > If this refers to CSSValue and Node, we have separate heaps for those that > would > > report overall allocations, so what's the added value beyond that to call a > > hook? > > Oh I see the question. Those are two slighly different reporting mechanisms: > one (the one we already have and you mentioned) gives the stats about the heaps. > That is cheap / less intrusive but doesn't know anything about object inside > those heaps. > This CL is about the heap profiler which, when enabled via the run time flag, is > more performance-intrusive but gives information about each individual > allocation, where did they happen and which type they are. > I don't think there is any code path which combines the two. In other words, > without this you can either see that: > - A given heap has X MB and # objects, but don't know when and where the object > were allocated. > - A given object was allocated at a given time, but you cannot know the type of > that object. > > The type strings for the heap allocator are the missing link to get the full > data for each allocation when heap profiling. Right, perhaps you'll get some increased fidelity of what's reported -- the exact timing of each Node/CSSValue allocation, rather than an overall liveness count&size at the next GC, but we run GCs fairly often. (And, for context, we're pushing our luck to get Oilpan to ship without any performance regressions in finite time, so adding something like this on the bump allocation path is making me nervous.)
hajimehoshi@: What's the performance result for this CL?
On 2016/02/29 23:32:03, haraken wrote: > hajimehoshi@: What's the performance result for this CL? Sorry but we have not had reliable evidence that this CL doesn't cause regression, I think there would be no regression though.
tasak@ confirmed that this CL won't introduce a regression (https://bugs.chromium.org/p/chromium/issues/detail?id=588123#c11). LGTM on my side. Let's wait for Sigbjorn's approval.
many thanks for testing with an official build, those blink_perf subtests look fine. blink_perf.shadow is heavier on allocations and may behave differently, but I'm now ok with trying. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:65: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(T); That this is HeapVector<> backing store isn't encoded in the type name; should it be? https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:77: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(T); Ditto, but for HeapHashTable<> backing store. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapTest.cpp:778: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(IntNode); On 2016/02/29 17:23:14, Primiano (throttled til Mar 4) wrote: > don't know how frequent this is, if is significant, 8 extra bytes for "IntNode" > won't be that bad :) (This is a unit test.)
Thank you! https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSValue.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSValue.h:47: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(CSSValue); On 2016/02/29 17:23:14, Primiano (throttled til Mar 4) wrote: > Considering that IIUC this is a pretty heavy user and that the string "CSSValue" > is just 9 bytes, should this be just > const char typeName[] = "CSSValue", so we get this also in official builds? > You don't really need the <T> expansion here, right? Right. It'd be best if we could get children class names of CSSValue and in this case we would need <T> expansion, but I'm fine with using the string literal so far. BTW, "blink::CSSValue" is the more accurate name? Done. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:187: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(Node); On 2016/02/29 17:23:14, Primiano (throttled til Mar 4) wrote: > ditto here Done. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:65: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(T); On 2016/03/03 08:37:30, sof wrote: > That this is HeapVector<> backing store isn't encoded in the type name; should > it be? I tried to pass HeapVectorBacking<T, VectorTraits<T>> to WTF_HEAP_PROFILER_TYPE_NAME but this didn't work because of the comma. Using braces also didn't work. I had to use ::WTF::getStringWithTypeName directly... Done. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:77: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(T); On 2016/03/03 08:37:30, sof wrote: > Ditto, but for HeapHashTable<> backing store. Done. https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapTest.cpp:778: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(IntNode); On 2016/03/03 08:37:30, sof wrote: > On 2016/02/29 17:23:14, Primiano (throttled til Mar 4) wrote: > > don't know how frequent this is, if is significant, 8 extra bytes for > "IntNode" > > won't be that bad :) > > (This is a unit test.) Since this is a unit test and we don't relatively have to care about performance, I prefer types which are checked by the compiler.
https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:65: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(T); On 2016/03/03 10:12:31, hajimehoshi wrote: > On 2016/03/03 08:37:30, sof wrote: > > That this is HeapVector<> backing store isn't encoded in the type name; should > > it be? > > I tried to pass HeapVectorBacking<T, VectorTraits<T>> to > WTF_HEAP_PROFILER_TYPE_NAME but this didn't work because of the comma. Using > braces also didn't work. I had to use ::WTF::getStringWithTypeName directly... > > Done. "#define COMMA ," or a local 'using/typedef' alias doesn't work out?
https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right): https://codereview.chromium.org/1721333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapAllocator.h:65: const char* typeName = WTF_HEAP_PROFILER_TYPE_NAME(T); On 2016/03/03 12:08:14, sof wrote: > On 2016/03/03 10:12:31, hajimehoshi wrote: > > On 2016/03/03 08:37:30, sof wrote: > > > That this is HeapVector<> backing store isn't encoded in the type name; > should > > > it be? > > > > I tried to pass HeapVectorBacking<T, VectorTraits<T>> to > > WTF_HEAP_PROFILER_TYPE_NAME but this didn't work because of the comma. Using > > braces also didn't work. I had to use ::WTF::getStringWithTypeName directly... > > > > Done. > > "#define COMMA ," or a local 'using/typedef' alias doesn't work out? Thanks. 'using' worked. Done.
lgtm
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1721333002/#ps60001 (title: "Use 'using'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721333002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721333002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1721333002/#ps80001 (title: "(rebasing)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721333002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721333002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1721333002/#ps100001 (title: "(rebasing)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721333002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721333002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 ========== to ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1761323002/ by treib@chromium.org. The reason for reverting is: Seems to have broken Google Chrome builds: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1766683002/ by sigbjornf@opera.com. The reason for reverting is: Broke official builds unfortunately, https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux....
Message was sent while issue was closed.
On 2016/03/04 10:35:04, sof wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/1766683002/ by mailto:sigbjornf@opera.com. > > The reason for reverting is: Broke official builds unfortunately, > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... Ah, thank you for noticing this. I'll go with 'COMMA' way.
Message was sent while issue was closed.
On 2016/03/04 10:44:28, hajimehoshi wrote: > On 2016/03/04 10:35:04, sof wrote: > > A revert of this CL (patchset #6 id:100001) has been created in > > https://codereview.chromium.org/1766683002/ by mailto:sigbjornf@opera.com. > > > > The reason for reverting is: Broke official builds unfortunately, > > > https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... > > Ah, thank you for noticing this. > > I'll go with 'COMMA' way. ok - treib@ got in a revert just before https://codereview.chromium.org/1766683002/ , hence cancelled.
Message was sent while issue was closed.
PTAL
Message was sent while issue was closed.
LGTM
Message was sent while issue was closed.
Description was changed from ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} ========== to ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} ==========
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1721333002/#ps120001 (title: "Use COMMA")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721333002/120001
Message was sent while issue was closed.
Description was changed from ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} ========== to ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} ========== to ========== Bug fix: Add BlinkGC allocation hooks to CSS, Node and vector/table backing BlinkGC allocation hooks for the memory infra were added at crrev.com/1676973002, but those were not enough. This CL adds missing hooks. BUG=588123 Committed: https://crrev.com/abd5af497f0a6f38c57f071a187b9484a0e3f306 Cr-Commit-Position: refs/heads/master@{#379252} Committed: https://crrev.com/d7c89d879b56ebad4e04d2bbc0452d35c1fa1f25 Cr-Commit-Position: refs/heads/master@{#379280} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d7c89d879b56ebad4e04d2bbc0452d35c1fa1f25 Cr-Commit-Position: refs/heads/master@{#379280} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
