Chromium Code Reviews
|
|
DescriptionAdding thread id to PartitionAlloc memory dumps for single process mode and bug fix.
There are multiple blink platform impl in the process when run using
single process mode or when the renderer has workers with blink. So,
the path of the dump is appended with thread id to differentiate the
dumps. Also, the windows implementation of vprintf does not support %zu.
So, %u is used here and casted.
BUG=501912
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197467
Patch Set 1 #
Total comments: 2
Patch Set 2 : Build fix. #Messages
Total messages: 23 (9 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
primiano@chromium.org changed reviewers: + haraken@chromium.org
LGTM, +haraken
LGTM with a comment https://codereview.chromium.org/1189173003/diff/1/Source/platform/PartitionAl... File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1189173003/diff/1/Source/platform/PartitionAl... Source/platform/PartitionAllocMemoryDumpProvider.cpp:40: dumpName = String::format("partition_alloc/thread_%lu/%s/directMap_%lu", static_cast<unsigned long>(WTF::currentThread()), partitionName, ++m_uid); Instead of calling WTF::currentThread() here, can we pass in the thread id to partitionsDumpBucketStats (or include the thread id in partitionName)? We want to keep PartitionAlloc an isolated library as much as possible (i.e., reduce the dependency on other classes). We have a plan to port PartitionAlloc to Chromium.
Patchset #2 (id:20001) has been deleted
ssid@chromium.org changed reviewers: + cevans@chromium.org
+cevans PTAL https://codereview.chromium.org/1189173003/diff/1/Source/platform/PartitionAl... File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1189173003/diff/1/Source/platform/PartitionAl... Source/platform/PartitionAllocMemoryDumpProvider.cpp:40: dumpName = String::format("partition_alloc/thread_%lu/%s/directMap_%lu", static_cast<unsigned long>(WTF::currentThread()), partitionName, ++m_uid); On 2015/06/18 17:55:30, haraken wrote: > > Instead of calling WTF::currentThread() here, can we pass in the thread id to > partitionsDumpBucketStats (or include the thread id in partitionName)? > > We want to keep PartitionAlloc an isolated library as much as possible (i.e., > reduce the dependency on other classes). We have a plan to port PartitionAlloc > to Chromium. > This module depends on Partitions.h, and Partitions.cpp already works with threading. I have added the id as uint64_t since some platforms define ThreadIdentifier as unsigned long int and that is the biggest type. Passing around a String means PartitionAlloc should depend on WTF::String now since it has to use String::format. So I just passed the id and converting it to string at the provider.
primiano@chromium.org changed reviewers: - cevans@chromium.org
-cevans he's more than welcome to comment but I don't think we should bug him for stuff like just renaming strings ;-) As long as haraken@ is OK, sgtm. https://codereview.chromium.org/1189173003/diff/40001/Source/platform/Partiti... File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1189173003/diff/40001/Source/platform/Partiti... Source/platform/PartitionAllocMemoryDumpProvider.cpp:41: dumpName = String::format("partition_alloc/thread_%lu/%s/bucket_%u", threadId, partitionName, static_cast<unsigned>(memoryStats->bucketSlotSize)); usnigned long + %lu plz, or this will get into compiler warnings
Added cevans@for owner review. He is the owner of WTF. https://codereview.chromium.org/1189173003/diff/40001/Source/platform/Partiti... File Source/platform/PartitionAllocMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1189173003/diff/40001/Source/platform/Partiti... Source/platform/PartitionAllocMemoryDumpProvider.cpp:41: dumpName = String::format("partition_alloc/thread_%lu/%s/bucket_%u", threadId, partitionName, static_cast<unsigned>(memoryStats->bucketSlotSize)); On 2015/06/18 21:26:35, Primiano Tucci wrote: > usnigned long + %lu plz, or this will get into compiler warnings The value is uint32_t. That's why made it unsigned and not long.
cevans@chromium.org changed reviewers: + cevans@chromium.org
https://codereview.chromium.org/1189173003/diff/40001/Source/wtf/Partitions.cpp File Source/wtf/Partitions.cpp (right): https://codereview.chromium.org/1189173003/diff/40001/Source/wtf/Partitions.c... Source/wtf/Partitions.cpp:112: uint64_t threadId = static_cast<uint64_t>(currentThread()); Is there any reason we need to query this here and then pass it all the way into PartitionAlloc and back out again? Would it be possible to do the currentThread() call in PartitionAllocMemoryDumpProvider instead?
> Is there any reason we need to query this here and then pass it all the way into > PartitionAlloc and back out again? > > Would it be possible to do the currentThread() call in > PartitionAllocMemoryDumpProvider instead? Please see haraken@ comment in this thread.
On 2015/06/18 21:48:42, ssid wrote:
> > Is there any reason we need to query this here and then pass it all the way
> into
> > PartitionAlloc and back out again?
> >
> > Would it be possible to do the currentThread() call in
> > PartitionAllocMemoryDumpProvider instead?
>
> Please see haraken@ comment in this thread.
I think I prefer the original patch. The call to currentThread() was in
PartitionAllocMemoryDumpProvider.cpp, which is not a part of PartitionAlloc. It
looks like the original patch did not touch PartitionAlloc.{h,cpp} at all.
> I think I prefer the original patch. The call to currentThread() was in
> PartitionAllocMemoryDumpProvider.cpp, which is not a part of PartitionAlloc.
It
> looks like the original patch did not touch PartitionAlloc.{h,cpp} at all.
ssid: see? this happens when you add two owners to a CL :P
Joking aside, I agree with Chris, since we have to have the GetCurrentThread()
call in once place, it's probably cleaner and less intrusive to have that in the
dumper rather than all this plumbing.
haraken@ WDYT?
On 2015/06/18 21:58:51, Primiano Tucci wrote:
> > I think I prefer the original patch. The call to currentThread() was in
> > PartitionAllocMemoryDumpProvider.cpp, which is not a part of PartitionAlloc.
> It
> > looks like the original patch did not touch PartitionAlloc.{h,cpp} at all.
>
> ssid: see? this happens when you add two owners to a CL :P
> Joking aside, I agree with Chris, since we have to have the GetCurrentThread()
> call in once place, it's probably cleaner and less intrusive to have that in
the
> dumper rather than all this plumbing.
>
> haraken@ WDYT?
I don't have a strong opinion. Let's go with the suggestion of cevans :)
On 2015/06/18 21:54:21, Chris Evans wrote:
> On 2015/06/18 21:48:42, ssid wrote:
> > > Is there any reason we need to query this here and then pass it all the
way
> > into
> > > PartitionAlloc and back out again?
> > >
> > > Would it be possible to do the currentThread() call in
> > > PartitionAllocMemoryDumpProvider instead?
> >
> > Please see haraken@ comment in this thread.
>
> I think I prefer the original patch. The call to currentThread() was in
> PartitionAllocMemoryDumpProvider.cpp, which is not a part of PartitionAlloc.
It
> looks like the original patch did not touch PartitionAlloc.{h,cpp} at all.
Ah, sorry; I was thinking that it was in PartitoinAlloc.cpp.
Sorry, the patch set 1 LGTM definitely.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by ssid@chromium.org
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/1189173003/#ps60001 (title: "Build fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189173003/60001
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197467 |
