Augment profile entries in the pseudostack with category information

RESOLVED FIXED in Firefox 32

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

No description provided.
Blocks: 879008
Depends on: 1004726
Assignee: nobody → vporof
Status: NEW → ASSIGNED
You might want to move this to the Gecko Profiler component. Just sayin'.
Blocks: 1007460
No longer blocks: 879008
Priority: -- → P2
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?
Posted patch v1 wip (obsolete) — Splinter Review
Posted patch v2 wip (obsolete) — Splinter Review
Attachment #8428004 - Attachment is obsolete: true
Re comment 2, what do you think?
Flags: needinfo?(bgirard)
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)
Posted patch v3 (obsolete) — Splinter Review
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
Blocks: 1017790
No longer blocks: 1007460
Attachment #8430936 - Attachment is obsolete: true
Attachment #8431032 - Flags: review?(kvijayan)
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 :(
(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.
Nice! Thanks!
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 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+
Posted patch v5, part 1 (obsolete) — Splinter Review
Addressed comments.
Attachment #8431846 - Flags: review+
Posted patch v5, part 2 (obsolete) — Splinter Review
Attachment #8431848 - Flags: review+
Sigh, rebased.
Attachment #8431846 - Attachment is obsolete: true
Attachment #8431848 - Attachment is obsolete: true
Attachment #8431967 - Flags: review+
Attachment #8431968 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b83af60025b8
https://hg.mozilla.org/mozilla-central/rev/9482bb487aa5
Status: ASSIGNED → RESOLVED
Closed: 5 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.
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.
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.
(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.
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?
(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...
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.
Duplicate of this bug: 871439
Depends on: 1362894
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.