1211839 - Don't allow off the main thread markers to nest under main thread markers

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: v1

Comment 2

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

Comment on attachment 8670175 [details] [diff] [review]
v1

Oops, wrong patch.

Comment 3

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

Attachment: v1

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8670239 [details] [diff] [review]
v1

Review of attachment 8670239 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed and marker docs -- someone else should probably review the platform code, though

::: devtools/client/performance/modules/logic/waterfall-utils.js
@@ +63,5 @@
> +    // A beter solution would be to collapse every marker with its siblings
> +    // from the same thread, but that would require a thread id attached
> +    // to all markers, which is potentially expensive and rather useless at
> +    // the moment, since we don't really have that many OTMT markers.
> +    if (!nestable || !curr.isMainThread) {

Very good comment, for real

@@ +73,5 @@
>      // recursively upwards if this marker is outside their ranges and nestable.
>      while (!finalized && parentNode) {
> +      // If the markers aren't from the same process, attach them to the root
> +      // node as well. Every process has its own main thread.
> +      if (!curr.processType != parentNode.processType) {

nit: !==

::: devtools/client/performance/test/unit/test_waterfall-utils-collapse-05.js
@@ +48,5 @@
> +  // This should never collapse.
> +  { start: 2, end: 3, name: "C", processType: 1, isMainThread: false },
> +
> +  // This should collapse only under A2-mt
> +  { start: 2, end: 3, name: "D", processType: 2, isMainThread: true },

So we collapse markers when they're on the main thread in the same process -- what scenario would we have where we see another process's main threads markers? I think this is "correct", but just trying to understand.

@@ +67,5 @@
> +      { start: 2, end: 3, name: "B" },
> +    ]},
> +    { start: 1, end: 4, name: "A1-otmt"},
> +    { start: 2, end: 3, name: "C"},
> +    { start: 1, end: 4, name: "A2-mt", submarkers: [

Shouldn't this be ordered before "C"? Maybe less of an issue, or no longer a guarantee with OTMT markers?

::: dom/webidl/ProfileTimelineMarker.webidl
@@ +31,5 @@
>    DOMHighResTimeStamp end = 0;
>    object? stack = null;
>  
> +  unsigned short processType;
> +  boolean isMainThread;

Need to add these to marker docs as all markers have them, and was a bit confused about what "processType" was, so it'd be good to list out the enums, atleast what we have right now, referencing the source in `xpcom/build/nsXULAppAPI.h` it looks like.

Comment 5

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Comment on attachment 8670239 [details] [diff] [review]
> v1
> 
> Review of attachment 8670239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed and marker docs -- someone else should probably
> review the platform code, though
> 
> ::: devtools/client/performance/modules/logic/waterfall-utils.js
> @@ +63,5 @@
> > +    // A beter solution would be to collapse every marker with its siblings
> > +    // from the same thread, but that would require a thread id attached
> > +    // to all markers, which is potentially expensive and rather useless at
> > +    // the moment, since we don't really have that many OTMT markers.
> > +    if (!nestable || !curr.isMainThread) {
> 
> Very good comment, for real
> 
> @@ +73,5 @@
> >      // recursively upwards if this marker is outside their ranges and nestable.
> >      while (!finalized && parentNode) {
> > +      // If the markers aren't from the same process, attach them to the root
> > +      // node as well. Every process has its own main thread.
> > +      if (!curr.processType != parentNode.processType) {
> 
> nit: !==
> 
> ::: devtools/client/performance/test/unit/test_waterfall-utils-collapse-05.js
> @@ +48,5 @@
> > +  // This should never collapse.
> > +  { start: 2, end: 3, name: "C", processType: 1, isMainThread: false },
> > +
> > +  // This should collapse only under A2-mt
> > +  { start: 2, end: 3, name: "D", processType: 2, isMainThread: true },
> 
> So we collapse markers when they're on the main thread in the same process
> -- what scenario would we have where we see another process's main threads
> markers? I think this is "correct", but just trying to understand.
> 

There is absolutely no scenario yet. See bug 1200120. However, hypothetically, we may want to see markers originating from some process dealing with Oculus rift inputs, for example. None of that is implemented yet either though, and I'm not even sure it has to be another process. A better example may be the compositor process: currently we're cheating a little bit by piggybacking the IPC protocol back to the main thread and adding markers there, but it'd be really nice if we could just skip all of this madness and just add markers directly near the code they're concerned with. This would require sending over markers through our own IPC protocol, to the TimelineConsumers instance living on the same process where the Performance tool lives.

> @@ +67,5 @@
> > +      { start: 2, end: 3, name: "B" },
> > +    ]},
> > +    { start: 1, end: 4, name: "A1-otmt"},
> > +    { start: 2, end: 3, name: "C"},
> > +    { start: 1, end: 4, name: "A2-mt", submarkers: [
> 
> Shouldn't this be ordered before "C"? Maybe less of an issue, or no longer a
> guarantee with OTMT markers?
> 

All of those markers start and end at exactly the same time. So not really an issue.

> ::: dom/webidl/ProfileTimelineMarker.webidl
> @@ +31,5 @@
> >    DOMHighResTimeStamp end = 0;
> >    object? stack = null;
> >  
> > +  unsigned short processType;
> > +  boolean isMainThread;
> 
> Need to add these to marker docs as all markers have them, and was a bit
> confused about what "processType" was, so it'd be good to list out the
> enums, atleast what we have right now, referencing the source in
> `xpcom/build/nsXULAppAPI.h` it looks like.

Sure.

Comment 6

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

Attachment: v2

Rebased.

Comment 7

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

Attachment: v3

Rebased.

Comment 8

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

Comment on attachment 8678149 [details] [diff] [review]
v3

Olli can you please take a look at the platform bits?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a5a31b8a5383

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a5a31b8a5383