619651 - Display stuff as soon as it comes in instead of waiting for everything to load

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Comment 1

[Markus Stange [:mstange]]

Attachment: part1, v1

Provide more fine-grained communication channels between Data and UserInterface.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 1, v2

forgot to remove handleRefresh method

Comment 3

[Markus Stange [:mstange]]

Attachment: part 2, v1

Also hide the go back arrow before pushlog has loaded.

Nothing to do with this bug really, I admit...

Comment 4

[Arpad Borsos [:Swatinem]]

Comment on attachment 498083 [details] [diff] [review]
part 1, v2

>   _statusCallback: null,
>-  _refreshCallback: null,
>+  _updatedPushCallback: null,
>+  _infraStatsCallback: null,
>+  _initialPushlogLoadCallback: null,

>     this._statusCallback = uiConf.status;
>-    this._refreshCallback = uiConf.refresh;
>+    this._updatedPushCallback = uiConf.handleUpdatedPush;
>+    this._infraStatsCallback = uiConf.handleInfraStatsUpdate;
>+    this._initialPushlogLoadCallback = uiConf.handleInitialPushlogLoad;

I think saving a reference to the whole uiConf object makes this code easier.

>-    return {status: self.updateStatus, refresh: function (pushes, infraStats) { self.handleRefresh(pushes, infraStats); } };
>+    return {
>+      status: function (status) {
>+        self.updateStatus(status);
>+      },
>+      handleUpdatedPush: function (push) {
>+        self.handleUpdatedPush(push);
>+      },
>+      handleInfraStatsUpdate: function (infraStats) {
>+        self.handleInfraStatsUpdate(infraStats);
>+      },
>+      handleInitialPushlogLoad: function () {
>+        self.handleInitialPushlogLoad();
>+      },
>+    };

You need all this indirection because otherwise the "this" reference will be messed up, right?

>+  handleInitialPushlogLoad: function UserInterface_handleInitialPushlogLoad() {
>+    $("#pushes").removeClass("initialload");
>   },

You can as well inline this trivial function.

Comment 5

[Markus Stange [:mstange]]

Attachment: part 3, v1

Display stuff as soon as it comes in instead of waiting for everything to load.

Sorry in advance for this giant patch.
This basically reimplements _combineResults, just with more verbose function names, and in a fashion that can deal with piecewise updates.

Comment 6

[Markus Stange [:mstange]]

Attachment: part 4, v1

Pass LoadTracker down into the things that load.

Comment 7

[Markus Stange [:mstange]]

Attachment: part 4, v2

missed something

Comment 8

[Arpad Borsos [:Swatinem]]

Comment on attachment 498097 [details] [diff] [review]
part 3, v1

>       },
>-      failCallback,
>+      function () { loadTracker.loadFailed() },
(twice)
This way we don’t know what exactly caused the error. Also, can you directly pass the loadFailed function as argument (haven’t tested it, but should work without messing up "this")

>+        if (pendingOrRunning == "running")
>+          self._discardPendingRunsThatAreAlreadyRunning(updatedPushes);
>+        else
>+          self._discardPendingRunsThatAreAlreadyRunning();

You removeResultFromPush() in both cases but only add it to the updated pushes in one case, I don’t understand why?

>+  _accumulatePushesWithRunningBuildsOnMachine: function Data__accumulatePushesWithRunningBuildsOnMachine(machine, updatedPushes) {
>+    for (var repo in this._runningJobs) {
>+      for (var rev in this._runningJobs[repo]) {
>+        if (!(rev in this._pushes) || (rev in updatedPushes))
>+          continue;
>+        for (var i = 0; i < this._runningJobs[repo][rev].length; i++) {
>+          var runningResult = this._runningJobs[repo][rev][i];
>+          if (machine == this.getMachine(runningResult.buildername)) {
>+            updatedPushes[rev] = this._pushes[rev];
>+            break;
>+          }
>+        }
>+      }
>+    }
>+  },

This function is called with the comment
// Make sure the ETA on running builds gets updated.
above it, inside addFinishedResultToPush() when the result is successful.
This does not seem quite right.

>+  _getSortedPushesArray: function Data__getSortedPushesArray(unsortedPushesObject) {
>+    var pushes = Controller.valuesFromObject(unsortedPushesObject);
>+    pushes.sort(function(a,b) { return a.date - b.date; });
>+    return pushes;
>+  },
>+
>+  _notifyUpdatedPushes: function Data__notifyUpdatedPushes(updatedPushes, callback) {
>+    this._getSortedPushesArray(updatedPushes).forEach(callback);
>+  },

You updated the UI to not depend on a sorted push list anymore, so why sort it?

>+        for (var i = 0; i < data[prBranchName][rev].length; i++) {

In multiple places: for var i in please

>+  _addFinishedResultToPush: function Data__addFinishedResultToPush(result, toWhichPushes, updatedPushes) {
[…]
>+    if (result.state == "success") {
[…]
>+      // Make sure the ETA on running builds gets updated.
>+      this._accumulatePushesWithRunningBuildsOnMachine(machine, updatedPushes);

See comment above.

>+  _addResultToPush: function Data__addResultToPush(result) {
[…]
>+    if (push.results[machine.os][debug][group].some(function (r) { return r.runID == result.runID})) {
>+      console.log("double-adding run!");
>+    }

This should be removed.

Getting this logic is quite complex, I’m not sure I got it all right. Please clear up the questions first.

Comment 9

[Arpad Borsos [:Swatinem]]

Comment on attachment 498103 [details] [diff] [review]
part 4, v2

This takes care of one of my comments on the previous patch, very well.

Comment 10

[Markus Stange [:mstange]]

Attachment: part 1, v3

(In reply to comment #4)
> Comment on attachment 498083 [details] [diff] [review]
> part 1, v2
> 
> >   _statusCallback: null,
> >-  _refreshCallback: null,
> >+  _updatedPushCallback: null,
> >+  _infraStatsCallback: null,
> >+  _initialPushlogLoadCallback: null,
> 
> >     this._statusCallback = uiConf.status;
> >-    this._refreshCallback = uiConf.refresh;
> >+    this._updatedPushCallback = uiConf.handleUpdatedPush;
> >+    this._infraStatsCallback = uiConf.handleInfraStatsUpdate;
> >+    this._initialPushlogLoadCallback = uiConf.handleInitialPushlogLoad;
> 
> I think saving a reference to the whole uiConf object makes this code easier.

Good idea, done.

> 
> >-    return {status: self.updateStatus, refresh: function (pushes, infraStats) { self.handleRefresh(pushes, infraStats); } };
> >+    return {
> >+      status: function (status) {
> >+        self.updateStatus(status);
> >+      },
> >+      handleUpdatedPush: function (push) {
> >+        self.handleUpdatedPush(push);
> >+      },
> >+      handleInfraStatsUpdate: function (infraStats) {
> >+        self.handleInfraStatsUpdate(infraStats);
> >+      },
> >+      handleInitialPushlogLoad: function () {
> >+        self.handleInitialPushlogLoad();
> >+      },
> >+    };
> 
> You need all this indirection because otherwise the "this" reference will be
> messed up, right?

Correct.

> 
> >+  handleInitialPushlogLoad: function UserInterface_handleInitialPushlogLoad() {
> >+    $("#pushes").removeClass("initialload");
> >   },
> 
> You can as well inline this trivial function.

Done.

Comment 11

[Markus Stange [:mstange]]

(In reply to comment #8)
> Also, can you directly
> pass the loadFailed function as argument (haven’t tested it, but should work
> without messing up "this")

I don't think that would work. Anyway, part 4 solves it differently.

> >+        if (pendingOrRunning == "running")
> >+          self._discardPendingRunsThatAreAlreadyRunning(updatedPushes);
> >+        else
> >+          self._discardPendingRunsThatAreAlreadyRunning();
> 
> You removeResultFromPush() in both cases but only add it to the updated pushes
> in one case, I don’t understand why?

The idea is that removeResultFromPush() won't do anything in one case, since it won't find the pending result on the push, since the pending results have only just arrived and not linked with the pushes yet. So then there's no need to updated the pushes.

Maybe instead of having _discardPendingRunsThatAreAlreadyRunning update updatedPushes, we should pass updatedPushes into _removeResultFromPush, which then only adds the push to updatedPushes if it really removed something. That would make this clearer, I think.

> 
> >+  _accumulatePushesWithRunningBuildsOnMachine: function Data__accumulatePushesWithRunningBuildsOnMachine(machine, updatedPushes) {
> >+    for (var repo in this._runningJobs) {
> >+      for (var rev in this._runningJobs[repo]) {
> >+        if (!(rev in this._pushes) || (rev in updatedPushes))
> >+          continue;
> >+        for (var i = 0; i < this._runningJobs[repo][rev].length; i++) {
> >+          var runningResult = this._runningJobs[repo][rev][i];
> >+          if (machine == this.getMachine(runningResult.buildername)) {
> >+            updatedPushes[rev] = this._pushes[rev];
> >+            break;
> >+          }
> >+        }
> >+      }
> >+    }
> >+  },
> 
> This function is called with the comment
> // Make sure the ETA on running builds gets updated.
> above it, inside addFinishedResultToPush() when the result is successful.
> This does not seem quite right.

It's called from the point where we update machine.averageCycleTime. Changing averageCycleTime should be reflected in the UI in the tooltip of running jobs, and we trigger this update by updating the corresponding pushes.

Does it make sense now, or am I missing something? Should I add another comment there?

> >+  _getSortedPushesArray: function Data__getSortedPushesArray(unsortedPushesObject) {
> >+    var pushes = Controller.valuesFromObject(unsortedPushesObject);
> >+    pushes.sort(function(a,b) { return a.date - b.date; });
> >+    return pushes;
> >+  },
> >+
> >+  _notifyUpdatedPushes: function Data__notifyUpdatedPushes(updatedPushes, callback) {
> >+    this._getSortedPushesArray(updatedPushes).forEach(callback);
> >+  },
> 
> You updated the UI to not depend on a sorted push list anymore, so why sort it?

You're right, at this point the sorting is unnecessary. But starting with part 2 in bug 619673, it causes us to request the results for recent pushes first.

> >+        for (var i = 0; i < data[prBranchName][rev].length; i++) {
> 
> In multiple places: for var i in please

http://andrewdupont.net/2006/05/18/javascript-associative-arrays-considered-harmful/#comment-508

I can use forEach instead, if you prefer that.

> >+  _addResultToPush: function Data__addResultToPush(result) {
> […]
> >+    if (push.results[machine.os][debug][group].some(function (r) { return r.runID == result.runID})) {
> >+      console.log("double-adding run!");
> >+    }
> 
> This should be removed.

Oops.

Comment 12

[Arpad Borsos [:Swatinem]]

Comment on attachment 498097 [details] [diff] [review]
part 3, v1

>+  _removeResultFromPush: function Data__removeResultFromPush(result) {
>+    var push = result.push;
>+    var machine = result.machine;
>+    var debug = machine.debug ? "debug" : "opt";
>+    var group = this.machineGroup(machine.type);
>+    var grouparr = push.results[machine.os][debug][group]

There is a missing semicolon.

Comment 13

[Arpad Borsos [:Swatinem]]

(In reply to comment #11)
> The idea is that removeResultFromPush() won't do anything in one case, since it
> won't find the pending result on the push, since the pending results have only
> just arrived and not linked with the pushes yet. So then there's no need to
> updated the pushes.
> 
> Maybe instead of having _discardPendingRunsThatAreAlreadyRunning update
> updatedPushes, we should pass updatedPushes into _removeResultFromPush, which
> then only adds the push to updatedPushes if it really removed something. That
> would make this clearer, I think.

Yes, I think that would make it more clear.

> 
> > 
> > >+  _accumulatePushesWithRunningBuildsOnMachine: function Data__accumulatePushesWithRunningBuildsOnMachine(machine, updatedPushes) {
> > >+    for (var repo in this._runningJobs) {
> > >+      for (var rev in this._runningJobs[repo]) {
> > >+        if (!(rev in this._pushes) || (rev in updatedPushes))
> > >+          continue;
> > >+        for (var i = 0; i < this._runningJobs[repo][rev].length; i++) {
> > >+          var runningResult = this._runningJobs[repo][rev][i];
> > >+          if (machine == this.getMachine(runningResult.buildername)) {
> > >+            updatedPushes[rev] = this._pushes[rev];
> > >+            break;
> > >+          }
> > >+        }
> > >+      }
> > >+    }
> > >+  },
> > 
> > This function is called with the comment
> > // Make sure the ETA on running builds gets updated.
> > above it, inside addFinishedResultToPush() when the result is successful.
> > This does not seem quite right.
> 
> It's called from the point where we update machine.averageCycleTime. Changing
> averageCycleTime should be reflected in the UI in the tooltip of running jobs,
> and we trigger this update by updating the corresponding pushes.
> 
> Does it make sense now, or am I missing something? Should I add another comment
> there?

Ok, it makes sense with this comment, you should add a comment to make it clear that the update is to handle changing averageCycleTimes.
Also, in that case the break is wrong, you want to update all the running jobs for that machine, not just the first one.

> > >+        for (var i = 0; i < data[prBranchName][rev].length; i++) {
> > 
> > In multiple places: for var i in please
> 
> http://andrewdupont.net/2006/05/18/javascript-associative-arrays-considered-harmful/#comment-508
> 
> I can use forEach instead, if you prefer that.

Well its fine this way. But I like using for in if you know what you are doing.

Comment 14

[Arpad Borsos [:Swatinem]]

(In reply to comment #13)
> Also, in that case the break is wrong, you want to update all the running jobs
> for that machine, not just the first one.

Oh wait, one per rev is enough. Ignore that comment.

Comment 15

[Markus Stange [:mstange]]

Attachment: part 3, v2

Comment 16

[Markus Stange [:mstange]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fdf20aed8ef9
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/9313d6f25465
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/6fd81008efeb
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/0296ee0669de