Closed Bug 651673 Opened 9 years ago Closed 9 years ago

Improve performance of endurance tests charts with lots of data

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → dave.hunt
Attachment #527418 - Flags: review?(hskupin)
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.
Attachment #527418 - Flags: review?(hskupin) → review-
(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.
Attachment #527418 - Attachment is obsolete: true
Attachment #529963 - Flags: review?(hskupin)
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.
Attachment #529963 - Flags: review?(hskupin) → review-
(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.
Attachment #529963 - Attachment is obsolete: true
Attachment #532036 - Flags: review?(hskupin)
Comment on attachment 532036 [details] [diff] [review]
Improve performance of endurance tests charts. v1.2

Sweet. Will land it right away.
Attachment #532036 - Flags: review?(hskupin) → review+
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.