651673 - Improve performance of endurance tests charts with lots of data

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Result Dashboard, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Unfortunately highcharts does not support large sets of data well, and attempts to plot all points, which can take a very long time. We need a way to improve the performance of displaying these charts.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Improve performance of endurance tests charts. v1

This patch simply drops checkpoints to 450 or less. As a side effect we may miss spikes in the charts, so we will need a better solution in the future.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 527418 [details] [diff] [review]
Improve performance of endurance tests charts. v1

>+        var maximumCheckpoints = 450;

Do you set the number of points for the chart? Can we use a constant or get this information from the chart somehow, instead of using a hardcoded number?

>+        var divisor = Math.ceil(allCheckpoints.length / maximumCheckpoints);

Please put this line into the else condition of the following if/else construct.

>+        if (allCheckpoints.length < maximumCheckpoints) {

Shouldn't this be '<='?

>+          for (var i=0; i < allCheckpoints.length; i++) {

nit: spaces around '='.

>+            if (i % divisor === 0) {
>+              context.checkpoints.push(allCheckpoints[i]);
>+            }

hm, we would drop a large number of checkpoints if the number is a bit higher than a multiple of the max checkpoints, i.e 1.1 => 2.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Improve performance of endurance tests charts. v1.1

(In reply to comment #2)
> Comment on attachment 527418 [details] [diff] [review] [review]
> Improve performance of endurance tests charts. v1
> 
> >+        var maximumCheckpoints = 450;
> 
> Do you set the number of points for the chart? Can we use a constant or get
> this information from the chart somehow, instead of using a hardcoded number?

This is taken from the width of the chart element. I've added a constant.

> >+        var divisor = Math.ceil(allCheckpoints.length / maximumCheckpoints);
> 
> Please put this line into the else condition of the following if/else
> construct.

Done.

> >+        if (allCheckpoints.length < maximumCheckpoints) {
> 
> Shouldn't this be '<='?

I don't think so. I tried it and I had errors. It's a zero indexed array, so allCheckpoints[allCheckpoints.length] is out of index, right?

> >+          for (var i=0; i < allCheckpoints.length; i++) {
> 
> nit: spaces around '='.

Done.

> >+            if (i % divisor === 0) {
> >+              context.checkpoints.push(allCheckpoints[i]);
> >+            }
> 
> hm, we would drop a large number of checkpoints if the number is a bit higher
> than a multiple of the max checkpoints, i.e 1.1 => 2.

I've changed this to not round the divisor, and only include checkpoints when the index has a remainder of < 1 and this seems to have a pretty good result. Please provide feedback on whether this is a good idea.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 529963 [details] [diff] [review]
Improve performance of endurance tests charts. v1.1

> > >+        if (allCheckpoints.length < maximumCheckpoints) {
> > 
> > Shouldn't this be '<='?
> 
> I don't think so. I tried it and I had errors. It's a zero indexed array, so
> allCheckpoints[allCheckpoints.length] is out of index, right?
> 
>+        if (allCheckpoints.length < MAX_CHART_CHECKPOINTS) {
>+          context.checkpoints = allCheckpoints;

Well, if the number of checkpoints is equal to the max number of checkpoints, you wanna execute the first case and not the else case.

>+        } else {

Put else in the next line.

>+          var divisor = allCheckpoints.length / MAX_CHART_CHECKPOINTS;
>+          for (var i = 0; i < allCheckpoints.length; i++) {
>+            if (i % divisor < 1) {
>+              context.checkpoints.push(allCheckpoints[i]);
>+            }
>+          }

Looks like an elegant solution. I like it. Can you just add brackets around the % operands so it is clearer? Also at least a comment would be nice.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Improve performance of endurance tests charts. v1.2

(In reply to comment #4)
> Comment on attachment 529963 [details] [diff] [review] [review]
> Improve performance of endurance tests charts. v1.1
> 
> > > >+        if (allCheckpoints.length < maximumCheckpoints) {
> > > 
> > > Shouldn't this be '<='?
> > 
> > I don't think so. I tried it and I had errors. It's a zero indexed array, so
> > allCheckpoints[allCheckpoints.length] is out of index, right?
> > 
> >+        if (allCheckpoints.length < MAX_CHART_CHECKPOINTS) {
> >+          context.checkpoints = allCheckpoints;
> 
> Well, if the number of checkpoints is equal to the max number of
> checkpoints, you wanna execute the first case and not the else case.

Sorry, you're 100% correct here. I think I misunderstood this the first time I read your comment and was applying it to another part of the code.

> >+        } else {
> 
> Put else in the next line.

Done. Just to note that we're inconsistent on this style elsewhere in the file.

> >+          var divisor = allCheckpoints.length / MAX_CHART_CHECKPOINTS;
> >+          for (var i = 0; i < allCheckpoints.length; i++) {
> >+            if (i % divisor < 1) {
> >+              context.checkpoints.push(allCheckpoints[i]);
> >+            }
> >+          }
> 
> Looks like an elegant solution. I like it. Can you just add brackets around
> the % operands so it is clearer? Also at least a comment would be nice.

Done.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 532036 [details] [diff] [review]
Improve performance of endurance tests charts. v1.2

Sweet. Will land it right away.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
https://github.com/whimboo/mozmill-dashboard/commit/4e7dbff6687ded7e7fa126427483229152cb18a2

Also pushed to all instances of the dashboard. For example see:
http://mozmill-crowd.brasstacks.mozilla.com/#/endurance/report/7da43dc703ad47e7276132c0993e97de