Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

Speed improvements #486

Closed
wants to merge 4 commits into from
Closed

Speed improvements #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
134db97
Update keys inline, since none of the fancy features of angular.exten…
fitzgen Apr 26, 2015
81832ec
Don't concatenate and create a new array, just push the items to the …
fitzgen Apr 26, 2015
0e582c3
Avoid underscore's map and object methods in ThJobModel.get_list. Les…
fitzgen Apr 26, 2015
3734b60
jQuery.event.fix shouldn't access properties (and trigger sync reflow…
fitzgen Apr 26, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 22 additions & 9 deletions webapp/app/js/models/job.js
View file
Open in desktop
Expand Up @@ -10,9 +10,11 @@ treeherder.factory('ThJobModel', [
// ThJobModel is the js counterpart of job

var ThJobModel = function(data) {
// creates a new instance of ThJobModel
// using the provided properties
angular.extend(this, data);
if (typeof data === "object" && data !== null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a comment here to explain why we're not using angular.extend, otherwise someone might be tempted to change it back (since the original form is more concise).

for (var k in data) {
this[k] = data[k];
}
}
};

ThJobModel.prototype.get_current_eta = function(){
Expand Down Expand Up @@ -55,19 +57,30 @@ treeherder.factory('ThJobModel', [
if(_.has(response.data, 'job_property_names')){
// the results came as list of fields
//we need to convert them to objects
item_list = _.map(response.data.results, function(elem){
var job_obj = _.object(response.data.job_property_names, elem);
return new ThJobModel(job_obj);
});
item_list = [];
var props = response.data.job_property_names;
var propsLen = props.length;
for (var i = 0, resultsLen = response.data.results.length; i < propsLen; i++) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition to break the loop should be i < resultsLen
I think this is the reason why the tests are failing. You probably don't see the problem if you look at mozilla-central, since the number of jobs per result set is > 36. On try you will likely see errors in the console.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before breaking this stuff out into loops (and making the code much less readable), I wonder if we should consider using lodash (https://lodash.com/) instead of underscore. It's supposed to be quite a bit faster than underscore (http://benmccormick.org/2014/11/12/underscore-vs-lodash/), and indeed uses the technique you're using here to iterate through arrays with map (and other things): https://github.com/lodash/lodash/blob/master/lodash.js#L1443

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition to break the loop should be i < resultsLen
I think this is the reason why the tests are failing.

Yes! Thanks! All passing now.

var model = new ThJobModel();
var elem = response.data.results[i];
for (var j = 0; j < propsLen; j++) {
model[props[j]] = elem[j];
}
item_list.push(model);
}
}else{
item_list = _.map(response.data.results, function(job_obj){
return new ThJobModel(job_obj);
});
}

// next_pages_jobs is wrapped in a $q.when call because it could be
// either a promise or a value
return $q.when(next_pages_jobs).then(function(maybe_job_list){
return item_list.concat(maybe_job_list);
return $q.when(next_pages_jobs).then(function(maybe_job_list) {
for (var i = 0, n = maybe_job_list.length; i < n; i++) {
item_list.push(maybe_job_list[i]);
}
return item_list;
})
});
};
Expand Down
16 changes: 13 additions & 3 deletions webapp/app/vendor/jquery-2.0.3.js
View file
Open in desktop
Expand Up @@ -4789,7 +4789,7 @@ jQuery.event = {
}

// Create a writable copy of the event object and normalize some properties
var i, prop, copy,
var i, copy,
type = event.type,
originalEvent = event,
fixHook = this.fixHooks[ type ];
Expand All @@ -4806,8 +4806,18 @@ jQuery.event = {

i = copy.length;
while ( i-- ) {
prop = copy[ i ];
event[ prop ] = originalEvent[ prop ];
(function () {
var prop = copy[ i ];
var ev = event;
var oe = originalEvent;
Object.defineProperty(ev, prop, {
configurable: true,
get: function () { return oe[prop]; },
set: function (v) {
Object.defineProperty(ev, prop, { value: v });
},
});
}());
}

// Support: Cordova 2.5 (WebKit) (#13255)
Expand Down