1007203 - Augment profile entries in the pseudostack with category information

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

Comment 1

[Rob Campbell [:rc] (:robcee)]

You might want to move this to the Gecko Profiler component. Just sayin'.

Comment 2

[Victor Porof [:vporof][:vp]]

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?

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v1 wip

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2 wip

Comment 5

[Victor Porof [:vporof][:vp]]

Re comment 2, what do you think?

Comment 6

[Benoit Girard (:BenWa)]

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.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v3

More wip. Augmented PROFILER_LABEL, PROFILER_LABEL_PRINTF, PROFILER_MAIN_THREAD_LABEL and PROFILER_MAIN_THREAD_LABEL_PRINTF calls.

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: v4 part 1 - Always add categories when pushing to the pseudostack

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: v4 part 2 - Set the categories as flags on profile entries

Comment 10

[Victor Porof [:vporof][:vp]]

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

[Jan de Mooij [:jandem]]

(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.

Comment 12

[Victor Porof [:vporof][:vp]]

Nice! Thanks!

Comment 13

[Kannan Vijayan [:djvj]]

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.

Comment 14

[Kannan Vijayan [:djvj]]

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.

Comment 15

[Victor Porof [:vporof][:vp]]

Attachment: v5, part 1

Addressed comments.

Comment 16

[Victor Porof [:vporof][:vp]]

Attachment: v5, part 2

Comment 17

[Victor Porof [:vporof][:vp]]

Attachment: v5, rebased, part 1

Sigh, rebased.

Comment 18

[Victor Porof [:vporof][:vp]]

Attachment: v5, rebased, part 2

Comment 19

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/b83af60025b8
https://hg.mozilla.org/integration/fx-team/rev/9482bb487aa5

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b83af60025b8
https://hg.mozilla.org/mozilla-central/rev/9482bb487aa5

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 22

[Victor Porof [:vporof][:vp]]

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.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 24

[Victor Porof [:vporof][:vp]]

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)?

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 26

[Victor Porof [:vporof][:vp]]

(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.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 28

[Victor Porof [:vporof][:vp]]

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.

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And what about the cases where functions being labeled are not methods of a class?

Comment 30

[Victor Porof [:vporof][:vp]]

(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?

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It still seems to me like everything should exist in the "IndexedDB" namespace and we can use the info arg for filename/class info...

Comment 33

[Victor Porof [:vporof][:vp]]

That works too, as long as we don't loose class names.

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.