Closed
Bug 1007203
Opened 7 years ago
Closed 7 years ago
Augment profile entries in the pseudostack with category information
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: vporof, Assigned: vporof)
References
Details
Attachments
(4 files, 5 obsolete files)
128.23 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
10.96 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
182.24 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
15.11 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Comment 1•7 years ago
|
||
You might want to move this to the Gecko Profiler component. Just sayin'.
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
Table of categories added to profile entries: https://pastebin.mozilla.org/5221652 I wonder if there's any benefit in making a distinction between layout and graphics (there's none made in the above pastebin). The key question here is whether web devs care about it or not, in the context of a "Profiler". IMHO a more comprehensive "Timeline" tool would be a better place to evidentiate such distinctions (like: here's how the layout was invalidated and what operations were on the stack, here's where painting happened abd what got painted, etc.). I'm also wondering if these pseudostack entries are enough for a comprehensive picture of "what's going on". It seems enough to me, because these already annotated functions are the most likely choke points, but would it be a good idea to annotate other parts of the code as well?
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8428004 -
Attachment is obsolete: true
Comment 6•7 years ago
|
||
I think the ultimate test is watching users and seeing if they are confused by it or not. I'd like to think that it would be useful but I think I will always prefer more data so I'm going to be heavily biased.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 7•7 years ago
|
||
More wip. Augmented PROFILER_LABEL, PROFILER_LABEL_PRINTF, PROFILER_MAIN_THREAD_LABEL and PROFILER_MAIN_THREAD_LABEL_PRINTF calls.
Attachment #8428005 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8430936 -
Attachment is obsolete: true
Attachment #8431032 -
Flags: review?(kvijayan)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8431033 -
Flags: review?(kvijayan)
Assignee | ||
Comment 10•7 years ago
|
||
Looks like the compiler we use on Windows XP doesn't know about |enum class|: https://tbpl.mozilla.org/?tree=Try&rev=32d1fb5ad9dd That's a bummer :(
Comment 11•7 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #10) > Looks like the compiler we use on Windows XP doesn't know about |enum > class|: https://tbpl.mozilla.org/?tree=Try&rev=32d1fb5ad9dd You can use MOZ_BEGIN_ENUM_CLASS / MOZ_END_ENUM_CLASS, defined in mfbt/TypedEnum.h; it uses enum classes if the compiler supports them and else it uses a class to emulate them.
Assignee | ||
Comment 12•7 years ago
|
||
Nice! Thanks!
Comment 13•7 years ago
|
||
Comment on attachment 8431032 [details] [diff] [review] v4 part 1 - Always add categories when pushing to the pseudostack Review of attachment 8431032 [details] [diff] [review]: ----------------------------------------------------------------- Where are the changes to the PROFILER_LABEL and PROFILER_LABEL_PRINTF defines? ::: gfx/layers/client/ClientThebesLayer.cpp @@ +39,3 @@ > NS_ASSERTION(ClientManager()->InDrawing(), > "Can only draw in drawing phase"); > Nit: pre-existing whitespace on blank line can be removed. ::: js/public/ProfilingStack.h @@ +71,5 @@ > + CC = 0x64, > + NETWORK = 0x128, > + GRAPHICS = 0x256, > + STORAGE = 0x512, > + EVENTS = 0x1024 These are decimal numbers in hex notation :) Probably not what you were going for. I think you want 0x04, 0x08, 0x10, 0x20, 0x40, 0x80, 0x100, etc.. ::: netwerk/base/src/nsStreamLoader.cpp @@ +90,1 @@ > NS_IMETHODIMP Nit: not introduced by you, but there's a blank space at the end of this line that could be removed.
Attachment #8431032 -
Flags: review?(kvijayan) → review+
Comment 14•7 years ago
|
||
Comment on attachment 8431033 [details] [diff] [review] v4 part 2 - Set the categories as flags on profile entries Review of attachment 8431033 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/PseudoStack.h @@ +361,5 @@ > // Make sure we increment the pointer after the name has > // been written such that mStack is always consistent. > entry.setLabel(aName); > entry.setCppFrame(aStackAddress, line); > + entry.setFlag(static_cast<uint32_t>(aCategory)); JS_ASSERT that aCategory is within its expected bounds here.
Attachment #8431033 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8431848 -
Flags: review+
Assignee | ||
Comment 17•7 years ago
|
||
Sigh, rebased.
Attachment #8431846 -
Attachment is obsolete: true
Attachment #8431848 -
Attachment is obsolete: true
Attachment #8431967 -
Flags: review+
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8431968 -
Flags: review+
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b83af60025b8 https://hg.mozilla.org/integration/fx-team/rev/9482bb487aa5
Whiteboard: [fixed-in-fx-team]
Comment 20•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b83af60025b8 https://hg.mozilla.org/mozilla-central/rev/9482bb487aa5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
This patch modified a bunch of indexedDB labels, moving everything out of a generic "IndexedDB" namespace into multiple unintelligible namespaces (e.g. "AsyncConnectionHelper")... Was there a reason to do this? I would have appreciated someone reaching out to me before modifying the indexedDB code like this.
Assignee | ||
Comment 22•7 years ago
|
||
It looked like a reasonable cleanup. We could use a separate "category" for those cases (like STORAGE | INDEXEDDB), and/or we can put those labels back in. Either way, I think it'd be a good idea to keep the namespaces as well.
I don't mind the category stuff, I think 'STORAGE' is fine. I just don't see that any value was added by creating namespaces like 'AsyncConnectionHelper', 'CursorHelper', 'ContinueHelper', etc.
Assignee | ||
Comment 24•7 years ago
|
||
It better reflects the reality (code-wise). Of course, I'm assuming that this is harder to search for in the Gecko Profiler, so I'm totally with you on this. How about having (IndexedDB::AsyncConnectionHelper, Run, STORAGE)?
I don't think it makes much sense to search for specific class names... That's not how I personally would approach a slow database problem. Rather I think I would want to just be able to filter/search all the indexedDB events.
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #25) > I don't think it makes much sense to search for specific class names... > That's not how I personally would approach a slow database problem. Rather I > think I would want to just be able to filter/search all the indexedDB events. You will certainly be able to do that if "IndexedDB" is prepended to labels (just like it worked before). I strongly believe that having correct class/method naming information in a profiler is a good idea, so removing this information is pretty dramatic. The best-case scenario here would be for the Gecko Profiler to actually enable searching for categories, like STORAGE and/or INDEXEDDB, but that's probably a bit more involved than just prepending "IndexedDB" to everything. I can take care of this.
(In reply to Victor Porof [:vporof][:vp] from comment #26) > I can take care of this. Actually I'd rather you didn't. I'm doing a fairly substantial rewrite of the idb code at the moment and this kind of change causes lots of merge time. Just tell me what you think the calls should look like and I'll update them in my branch. We can leave trunk alone.
Assignee | ||
Comment 28•7 years ago
|
||
Oh, ok. I filed bug 1019125 anyway. Like I said, PROFILER_LABEL("IndexedDB::AsyncConnectionHelper", "Run", Bla::Bla::STORAGE) instead of PROFILER_LABEL("AsyncConnectionHelper", "Run", Bla::Bla::STORAGE) is a good approach imho.
And what about the cases where functions being labeled are not methods of a class?
Assignee | ||
Comment 30•7 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #29) > And what about the cases where functions being labeled are not methods of a > class? How about filename, namespace, or empty string if nothing else makes sense?
I don't think that makes sense really... 1. Filename. Why would we do that for non-class functions but then not for class methods? 2. Namespace. IDB uses a bunch of anonymous namespaces (there are, for example, two 'GetHelper' classes in two different files). 3. Empty string. This seems to defeat the whole purpose of namespacing...
It still seems to me like everything should exist in the "IndexedDB" namespace and we can use the info arg for filename/class info...
Assignee | ||
Comment 33•7 years ago
|
||
That works too, as long as we don't loose class names.
(In reply to Victor Porof [:vporof][:vp] from comment #33) > That works too, as long as we don't loose class names. Ok. That's what I had before.
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•