859041 - Display timing interval divisions (ms ticks) in the timeline

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

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

For example, each 250, 1000 and 2000 milliseconds should be delimited by subtle vertical lines.

Comment 2

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

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 3

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

Attachment: v1

Well, that's that.

Comment 4

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

Attachment: v1

There's a bit of witchcraft going on in there, but it makes things fly fast.

I hope I made the code as clear as possible, but when it comes to anything canvas and buffers related, it's hard not to create black holes.

Comment 5

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

Attachment: le screenshot

Comment 6

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

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

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

Some concern about the performance when flushing, but otherwise I am quite pleased by this code. You may choose to ignore silly suggestions as they were mostly meant for entertainment purposes.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +362,5 @@
>      if (!this.lazyUpdate) {
>        return void this._flushRequests();
>      }
> +    // Allow requests to settle down first.
> +    drain("update-requests", REQUESTS_REFRESH_RATE, () => this._flushRequests());

flush 'em down the drain.

@@ +598,5 @@
>    _flushWaterfallViews: function NVRM__flushWaterfallViews(aReset) {
> +    // To avoid expensive operations like getBoundingClientRect() and
> +    // rebuilding the waterfall background each time a new request comes in,
> +    // stuff is cached. However, in certain scenarios like when the window
> +    // is resized, this needs to be invalidated.

looks like this just got more expensive with the hideOverflowingColumns.

@@ +603,3 @@
>      if (aReset) {
>        this._cachedWaterfallWidth = 0;
> +      this._hideOverflowingColumns();

flushes, drains, waterfalls... This code is making me thirsty.

@@ +652,5 @@
> +  _showWaterfallDivisionLabels: function NVRM__showWaterfallDivisionLabels(aScale) {
> +    let container = $("#requests-menu-waterfall-header-box");
> +    let availableWidth = this._waterfallWidth - REQUESTS_WATERFALL_SAFE_BOUNDS;
> +
> +    // Nuke all existing labels.

shouldn't you be using a more watery image for this?

e.g., "Splash"?

@@ +654,5 @@
> +    let availableWidth = this._waterfallWidth - REQUESTS_WATERFALL_SAFE_BOUNDS;
> +
> +    // Nuke all existing labels.
> +    while (container.hasChildNodes()) {
> +      container.firstChild.remove();

!

@@ +665,5 @@
> +    while (!optimalTickIntervalFound) {
> +      // Ignore any divisions that would end up being too close to each other.
> +      let scaledStep = aScale * timingStep;
> +      if (scaledStep < REQUESTS_WATERFALL_HEADER_TICKS_SPACING_MIN) {
> +        timingStep *= 2;

could use a right bit shift here. Just sayin'.

@@ +667,5 @@
> +      let scaledStep = aScale * timingStep;
> +      if (scaledStep < REQUESTS_WATERFALL_HEADER_TICKS_SPACING_MIN) {
> +        timingStep *= 2;
> +        continue;
> +      }

this is almost a literal implementation of the Fast Bogo Interval Search algorithm.

@@ +679,5 @@
> +        node.className = "plain requests-menu-timings-division";
> +        node.style.transform = translateX;
> +
> +        node.setAttribute("value", text);
> +        container.appendChild(node);

Wonder if we should use some sort of documentFragment thingy to contain these while constructing and then move them into the container en masse?

I guess we're only doing this for the division labels so probably not that many total. < 100.

It's fine.

@@ +699,5 @@
> +    }
> +    let canvas = this._canvas;
> +    let ctx = this._ctx;
> +
> +    // Nuke the context.

"Slosh"?

@@ +701,5 @@
> +    let ctx = this._ctx;
> +
> +    // Nuke the context.
> +    let canvasWidth = canvas.width = this._waterfallWidth;
> +    let canvasHeight = canvas.height = 1; // Awww yeah, 1px, repeats on Y axis.

!

@@ +703,5 @@
> +    // Nuke the context.
> +    let canvasWidth = canvas.width = this._waterfallWidth;
> +    let canvasHeight = canvas.height = 1; // Awww yeah, 1px, repeats on Y axis.
> +
> +    // Ironically, ArrayBufferViews add too much overhead in this case. KISS.

It's like O Henry and Alanis Morissette had a baby and named it this exact situation!

@@ +717,5 @@
> +    while (!optimalTickIntervalFound) {
> +      // Ignore any divisions that would end up being too close to each other.
> +      let scaledStep = aScale * timingStep;
> +      if (scaledStep < REQUESTS_WATERFALL_BACKGROUND_TICKS_SPACING_MIN) {
> +        timingStep *= 2;

bitshift! (I'm just kidding, you don't have to do that).

@@ +723,5 @@
> +      }
> +
> +      // Insert one pixel for each division on each scale.
> +      for (let i = 1; i <= REQUESTS_WATERFALL_BACKGROUND_TICKS_SCALES; i++) {
> +        for (let x = 0; x < canvasWidth; x += scaledStep * Math.pow(2, i)) {

scaledStep * Math.pow(2, i)

you could pre-calculate this in the outer loop.

@@ +726,5 @@
> +      for (let i = 1; i <= REQUESTS_WATERFALL_BACKGROUND_TICKS_SCALES; i++) {
> +        for (let x = 0; x < canvasWidth; x += scaledStep * Math.pow(2, i)) {
> +          let index = (x | 0) * 4;
> +          pixelArray[index] = pixelArray[++index] = pixelArray[++index] = 255;
> +          pixelArray[++index] = alphaComponent;

is this really the easiest+fastest way to display an alpha-blended white pixel? -__-

It smacks of pointer math. And I hate pointer math.

@@ +1344,5 @@
>  
>  /**
> + * DOM query helper.
> + */
> +function $(aSelector, aTarget = document) aTarget.querySelector(aSelector);

require(["jquery", "jquery.alpha", "jquery.beta"], function($) {
    //the jquery.alpha.js and jquery.beta.js plugins have been loaded.
    $(function() {
        $('body').alpha().beta();
    });
});

::: browser/devtools/netmonitor/test/browser_net_timeline_ticks.js
@@ +133,5 @@
> +    });
> +
> +    aDebuggee.location.reload();
> +  });
> +}

yeah, I'm sure this is all fine.

::: browser/locales/en-US/chrome/browser/devtools/netmonitor.properties
@@ +106,5 @@
>  networkMenu.totalMS=→ %S ms
> +
> +# LOCALIZATION NOTE (networkMenu.divisionMS): This is the label displayed
> +# in the network menu specifying timing interval divisions (in milliseconds).
> +networkMenu.divisionMS=%S ms

do we need a space between the %S and ms? I'm not sure what the best practice is there.

Some office polling has declared the space essential (with an error bar of no higher than 33%) and agrees with the SI:

https://en.wikipedia.org/wiki/SI#Writing_unit_symbols_and_the_values_of_quantities

::: browser/themes/linux/devtools/netmonitor.css
@@ +137,5 @@
>  
>  .requests-menu-subitem.requests-menu-waterfall {
>    -moz-padding-start: 4px;
>    -moz-padding-end: 4px;
> +  background-repeat: repeat-y; /* Background created on a <canvas> in js. */

that's awesome. :)

Comment 7

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> Comment on attachment 741706 [details] [diff] [review]
> v1
> 
> Review of attachment 741706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> 
> @@ +598,5 @@
> >    _flushWaterfallViews: function NVRM__flushWaterfallViews(aReset) {
> > +    // To avoid expensive operations like getBoundingClientRect() and
> > +    // rebuilding the waterfall background each time a new request comes in,
> > +    // stuff is cached. However, in certain scenarios like when the window
> > +    // is resized, this needs to be invalidated.
> 
> looks like this just got more expensive with the hideOverflowingColumns.
> 

It was there from bug 860175. I just tidied up a bit.
But I do agree that rebuilding the canvas background on resize adds a bit of slowness, but I couldn't possibly optimize this more.

PS: Having a *larger* canvas to avoid recreating it on resize to exact dimensions was actually *way!!* slower.

> @@ +603,3 @@
> >      if (aReset) {
> >        this._cachedWaterfallWidth = 0;
> > +      this._hideOverflowingColumns();
> 
> flushes, drains, waterfalls... This code is making me thirsty.
> 

How many beers do I owe you? :)

> 
> @@ +665,5 @@
> > +    while (!optimalTickIntervalFound) {
> > +      // Ignore any divisions that would end up being too close to each other.
> > +      let scaledStep = aScale * timingStep;
> > +      if (scaledStep < REQUESTS_WATERFALL_HEADER_TICKS_SPACING_MIN) {
> > +        timingStep *= 2;
> 
> could use a right bit shift here. Just sayin'.
> 

All my love.
 
> @@ +679,5 @@
> > +        node.className = "plain requests-menu-timings-division";
> > +        node.style.transform = translateX;
> > +
> > +        node.setAttribute("value", text);
> > +        container.appendChild(node);
> 
> Wonder if we should use some sort of documentFragment thingy to contain
> these while constructing and then move them into the container en masse?
> 
> I guess we're only doing this for the division labels so probably not that
> many total. < 100.
> 

I bet it's always less than 10 nodes. But it's a worthwhile optimization, I'll do it.

> It's fine.
> 
> @@ +699,5 @@
> > +    }
> > +    let canvas = this._canvas;
> > +    let ctx = this._ctx;
> > +
> > +    // Nuke the context.
> 
> "Slosh"?
> 
> @@ +701,5 @@
> > +    let ctx = this._ctx;
> > +
> > +    // Nuke the context.
> > +    let canvasWidth = canvas.width = this._waterfallWidth;
> > +    let canvasHeight = canvas.height = 1; // Awww yeah, 1px, repeats on Y axis.
> 
> !
> 
> @@ +703,5 @@
> > +    // Nuke the context.
> > +    let canvasWidth = canvas.width = this._waterfallWidth;
> > +    let canvasHeight = canvas.height = 1; // Awww yeah, 1px, repeats on Y axis.
> > +
> > +    // Ironically, ArrayBufferViews add too much overhead in this case. KISS.
> 
> It's like O Henry and Alanis Morissette had a baby and named it this exact
> situation!
> 

I'm learning so many things here at Mozilla.

> > +      }
> > +
> > +      // Insert one pixel for each division on each scale.
> > +      for (let i = 1; i <= REQUESTS_WATERFALL_BACKGROUND_TICKS_SCALES; i++) {
> > +        for (let x = 0; x < canvasWidth; x += scaledStep * Math.pow(2, i)) {
> 
> scaledStep * Math.pow(2, i)
> 
> you could pre-calculate this in the outer loop.
> 

Why didn't I think of this? :( My gfx optimization roots are getting rusty.

> @@ +726,5 @@
> > +      for (let i = 1; i <= REQUESTS_WATERFALL_BACKGROUND_TICKS_SCALES; i++) {
> > +        for (let x = 0; x < canvasWidth; x += scaledStep * Math.pow(2, i)) {
> > +          let index = (x | 0) * 4;
> > +          pixelArray[index] = pixelArray[++index] = pixelArray[++index] = 255;
> > +          pixelArray[++index] = alphaComponent;
> 
> is this really the easiest+fastest way to display an alpha-blended white
> pixel? -__-
> 
> It smacks of pointer math. And I hate pointer math.
> 

Would you prefer me using buffer views? It'd be like
data32[ptr] = (alphaComponent << 24) | (255 << 16) | (255 <<  8) | 255;


> ::: browser/locales/en-US/chrome/browser/devtools/netmonitor.properties
> @@ +106,5 @@
> >  networkMenu.totalMS=→ %S ms
> > +
> > +# LOCALIZATION NOTE (networkMenu.divisionMS): This is the label displayed
> > +# in the network menu specifying timing interval divisions (in milliseconds).
> > +networkMenu.divisionMS=%S ms
> 
> do we need a space between the %S and ms? I'm not sure what the best
> practice is there.
> 

Apparently yes: https://bugzilla.mozilla.org/show_bug.cgi?id=855544#c27

Comment 8

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

Attachment: v1.1

Addressed comments.

Comment 9

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

Green: https://tbpl.mozilla.org/?tree=Try&rev=6dcc1c0f1ecc

Comment 10

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

(In reply to Victor Porof [:vp] from comment #7)
> > 
> > scaledStep * Math.pow(2, i)
> > 
> > you could pre-calculate this in the outer loop.
> > 

Also bitshift it: scaledStep << i :)

Comment 11

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

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

Comment 12

[Joe Walker [:jwalker]]

https://hg.mozilla.org/mozilla-central/rev/1f9bab0483b1

Comment 13

[Alfred Kayser]

Generating a background with hardcoded semi-transparent white bars is very theme unfriendly. With non-black background (i.e. normal white or -moz-field) the bars are invisible.
Either use a color derived from some css rule or use a dotted line so that it works on both dark and light backgrounds.

Hardcode color coding:
+        for (let x = 0; x < canvasWidth; x += increment) {
+          data32[x | 0] = (alphaComponent << 24) | (255 << 16) | (255 <<  8) | 255;
+        }

Comment 14

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

(In reply to Alfred Kayser from comment #13)
> Generating a background with hardcoded semi-transparent white bars is very
> theme unfriendly. With non-black background (i.e. normal white or
> -moz-field) the bars are invisible.

True, however the netmonitor doesn't have a white theme at this point. I suppose moving the pixels color into a const is acceptable, but deriving color from a css rule back into js is overkill IMHO, and using a dotted like would have a noticeable perf impact. Maybe I'm missing something.

Comment 15

[Alfred Kayser]

Re: "however the netmonitor doesn't have a white theme at this point":
The point of themes (such as Walnut, Nautipolis, etc) is to allow different color an style schemes.

There are several options to make the interval pattern more theme aware:
1. Instead of generating a background image with bars, one could also use a linear-gradient as follows:
background-size: <timing-interval>
background-image:linear-gradient(to right <barcolor> 1px, transparent)
2. Or just use rgba(128,128,128,<alpha>) so it works both on dark and light backgrounds.
3. Dotted (2px high, with the first row set with rgba(255,255,255,alpha) and second with rgba(0,0,0,alpha). When a background image is used, the size doesn't matter that much (either repeat height times 1px row or repeat height/2 a 2px row).
4. Get the background color of the panel and set a contrasting color.

Comment 16

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

(In reply to Alfred Kayser from comment #15)

> There are several options to make the interval pattern more theme aware:
> 1. Instead of generating a background image with bars, one could also use a
> linear-gradient as follows:
> background-size: <timing-interval>
> background-image:linear-gradient(to right <barcolor> 1px, transparent)

I know. It was like this before and the performance was terrible, this is the reason it was removed.

> 2. Or just use rgba(128,128,128,<alpha>) so it works both on dark and light
> backgrounds.

I think this is the most sensible and performant approach.

Comment 17

[Alfred Kayser]

Thamks, it works in my themes beautifully!

Comment 18

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

(In reply to Alfred Kayser from comment #17)
> Thamks, it works in my themes beautifully!

\o/ Care to post a screenshot, I'm curious how it looks :)