Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1391933004:
[Tracing] Add hook to PartitionAlloc for heap profiling (Closed)
|
Created:
5 years, 2 months ago by Ruud van Asseldonk Modified:
5 years, 2 months ago Reviewers:
Primiano Tucci (use gerrit), jochen (gone - plz use gerrit), haraken, Jens Widell, tasak, bashi CC:
Mads Ager (chromium), blink-reviews, blink-reviews-api_chromium.org, blink-reviews-wtf_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, kouhei+heap_chromium.org, Mikhail, oilpan-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Tracing] Add hook to PartitionAlloc for heap profiling
This modifies PartitionAlloc to call a function on allocation and free,
which will be used for heap profiling. Care has been taken to impact the
fast path as little as possible. https://crbug.com/530502 suggests that
there is no significant performance impact.
This is part of the heap profiler in chrome://tracing.
BUG=524631
Committed: https://crrev.com/65d38472921ad574623410efdfbae840b76fd43e
Cr-Commit-Position: refs/heads/master@{#355304}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Address some primiano comments #
Total comments: 12
Patch Set 3 : Address primiano comments #
Total comments: 3
Patch Set 4 : Preemptively address primiano comments #
Total comments: 23
Patch Set 5 : Address review comments #
Total comments: 7
Patch Set 6 : Address haraken comments #Patch Set 7 : Do check both flags on realloc #
Total comments: 6
Patch Set 8 : Address jl comments #Messages
Total messages: 38 (11 generated)
ruuda@google.com changed reviewers: + haraken@chromium.org, primiano@chromium.org
primiano@chromium.org changed reviewers: - haraken@chromium.org
Many thanks for doing this. Really excited to see this coming. Temporary removing haraken. Rationale: you want to add OWNERS only after the initial internal review rounds. OWNERS tend to receive dozen reviews per day *, and you want to spam their inbox as less as possible. Other than that, TL;DR - I think we should revisit a bit the locking model - I think this is missing some big partition alloc entry points for the thread-safe partitions. - In general this looks ok % minor comments here and there. * If you want to see the actual scary numbers: https://codereview.chromium.org/user/haraken@chromium.org/stats/2015-q3 :) https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_context.cc (right): https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_context.cc:35: i != End(context.backtrace.frames); i++) You can make this simpler, shorter and more readable without using the End() template magic: for (size_t i = 0; i < arraysize(context.backtrace.frames); ++i) context.backtrace.frames[i] = nullptr Also "i" for a non-numeric type is a very bad naming choice. Furthermore this for loop would require extra braces around the *i = nullptr as the for spawns across multiple lines. https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_context.cc:38: for (auto* i = context.fields; i != End(context.fields); i++) ditto https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_context.h (right): https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_context.h:91: static AllocationContext Empty(); s/Empty/GetEmptyContext/ or just GetEmpty, as you prefer. Just empty is too misleading. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:17: AllocationRegister* gAllocationRegister; This is still chrome-land, this should be named g_allocation_register https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:20: void reportAllocation(void* address, size_t size) { I would probably rename these with a locked suffix and move the locking reponsibility to the caller. Rationale: some partition alloc partitions are single threaded and there is no need to take an extra lock there. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:20: void reportAllocation(void* address, size_t size) { Also, IIRC this should start with a capital R https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:31: void reportFree(void* address) { ditto s/report/Report/ https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:65: if (web_memory_dump_provider_->supportsHeapProfiling()) { Just to avoid forgetting this in future, here you will want also a check that args.level_of_detail == HUUUUUUGE https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:72: void WebMemoryDumpProviderAdapter::OnHeapProfilingEnabled(bool enabled) { I'd probably move the supportsHeapProfiling() on the top and early return: if (!web_memory_dump_provider_->supportsHeapProfiling()) return if (enabled) ... else ... this will remove one level of indentation and make your code below more readable https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:76: gAllocationRegister = new AllocationRegister(); these will make LeaskSanitizer barf. Either you need a suppression or you want to use LazyInstance (but that willl add an interlocked operation in the slow-path) https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:20: bool supportsHeapProfiling() override { return true; } isn't clang going to be unhappy about inline virtual methods? https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1069: if (UNLIKELY(partitionAllocHookAlloc && partitionAllocHookFree)) { just check one of them. You always set/reset them in couples, not worth spending time checking both. In the very worst case (if really you end up setting only one hook) you will crash and it will be very visible :) https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1101: // enabled. ditto https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:414: extern WTF_EXPORT void (*partitionAllocHookAlloc)(void* addr, size_t size); Hmm I think that if you split this into the actual fptr declaration and the storage definition it will be more clear and will reduce code duplication w.r.t. the .cc file also making them static fields of some class would remove the need of the awkward "extern" https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:708: if (UNLIKELY(partitionAllocHookAlloc != nullptr)) see my previous comment about locking. I think you might want to move this under the spinlock. https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:708: if (UNLIKELY(partitionAllocHookAlloc != nullptr)) Also, if you hook only partitionAllocGeneric you will miss the calls entering via partitionAlloc() and partitionFree() https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:727: if (UNLIKELY(partitionAllocHookFree != nullptr)) ditto
https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_context.cc (right): https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_context.cc:35: i != End(context.backtrace.frames); i++) On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > You can make this simpler, shorter and more readable without using the End() template magic: > > for (size_t i = 0; i < arraysize(context.backtrace.frames); ++i) > context.backtrace.frames[i] = nullptr > > Also "i" for a non-numeric type is a very bad naming choice. > Furthermore this for loop would require extra braces around the *i = nullptr as the for spawns across multiple lines. Done. https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_context.cc:38: for (auto* i = context.fields; i != End(context.fields); i++) On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > ditto Done. https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... File base/trace_event/memory_profiler_allocation_context.h (right): https://codereview.chromium.org/1391933004/diff/1/base/trace_event/memory_pro... base/trace_event/memory_profiler_allocation_context.h:91: static AllocationContext Empty(); On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > s/Empty/GetEmptyContext/ or just GetEmpty, as you prefer. Just empty is too misleading. As you wish. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:17: AllocationRegister* gAllocationRegister; On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > This is still chrome-land, this should be named g_allocation_register Done. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:20: void reportAllocation(void* address, size_t size) { On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > I would probably rename these with a locked suffix and move the locking reponsibility to the caller. > Rationale: some partition alloc partitions are single threaded and there is no need to take an extra lock there. No, the caller should not have to deal with locking (or reentrancy, though that is not an issue with PartitionAlloc yet). The reason is that when the memory dump manager calls |OnMemoryDump|, the register needs to be snapshotted and it must not be modified when it is being iterated. Furthermore, the partitions being single-threaded does not help at all, unless you keep a dedicated allocation register per partition. > Also, IIRC this should start with a capital R Done. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:31: void reportFree(void* address) { On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > ditto s/report/Report/ Done. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:72: void WebMemoryDumpProviderAdapter::OnHeapProfilingEnabled(bool enabled) { On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > I'd probably move the supportsHeapProfiling() on the top and early return: > if (!web_memory_dump_provider_->supportsHeapProfiling()) As you wish. https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:76: gAllocationRegister = new AllocationRegister(); On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > these will make LeaskSanitizer barf. > Either you need a suppression or you want to use LazyInstance (but that willl add an interlocked operation in the slow-path) The proper way of doing this is to delete when heap profiling is disabled (or in the dump provider destructor once it can live in Blink), but the hook checks are nobarrier loads so then potentially a hook can be called after the register has been deleted. So then the hooks would need to do a barriered check for the register and there will be |base::subtle| all over the place ... What do you think, just use |LazyInstance| then? https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.h:20: bool supportsHeapProfiling() override { return true; } On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > isn't clang going to be unhappy about inline virtual methods? I did not hear it complain. https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1069: if (UNLIKELY(partitionAllocHookAlloc && partitionAllocHookFree)) { On 2015/10/12 at 16:36:46, Primiano Tucci wrote: > just check one of them. You always set/reset them in couples, not worth spending time checking both. > In the very worst case (if really you end up setting only one hook) you will crash and it will be very visible :) My reasoning was that an allocation could be happening while the hooks are being set, but now that I think of it, the correct way to handle that would be to store the values in a local variable and call those. Though I think the compiler will only emit a single read here because the variables are not volatile. What do you think? https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:414: extern WTF_EXPORT void (*partitionAllocHookAlloc)(void* addr, size_t size); On 2015/10/12 at 16:36:46, Primiano Tucci wrote: > Hmm I think that if you split this into the actual fptr declaration and the storage definition it will be more clear and will reduce code duplication w.r.t. the .cc file > also making them static fields of some class would remove the need of the awkward "extern" Done. https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:708: if (UNLIKELY(partitionAllocHookAlloc != nullptr)) On 2015/10/12 at 16:36:46, Primiano Tucci wrote: > Also, if you hook only partitionAllocGeneric you will miss the calls entering via partitionAlloc() and partitionFree() Oooops, should have hooked |partitionBucketAlloc| and |partitionFreeWithPage| in the common code path. https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.h:727: if (UNLIKELY(partitionAllocHookFree != nullptr)) On 2015/10/12 at 16:36:46, Primiano Tucci wrote: > ditto Yes.
https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/1/content/child/web_memory_du... content/child/web_memory_dump_provider_adapter.cc:20: void reportAllocation(void* address, size_t size) { On 2015/10/13 10:42:18, Ruud van Asseldonk wrote: > On 2015/10/12 at 16:36:45, Primiano Tucci wrote: > > I would probably rename these with a locked suffix and move the locking > reponsibility to the caller. > > Rationale: some partition alloc partitions are single threaded and there is no > need to take an extra lock there. > > No, the caller should not have to deal with locking (or reentrancy, though that > is not an issue with PartitionAlloc yet). The reason is that when the memory > dump manager calls |OnMemoryDump|, the register needs to be snapshotted and it > must not be modified when it is being iterated. Furthermore, the partitions > being single-threaded does not help at all, unless you keep a dedicated > allocation register per partition. > > > Also, IIRC this should start with a capital R > > Done. Ah, you are definitely right. Ignore my comment about locking. https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1069: if (UNLIKELY(partitionAllocHookAlloc && partitionAllocHookFree)) { On 2015/10/13 10:42:18, Ruud van Asseldonk wrote: > On 2015/10/12 at 16:36:46, Primiano Tucci wrote: > > just check one of them. You always set/reset them in couples, not worth > spending time checking both. > > In the very worst case (if really you end up setting only one hook) you will > crash and it will be very visible :) > > My reasoning was that an allocation could be happening while the hooks are being > set, but now that I think of it, the correct way to handle that would be to > store the values in a local variable and call those. Though I think the compiler > will only emit a single read here because the variables are not volatile. What > do you think? After discussion offline, I think you want to just cache these two in a local variable (one each) and null check them before calling them https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:18: Lock* g_allocation_register_lock; After discussion offline, the lock should really be base::LazyInstance<base::Lock>::Leaky g_allocation_register_lock https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:28: g_allocation_register->Insert(address, size, context); null-check for the register before using it same below https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:78: if (g_allocation_register == nullptr) { take the lock here https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:414: class WTF_EXPORT PartitionHooks { nit: PartitionAllocHooks ? otherwise it seems that you are hooking partition-specific events https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:421: static AllocationHook* allocationHook; nit: m_allocationHook i believe https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:422: static FreeHook* freeHook; I think you can make the static private: and expose inline public methods which do the null-check (using local variables) + call. This would reduce the boilerplate elsewhere, like inline void maybeInvokeAllocationHook(void* address, size_t size) { AllocationHook* hook = m_allocationHook; if (hook) hook(address, size); }
https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:18: Lock* g_allocation_register_lock; On 2015/10/14 at 10:10:12, Primiano Tucci wrote: > After discussion offline, the lock should really be > base::LazyInstance<base::Lock>::Leaky g_allocation_register_lock Done. https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:28: g_allocation_register->Insert(address, size, context); On 2015/10/14 at 10:10:12, Primiano Tucci wrote: > null-check for the register before using it > same below Done. https://codereview.chromium.org/1391933004/diff/20001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:78: if (g_allocation_register == nullptr) { On 2015/10/14 at 10:10:12, Primiano Tucci wrote: > take the lock here Done. https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:414: class WTF_EXPORT PartitionHooks { On 2015/10/14 at 10:10:12, Primiano Tucci wrote: > nit: PartitionAllocHooks ? otherwise it seems that you are hooking partition-specific events Done. https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:421: static AllocationHook* allocationHook; On 2015/10/14 at 10:10:12, Primiano Tucci wrote: > nit: m_allocationHook i believe Done. https://codereview.chromium.org/1391933004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:422: static FreeHook* freeHook; On 2015/10/14 at 10:10:12, Primiano Tucci wrote: > I think you can make the static private: and expose inline public methods which do the null-check (using local variables) + call. > This would reduce the boilerplate elsewhere, like > > inline void maybeInvokeAllocationHook(void* address, size_t size) { > AllocationHook* hook = m_allocationHook; > if (hook) > hook(address, size); > } Great idea, done.
Description was changed from ========== [Tracing] Add hook to PartitionAlloc for heap profiling This modifies PartitionAlloc to call a function on allocation and free, which will be used for heap profiling. Care has been taken to impact the fast path as little as possible. https://crbug.com/530502 suggests that there is no significant performance impact. This is part of the heap profiler in chrome://tracing. BUG=524631 ========== to ========== [Tracing] Add hook to PartitionAlloc for heap profiling This modifies PartitionAlloc to call a function on allocation and free, which will be used for heap profiling. Care has been taken to impact the fast path as little as possible. https://crbug.com/530502 suggests that there is no significant performance impact. This is part of the heap profiler in chrome://tracing. BUG=524631 ==========
LGTM. At this point add haraken. Make sure he, or somebody from his team, doublechecks that you put the hooks in the right places. I'm not too familiar with PartAlloc internals. https://codereview.chromium.org/1391933004/diff/40001/content/child/web_memor... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/40001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:24: // Calling |GetContextSnapshot| is only valid when |capture_enabled| is true. maybe add a comment saying that you do the GetEmpty() to deal with the nobarrier cases https://codereview.chromium.org/1391933004/diff/40001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:26: AllocationContextTracker::capture_enabled() We had a chat offline, I think that you can delete this and just rely on GetContextSnapshot to return you a valid context. It should never happen that you get this call when the TLS in the COntextTracker is not initialized, and if that happens it's a bug https://codereview.chromium.org/1391933004/diff/40001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:85: AutoLock guard(g_allocation_register_lock.Get()); either put brances around here or move the lock up (before if(enabled) https://codereview.chromium.org/1391933004/diff/60001/content/child/web_memor... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/60001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:32: AutoLock guard(g_allocation_register_lock.Get()); nit: here and elsewhere s/guard/lock/ The name is really irrelevant but I've always seen lock in all the chrome codebase. https://codereview.chromium.org/1391933004/diff/60001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:79: AutoLock guard(g_allocation_register_lock.Get()); Can you restrict the scope of the guard only to the if(!g_allocation_register) g_allocation_register = new. ... section below. Rationale: - You don't really need the lock to call the onHeapprofilingEnabled/DIsabled - As you don't know here what onHeapprofilingEnabled/Disabled are going to to it's safer to not take the lock to avoid reentrancy - The lock going out of scope before onHeapprofilingEnabled/Disabled will enforce the barrier that will give you the guarantee that: if an allocation happens in blink soon after calling onHeapprofilingEnabled the register will be initialized. Without that guarantee, the if (g_allocation_register) you have above don't make any sense. Rationale: id you don't enclose the g_allocation_register = new AllocationRegister() in a locked scope, it might happen that, due to out-of-order execution, you see the g_allocation_register being != null but its state it still non consistent (because the writes of the AllocationRegister ctors have not had effect yet). in essence, you really want to do the if + initialization of AllocationRegister in its scope. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:424: AllocationHook* hook = m_allocationHook; probably a bit more readable if you s/hook/allocationHook/ same below https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebMemoryDumpProvider.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMemoryDumpProvider.h:43: // detect for which dump provider it should do the bookkeeping. s/for which dump provider it should do the bookkeeping/whether the current dump provider support heap profiling/ https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMemoryDumpProvider.h:48: // pointers). This is a workaround for the fact that Blink cannot use Remove the "Ths is a workaround" part. You already explained that above, should be enough.
haraken@chromium.org changed reviewers: + bashi@chromium.org, haraken@chromium.org, tasak@google.com
Thanks for working on this! https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1066: PartitionAllocHooks::maybeInvokeReallocationHook(ptr, ptr, newSize); You don't need to call this because no reallocation happens here, right? https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:422: static void maybeInvokeAllocationHook(void* address, size_t size) I'd rename maybeInvokeAllocationHook() to allocationHook(), simply. Or allocationHookIfEnabled(). https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:436: static void maybeInvokeReallocationHook(void* oldAddress, void* newAddress, size_t size) You won't need this method if you insert the allocation/free hooks to partitionAlloc/partitionFree. See my comment below. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:657: PartitionAllocHooks::maybeInvokeAllocationHook(ret, size); You need to insert the allocation hook to partitionDirectMap() as well. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:657: PartitionAllocHooks::maybeInvokeAllocationHook(ret, size); And you need to insert the allocation hook to partitionPageAllocAndFillFreelist() as well... https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:692: PartitionAllocHooks::maybeInvokeFreeHook(ptr); You need to insert the free hook to partitionDirectUnmap() as well. OK, now I realized that it's not a good idea to insert the hooks to the places where we actually allocate/free objects. Alternatively, we can insert the hooks to PartitionAlloc's allocation/free APIs. In other words, you can insert the hooks to partitionAlloc/partitionFree/partitionAllocGeneric/partitionFreeGeneric. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebMemoryDumpProvider.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMemoryDumpProvider.h:43: // detect for which dump provider it should do the bookkeeping. Add a TODO and mention that this should be removed once the dependency from wtf/ to base/ is added.
https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:425: if (UNLIKELY(hook != nullptr)) nit: UNLIKELY(hook). We don't use ptr != nullptr in blink (not sure this is still a preferred style though) BTW, do you have any idea whether this branch would have negative impact on performance when tracing is disabled? My guess is that this doesn't cause performance regression, but not sure.
On 2015/10/20 00:33:49, bashi1 wrote: > https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): > > https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/PartitionAlloc.h:425: if (UNLIKELY(hook != > nullptr)) > nit: UNLIKELY(hook). > > We don't use ptr != nullptr in blink (not sure this is still a preferred style > though) > > BTW, do you have any idea whether this branch would have negative impact on > performance when tracing is disabled? My guess is that this doesn't cause > performance regression, but not sure. The performance concern was addressed in https://code.google.com/p/chromium/issues/detail?id=530502.
On 2015/10/20 00:35:42, haraken wrote: > On 2015/10/20 00:33:49, bashi1 wrote: > > > https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): > > > > > https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/wtf/PartitionAlloc.h:425: if (UNLIKELY(hook != > > nullptr)) > > nit: UNLIKELY(hook). > > > > We don't use ptr != nullptr in blink (not sure this is still a preferred style > > though) > > > > BTW, do you have any idea whether this branch would have negative impact on > > performance when tracing is disabled? My guess is that this doesn't cause > > performance regression, but not sure. > > The performance concern was addressed in > https://code.google.com/p/chromium/issues/detail?id=530502. Ah, thanks for the link. I missed it :(
https://codereview.chromium.org/1391933004/diff/60001/content/child/web_memor... File content/child/web_memory_dump_provider_adapter.cc (right): https://codereview.chromium.org/1391933004/diff/60001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:32: AutoLock guard(g_allocation_register_lock.Get()); On 2015/10/19 at 21:32:03, Primiano Tucci wrote: > nit: here and elsewhere s/guard/lock/ > The name is really irrelevant but I've always seen lock in all the chrome codebase. Done. https://codereview.chromium.org/1391933004/diff/60001/content/child/web_memor... content/child/web_memory_dump_provider_adapter.cc:79: AutoLock guard(g_allocation_register_lock.Get()); On 2015/10/19 at 21:32:03, Primiano Tucci wrote: > Can you restrict the scope of the guard only to the if(!g_allocation_register) g_allocation_register = new. ... section below. > Rationale: > - You don't really need the lock to call the onHeapprofilingEnabled/DIsabled > - As you don't know here what onHeapprofilingEnabled/Disabled are going to to it's safer to not take the lock to avoid reentrancy > - The lock going out of scope before onHeapprofilingEnabled/Disabled will enforce the barrier that will give you the guarantee that: if an allocation happens in blink soon after calling onHeapprofilingEnabled the register will be initialized. > Without that guarantee, the if (g_allocation_register) you have above don't make any sense. > Rationale: id you don't enclose the g_allocation_register = new AllocationRegister() in a locked scope, it might happen that, due to out-of-order execution, you see the g_allocation_register being != null but its state it still non consistent (because the writes of the AllocationRegister ctors have not had effect yet). > > in essence, you really want to do the if + initialization of AllocationRegister in its scope. Done, changed to protect |g_allocation_register| access only. We discussed this before, justification for locking at the top was more aesthetics than necessity. Performance is irrelevant here because the code is cold. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1066: PartitionAllocHooks::maybeInvokeReallocationHook(ptr, ptr, newSize); On 2015/10/20 at 00:14:19, haraken wrote: > You don't need to call this because no reallocation happens here, right? This updates the size (even though the address does not change). It also replaces the context that was captured at allocation time with the current context. Attributing all of the memory to the context in which the reallocation happened is not entirely correct, but attributing it to the context of first allocation is not right either. I think treating a realloc as free+alloc is the least bad thing to do here. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:422: static void maybeInvokeAllocationHook(void* address, size_t size) On 2015/10/20 at 00:14:19, haraken wrote: > I'd rename maybeInvokeAllocationHook() to allocationHook(), simply. Or allocationHookIfEnabled(). Done. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:424: AllocationHook* hook = m_allocationHook; On 2015/10/19 at 21:32:03, Primiano Tucci wrote: > probably a bit more readable if you s/hook/allocationHook/ > same below Done. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:425: if (UNLIKELY(hook != nullptr)) On 2015/10/20 at 00:33:49, bashi1 wrote: > nit: UNLIKELY(hook). > > We don't use ptr != nullptr in blink (not sure this is still a preferred style though) This is because |UNLIKELY(ptr)| expands to |__builtin_expect(ptr, 0)| which makes the compiler complain about initializing a parameter of type |long| with a pointer. I could replace it with |UNLIKELY(!!ptr)| if you like. > BTW, do you have any idea whether this branch would have negative impact on performance when tracing is disabled? My guess is that this doesn't cause performance regression, but not sure. I found no significant performance impact, see https://crbug.com/530502. I'll wait a few days before landing anything that depends on this CL, so if a regression is detected after all this CL can be reverted cleanly. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:436: static void maybeInvokeReallocationHook(void* oldAddress, void* newAddress, size_t size) On 2015/10/20 at 00:14:19, haraken wrote: > You won't need this method if you insert the allocation/free hooks to partitionAlloc/partitionFree. See my comment below. Wouldn't I still need it for |partitionReallocGeneric|? https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:692: PartitionAllocHooks::maybeInvokeFreeHook(ptr); On 2015/10/20 at 00:14:19, haraken wrote: > You need to insert the free hook to partitionDirectUnmap() as well. > > OK, now I realized that it's not a good idea to insert the hooks to the places where we actually allocate/free objects. Alternatively, we can insert the hooks to PartitionAlloc's allocation/free APIs. In other words, you can insert the hooks to partitionAlloc/partitionFree/partitionAllocGeneric/partitionFreeGeneric. That makes sense. Fixed. What about |partitionReallocGeneric|? https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebMemoryDumpProvider.h (right): https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMemoryDumpProvider.h:43: // detect for which dump provider it should do the bookkeeping. On 2015/10/19 at 21:32:04, Primiano Tucci wrote: > s/for which dump provider it should do the bookkeeping/whether the current dump provider support heap profiling/ Done, though that does miss the fact that the glue layer can do bookkeeping for only one dump provider. > Add a TODO and mention that this should be removed once the dependency from wtf/ to base/ is added. Done. https://codereview.chromium.org/1391933004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMemoryDumpProvider.h:48: // pointers). This is a workaround for the fact that Blink cannot use On 2015/10/19 at 21:32:03, Primiano Tucci wrote: > Remove the "Ths is a workaround" part. You already explained that above, should be enough. Done.
https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1093: PartitionAllocHooks::reallocHookIfEnabled(ptr, ret, newSize); I don't think this is needed since the above partitionAllocGeneric and partitionFreeGeneric already called the hook functions. https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:417: typedef void FreeHook(void* address); typedef => using https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:441: if (UNLIKELY(allocationHook && freeHook)) { if (UNLIKELY(allocationHook)) { ASSERT(freeHook); ...; } will be a bit faster.
https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.cpp:1093: PartitionAllocHooks::reallocHookIfEnabled(ptr, ret, newSize); On 2015/10/20 at 12:42:00, haraken wrote: > I don't think this is needed since the above partitionAllocGeneric and partitionFreeGeneric already called the hook functions. You are right, fixed. https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:417: typedef void FreeHook(void* address); On 2015/10/20 at 12:42:00, haraken wrote: > typedef => using Curiously, a using causes compile errors on MSVC but a typedef does not. I reported this at https://connect.microsoft.com/VisualStudio/feedback/details/1895253. https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:441: if (UNLIKELY(allocationHook && freeHook)) { On 2015/10/20 at 12:42:00, haraken wrote: > if (UNLIKELY(allocationHook)) { > ASSERT(freeHook); > ...; > } > > will be a bit faster. Done. Why will it be faster though? I expect that the load of |m_allocationHook| and |m_freeHook| will be the bottleneck here, and if they are on the same cache line loading one or both shouldn't make that much of a difference, should it? The reason for checking both was that the hooks are not set together atomically (because a lock would be expensive), so in the extremely unlikely event that the hooks are being installed while a reallocation is happening that could call a null pointer. So there is a tiny chance that the assert fails, but only when heap profiling is enabled so it will not impact regular users.
https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PartitionAlloc.h:441: if (UNLIKELY(allocationHook && freeHook)) { On 2015/10/20 13:22:16, Ruud van Asseldonk wrote: > On 2015/10/20 at 12:42:00, haraken wrote: > > if (UNLIKELY(allocationHook)) { > > ASSERT(freeHook); > > ...; > > } > > > > will be a bit faster. > > Done. Why will it be faster though? I expect that the load of |m_allocationHook| > and |m_freeHook| will be the bottleneck here, and if they are on the same cache > line loading one or both shouldn't make that much of a difference, should it? > > The reason for checking both was that the hooks are not set together atomically > (because a lock would be expensive), so in the extremely unlikely event that the > hooks are being installed while a reallocation is happening that could call a > null pointer. So there is a tiny chance that the assert fails, but only when > heap profiling is enabled so it will not impact regular users. Yeah, you're right. Let's check both. BTW, what happens if: 1) Run chrome normally. Chrome allocates an object X. 2) Enable memory-infra. 3) Chrome frees the object X. At the point of 3), won't the system get confused because there is no record for 1)?
On 2015/10/20 at 13:25:01, haraken wrote: > Yeah, you're right. Let's check both. Done. > BTW, what happens if: > > 1) Run chrome normally. Chrome allocates an object X. > 2) Enable memory-infra. > 3) Chrome frees the object X. > > At the point of 3), won't the system get confused because there is no record for 1)? At point 3, the hook will call |AllocationRegister::Remove|, which is designed to deal with this case. To avoid this scenario and capture as much allocations as possible, heap profiling can be enabled with the --enable-heap-profiling switch, because the switches are available earlier than the trace config.
On 2015/10/20 13:31:29, Ruud van Asseldonk wrote: > On 2015/10/20 at 13:25:01, haraken wrote: > > Yeah, you're right. Let's check both. > > Done. > > > BTW, what happens if: > > > > 1) Run chrome normally. Chrome allocates an object X. > > 2) Enable memory-infra. > > 3) Chrome frees the object X. > > > > At the point of 3), won't the system get confused because there is no record > for 1)? > > At point 3, the hook will call |AllocationRegister::Remove|, which is designed > to deal with this case. To avoid this scenario and capture as much allocations > as possible, heap profiling can be enabled with the --enable-heap-profiling > switch, because the switches are available earlier than the trace config. Sounds great! LGTM
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1391933004/#ps120001 (title: "Do check both flags on realloc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391933004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391933004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ruuda@google.com changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in content/child and WebKit/public/platform
jochen@chromium.org changed reviewers: + jl@opera.com
Jens, can you have a look at this please?
https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PartitionAlloc.h:717: PartitionAllocHooks::freeHookIfEnabled(ptr); In debug, |ptr| here doesn't match what we called the allocation hook with. The value passed to the allocation hook is what was returned to the caller (which is what was actually allocated plus kCookieSize) whereas the value we pass to the free hook here is what was actually allocated (sans that kCookieSize adjustment.) I don't know if this is important or not. https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PartitionAlloc.h:747: PartitionAllocHooks::allocationHookIfEnabled(ret, size); Note that |size| here isn't necessarily the size of the returned memory block, which may be larger. Don't know if we consider this important to adjust for. If we intend to record the actual size the caller requested, we might want undo the adjustment made by partitionCookieSizeAdjustAdd() above. Again, I don't know if this is something that's important enough to care about. https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PartitionAlloc.h:773: PartitionAllocHooks::freeHookIfEnabled(ptr); Same kCookieSize adjustment issue here.
https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PartitionAlloc.h:717: PartitionAllocHooks::freeHookIfEnabled(ptr); On 2015/10/21 13:56:51, Jens Widell wrote: > In debug, |ptr| here doesn't match what we called the allocation hook with. The > value passed to the allocation hook is what was returned to the caller (which is > what was actually allocated plus kCookieSize) whereas the value we pass to the > free hook here is what was actually allocated (sans that kCookieSize > adjustment.) > > I don't know if this is important or not. The pointers in the allocation hook and free hook definitely need to match. It should be fixed now, can you please verify? https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PartitionAlloc.h:747: PartitionAllocHooks::allocationHookIfEnabled(ret, size); On 2015/10/21 13:56:51, Jens Widell wrote: > Note that |size| here isn't necessarily the size of the returned memory block, > which may be larger. Don't know if we consider this important to adjust for. > > If we intend to record the actual size the caller requested, we might want undo > the adjustment made by partitionCookieSizeAdjustAdd() above. Again, I don't know > if this is something that's important enough to care about. Good point. I want to record the requested size. If the bucket size happens to be larger, the extra memory would be allocator overhead (along with the metadata pages), and the cookie is overhead as well. Fixed it now to report the requested size. On a side note, the way in which |partitionDumpPageStats| treats the requested/allocated distinction is not ideal if we want to combine that information with the numbers reported by the heap profiler. See https://crbug.com/527121. https://codereview.chromium.org/1391933004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/PartitionAlloc.h:773: PartitionAllocHooks::freeHookIfEnabled(ptr); On 2015/10/21 13:56:51, Jens Widell wrote: > Same kCookieSize adjustment issue here. Fixed.
PartitionAlloc changes LGTM.
lgtm
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1391933004/#ps140001 (title: "Address jl comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1391933004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1391933004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/65d38472921ad574623410efdfbae840b76fd43e Cr-Commit-Position: refs/heads/master@{#355304} |
