Chromium Code Reviews
|
|
Description[tracing] Expose AddOwnershipEdge and CreateAllocatorDump with guid to blink. (blink-side).
This CL exposes AddOwnershipEdge api and CreateAllocatorDump overload
with guid to blink. The code in chromium already had the API, now it is
required in blink for the BlinkGC dumper. The plumbing code just stores
the guid as given to it. The logic for hashing the string still remains
in base. Chromium side patch : crrev.com/1156863004
BUG=480500, 492102
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197398
Patch Set 1 #Patch Set 2 : Nits. #Patch Set 3 : A blank line. #
Total comments: 4
Patch Set 4 : Adding guid getter and create dump with guid. #Patch Set 5 : Removing constructor with args. #Patch Set 6 : non const struct for assignment. #
Total comments: 8
Patch Set 7 : Constructor change. #Patch Set 8 : Adding asserts. #
Total comments: 7
Patch Set 9 : Making guid private. #Patch Set 10 : Making guid virtual class, #Patch Set 11 : Fixing comments. #
Total comments: 1
Patch Set 12 : Using guid as uint64. #Patch Set 13 : nit. #
Total comments: 6
Patch Set 14 : Moved guid() to impl. #
Total comments: 3
Patch Set 15 : Removed a redundant typedef. #Patch Set 16 : Rebase #
Messages
Total messages: 28 (6 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL.
Patchset #2 (id:20001) has been deleted
This looks good (% some comment) but at this point this should expose also crrev.com/1157433005 (CreateAllDump with guid argument) and the guid() getter in WMAD https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebMemo... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebMemo... public/platform/WebMemoryAllocatorDump.h:17: Int64Type, Technically this is a uint64 not int64, so either you call this Uint64Type or just IntType (I'd prefer the latter) https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebProc... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebProc... public/platform/WebProcessMemoryDump.h:52: virtual void AddOwnershipEdge(WebMemoryAllocatorDumpGuid source, WebMemoryAllocatorDumpGuid target, int importance) const WMADG& source (and target)
Made changes PTAL. https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebMemo... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebMemo... public/platform/WebMemoryAllocatorDump.h:17: Int64Type, On 2015/05/29 11:24:53, Primiano Tucci wrote: > Technically this is a uint64 not int64, so either you call this Uint64Type or > just IntType (I'd prefer the latter) Done. https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebProc... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/50003/public/platform/WebProc... public/platform/WebProcessMemoryDump.h:52: virtual void AddOwnershipEdge(WebMemoryAllocatorDumpGuid source, WebMemoryAllocatorDumpGuid target, int importance) On 2015/05/29 11:24:53, Primiano Tucci wrote: > const WMADG& source (and target) Done.
https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:22: uint64_t guidInt; is this the right order in blink? Shouldn't these go last? Please check https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:46: : m_guid(WebMemoryAllocatorDumpGuid(0)) I am not sure I understand this. Why the guid is just not passed as an arg of the ctor? It seems odd that here you initialize it with an invalid value, and on the other side you set the protected. Or if you prefer you can just not store anything here and make the getter virtual, and do the conversion in content/ https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebPro... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:29: virtual WebMemoryAllocatorDump* createMemoryAllocatorDump(const WebString& absoluteName) Well at this point you should swap the order of the two and have the overload with the GUID here, and the reduced one below https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:60: virtual void AddOwnershipEdge(const WebMemoryAllocatorDumpGuid source, const WebMemoryAllocatorDumpGuid target, int importance) nit, missing & (const WMADG& source). Same below
Made changes. https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:22: uint64_t guidInt; On 2015/06/02 12:38:52, Primiano Tucci wrote: > is this the right order in blink? Shouldn't these go last? Please check There is one place where it is in this order, thought that was right. https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:46: : m_guid(WebMemoryAllocatorDumpGuid(0)) On 2015/06/02 12:38:52, Primiano Tucci wrote: > I am not sure I understand this. Why the guid is just not passed as an arg of > the ctor? > It seems odd that here you initialize it with an invalid value, and on the other > side you set the protected. > Or if you prefer you can just not store anything here and make the getter > virtual, and do the conversion in content/ Done. https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebPro... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:29: virtual WebMemoryAllocatorDump* createMemoryAllocatorDump(const WebString& absoluteName) On 2015/06/02 12:38:52, Primiano Tucci wrote: > Well at this point you should swap the order of the two and have the overload > with the GUID here, and the reduced one below Done. https://codereview.chromium.org/1159923006/diff/110001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:60: virtual void AddOwnershipEdge(const WebMemoryAllocatorDumpGuid source, const WebMemoryAllocatorDumpGuid target, int importance) On 2015/06/02 12:38:52, Primiano Tucci wrote: > nit, missing & (const WMADG& source). > Same below Done.
https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:45: // TODO(ssid): This constructor should be removed once the usage is I don't think you need neither of these constructors. The WMAD is constructed via createMemALlocatorDump, which passes the guid to the chromium layer. https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:73: // graph APIs (see TODO_method_name) to express retention / suballocation or s/TODO_method_name/AddOwnershipEdge/ https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:80: BLINK_ASSERT(m_guid.guidInt); This should be just a virtual and the chrome layer should know how to compute the guid. https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:85: WebMemoryAllocatorDumpGuid m_guid; You don't need this. Just pass the guid you get in createMemoryAllocatorDump to the Web...Impl on the chrome side, and store it there. https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebPro... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:31: return nullptr; Can you plz add BLINK_ASSERT_NOT_REACHED(); here and below for uniformity.
Patchset #9 (id:170001) has been deleted
As discussed offline, made changes. https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:73: // graph APIs (see TODO_method_name) to express retention / suballocation or On 2015/06/08 09:39:16, Primiano Tucci wrote: > s/TODO_method_name/AddOwnershipEdge/ Done. https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebPro... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/150001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:31: return nullptr; On 2015/06/08 09:39:16, Primiano Tucci wrote: > Can you plz add BLINK_ASSERT_NOT_REACHED(); here and below for uniformity. Done.
PTAL.
LGTM with some minor comments. Please add a link to the original CL of mine that introduced the api and to the corresponding change in chromium and add an OWNER https://codereview.chromium.org/1159923006/diff/230001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/230001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:26: private: I think this should be protected for sake of clarity. https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t WebMemoryAllocatorDumpGuid; using WebMemoryAllocatorDumpGuid = uint64_t? https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebPro... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:31: virtual WebMemoryAllocatorDump* createMemoryAllocatorDump(const WebString& absoluteName, const WebMemoryAllocatorDumpGuid& guid) No need of the const& on the guid anymore, this is now a pod https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:73: virtual void AddOwnershipEdge(const WebMemoryAllocatorDumpGuid& source, const WebMemoryAllocatorDumpGuid& target, int importance) ditto here and below
LGTM with some minor comments. Please add a link to the original CL of mine that introduced the api and to the corresponding change in chromium and add an OWNER
On 2015/06/09 20:51:35, Primiano Tucci wrote: > LGTM with some minor comments. > Please add a link to the original CL of mine that introduced the api and to the > corresponding change in chromium and add an OWNER Something wrong happened with rietveld. The message was sent twice and there was an extra comment from a previous patchset (foget the thing about protected). The other comments are valid though.
On 2015/06/09 20:51:35, Primiano Tucci wrote: > LGTM with some minor comments. > Please add a link to the original CL of mine that introduced the api and to the > corresponding change in chromium and add an OWNER Something wrong happened with rietveld. The message was sent twice and there was an extra comment from a previous patchset (foget the thing about protected). The other comments are valid though.
I made the guid() as virtual and moved the member to impl. PTAL. https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t WebMemoryAllocatorDumpGuid; On 2015/06/09 20:51:34, Primiano Tucci wrote: > using WebMemoryAllocatorDumpGuid = uint64_t? Webkit has a lot of files which use typedef, for example "typedef uint64_t TraceEventHandle". If you still want to change it, should i change at all 3 places where i did typedef? https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebPro... File public/platform/WebProcessMemoryDump.h (right): https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:31: virtual WebMemoryAllocatorDump* createMemoryAllocatorDump(const WebString& absoluteName, const WebMemoryAllocatorDumpGuid& guid) On 2015/06/09 20:51:34, Primiano Tucci wrote: > No need of the const& on the guid anymore, this is now a pod Done. https://codereview.chromium.org/1159923006/diff/270001/public/platform/WebPro... public/platform/WebProcessMemoryDump.h:73: virtual void AddOwnershipEdge(const WebMemoryAllocatorDumpGuid& source, const WebMemoryAllocatorDumpGuid& target, int importance) On 2015/06/09 20:51:34, Primiano Tucci wrote: > ditto here and below Done.
OK, add owners to this. Please don't land it today because I'm in the middle of a multisided patch with memory infra
ssid@chromium.org changed reviewers: + rbyers@chromium.org
PTAL.
public/ rubber stamp LGTM https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t WebMemoryAllocatorDumpGuid; this is redundant with the definition in the other file, right?
comment inline. https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t WebMemoryAllocatorDumpGuid; On 2015/06/11 14:12:16, Rick Byers wrote: > this is redundant with the definition in the other file, right? I added this here to avoid an extra #include. I did the same in Platform.h too. Do you think i should include the file here?
https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... File public/platform/WebMemoryAllocatorDump.h (right): https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t WebMemoryAllocatorDumpGuid; On 2015/06/11 14:12:16, Rick Byers wrote: > this is redundant with the definition in the other file, right? Sorry, I misunderstood the comment. So, this is the file where this typedef should be present. But the Platform.h has an extra typedef to avoid an include. This is a pattern in the other typedefs there, eg. TraceEventAPIAtomicWord and TraceEventHandle. That is why I added a redundant typedef there.
On 2015/06/11 14:49:06, ssid wrote: > https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... > File public/platform/WebMemoryAllocatorDump.h (right): > > https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... > public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t > WebMemoryAllocatorDumpGuid; > On 2015/06/11 14:12:16, Rick Byers wrote: > > this is redundant with the definition in the other file, right? > > Sorry, I misunderstood the comment. > > So, this is the file where this typedef should be present. But the Platform.h > has an extra typedef to avoid an include. This is a pattern in the other > typedefs there, eg. TraceEventAPIAtomicWord and TraceEventHandle. That is why I > added a redundant typedef there. Ok, if this is just extending that pattern then it's OK with me. IMHO Ideally we'll cleanup the dependencies here at some point so such redundancy isn't necessary (probably can be part of the cleanup haraken@ is planning).
On 2015/06/11 14:58:39, Rick Byers wrote: > On 2015/06/11 14:49:06, ssid wrote: > > > https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... > > File public/platform/WebMemoryAllocatorDump.h (right): > > > > > https://codereview.chromium.org/1159923006/diff/290001/public/platform/WebMem... > > public/platform/WebMemoryAllocatorDump.h:12: typedef uint64_t > > WebMemoryAllocatorDumpGuid; > > On 2015/06/11 14:12:16, Rick Byers wrote: > > > this is redundant with the definition in the other file, right? > > > > Sorry, I misunderstood the comment. > > > > So, this is the file where this typedef should be present. But the Platform.h > > has an extra typedef to avoid an include. This is a pattern in the other > > typedefs there, eg. TraceEventAPIAtomicWord and TraceEventHandle. That is why > I > > added a redundant typedef there. > > Ok, if this is just extending that pattern then it's OK with me. IMHO Ideally > we'll cleanup the dependencies here at some point so such redundancy isn't > necessary (probably can be part of the cleanup haraken@ is planning). Yeah, this is the common pattern. LGTM.
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, rbyers@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1159923006/#ps330001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159923006/330001
Message was sent while issue was closed.
Committed patchset #16 (id:330001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197398 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
