Skip to content
This repository was archived by the owner on Jul 29, 2019. It is now read-only.

Group redraw performance #3409

Merged
merged 8 commits into from
Sep 18, 2017
Merged

Group redraw performance #3409

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0b4dc73
Implement group redraw @grimalschi performance enhancement
yotamberk Sep 2, 2017
845a01a
Fix JSDoc for redraw
yotamberk Sep 2, 2017
6b8e2b3
Remove commented out in itemset
yotamberk Sep 2, 2017
329d981
Remove fasdom
yotamberk Sep 2, 2017
c00ebf2
Clean up queue functions in group redraw
yotamberk Sep 16, 2017
3ed7477
Fix mistake in comments
yotamberk Sep 16, 2017
d7b770c
Remove extra read-write from _didResize
yotamberk Sep 16, 2017
6dba6bd
Resolve review comments
yotamberk Sep 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
239 changes: 140 additions & 99 deletions lib/timeline/component/Group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,119 +212,160 @@ Group.prototype.getLabelWidth = function() {
* @param {{start: number, end: number}} range
* @param {{item: {horizontal: number, vertical: number}, axis: number}} margin
* @param {boolean} [forceRestack=false] Force restacking of all items
* @return {boolean} Returns true if the group is resized
* @param {boolean} [returnQueue=false] return the queue or the result
* @return {boolean} Returns true if the group is resized or the redraw queue if returnQueue=true
*/
Group.prototype.redraw = function(range, margin, forceRestack) {
var resized = false;

// force recalculation of the height of the items when the marker height changed
// (due to the Timeline being attached to the DOM or changed from display:none to visible)
var markerHeight = this.dom.marker.clientHeight;
if (markerHeight != this.lastMarkerHeight) {
this.lastMarkerHeight = markerHeight;
util.forEach(this.items, function (item) {
item.dirty = true;
if (item.displayed) item.redraw();
});

forceRestack = true;
}

// recalculate the height of the subgroups
this._calculateSubGroupHeights(margin);

// calculate actual size and position
var foreground = this.dom.foreground;
this.top = foreground.offsetTop;
this.right = foreground.offsetLeft;
this.width = foreground.offsetWidth;

var lastIsVisible = this.isVisible;
this.isVisible = this._isGroupVisible(range, margin);

var restack = forceRestack || this.stackDirty || (this.isVisible && !lastIsVisible);

this._updateSubgroupsSizes();
// if restacking, reposition visible items vertically
if(restack) {
if (typeof this.itemSet.options.order === 'function') {
// a custom order function
// brute force restack of all items

// show all items
var me = this;
var limitSize = false;
util.forEach(this.items, function (item) {
if (!item.dom) { // If this item has never been displayed then the dom property will not be defined, this means we need to measure the size
item.redraw();
me.visibleItems.push(item);
Group.prototype.redraw = function(range, margin, forceRestack, returnQueue) {
var queue = [];
Copy link
Contributor

@macleodbroad-wf macleodbroad-wf Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If pure performance is the concern, I think it would be more readable and slightly more performant (maybe a micro optimization?) to have all of these anonymous functions defined at a higher level, and then simply:

var queue = [recalcHeight(this), recalcSubGroupHeights(this), calcSizeAndPosition, etc...];
if (returnQueue) {
    return queue;
} else {
    var result;
    queue.forEach(function (fn) {
        result = fn(this);
    });
    return result;
}
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree but I was planning doing it in another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

How often does redraw get called?

Copy link
Member Author

Choose a reason for hiding this comment

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

every zoom/scroll/drag - alot

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, can you include a reference to the follow-up issue? Just want to make sure it doesn't get lost.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can include it here if you think it's really important.
Wait with this PR if so

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth doing, given how often this is used.


queue.push((function () {
// force recalculation of the height of the items when the marker height changed
// (due to the Timeline being attached to the DOM or changed from display:none to visible)
var markerHeight = this.dom.marker.clientHeight;
if (markerHeight != this.lastMarkerHeight) {
this.lastMarkerHeight = markerHeight;
util.forEach(this.items, function (item) {
item.dirty = true;
if (item.displayed) item.redraw();
});

forceRestack = true;
}
}).bind(this));

queue.push((function () {
// recalculate the height of the subgroups
this._updateSubGroupHeights(margin);
}).bind(this));

queue.push((function () {
// calculate actual size and position
var foreground = this.dom.foreground;
this.top = foreground.offsetTop;
this.right = foreground.offsetLeft;
this.width = foreground.offsetWidth;
}).bind(this));

var resized;
var lastIsVisible;

queue.push((function () {
resized = false;
lastIsVisible = this.isVisible;
this.isVisible = this._isGroupVisible(range, margin);
}).bind(this));

queue.push((function () {
var restack = forceRestack || this.stackDirty || this.isVisible && !lastIsVisible;

// if restacking, reposition visible items vertically
if (restack) {
if (typeof this.itemSet.options.order === 'function') {
// a custom order function
// brute force restack of all items

// show all items
var me = this;
var limitSize = false;
util.forEach(this.items, function (item) {
if (!item.displayed) {
item.redraw();
me.visibleItems.push(item);
}
item.repositionX(limitSize);
});

// order all items and force a restacking
var customOrderedItems = this.orderedItems.byStart.slice().sort(function (a, b) {
return me.itemSet.options.order(a.data, b.data);
});
stack.stack(customOrderedItems, margin, true /* restack=true */);
this.visibleItems = this._updateItemsInRange(this.orderedItems, this.visibleItems, range);
} else {
// no custom order function, lazy stacking
this.visibleItems = this._updateItemsInRange(this.orderedItems, this.visibleItems, range);

if (this.itemSet.options.stack) {
// TODO: ugly way to access options...
stack.stack(this.visibleItems, margin, true /* restack=true */);
} else {
// no stacking
stack.nostack(this.visibleItems, margin, this.subgroups, this.itemSet.options.stackSubgroups);
}
}
item.repositionX(limitSize);
});

// order all items and force a restacking
var customOrderedItems = this.orderedItems.byStart.slice().sort(function (a, b) {
return me.itemSet.options.order(a.data, b.data);
});
stack.stack(customOrderedItems, margin, true /* restack=true */);
this.visibleItems = this._updateItemsInRange(this.orderedItems, this.visibleItems, range);

}
else {
// no custom order function, lazy stacking
this.visibleItems = this._updateItemsInRange(this.orderedItems, this.visibleItems, range);

if (this.itemSet.options.stack) { // TODO: ugly way to access options...
stack.stack(this.visibleItems, margin, true /* restack=true */);
this.stackDirty = false;
}
else { // no stacking
stack.nostack(this.visibleItems, margin, this.subgroups, this.itemSet.options.stackSubgroups);
}).bind(this));

var height;

queue.push((function () {
this._updateSubgroupsSizes();
}).bind(this));

queue.push((function () {
// recalculate the height of the group
height = this._calculateHeight(margin);
}).bind(this));

queue.push((function () {
// calculate actual size and position
var foreground = this.dom.foreground;
this.top = foreground.offsetTop;
this.right = foreground.offsetLeft;
this.width = foreground.offsetWidth;
}).bind(this));

queue.push((function () {
resized = util.updateProperty(this, 'height', height) || resized;
// recalculate size of label
resized = util.updateProperty(this.props.label, 'width', this.dom.inner.clientWidth) || resized;
resized = util.updateProperty(this.props.label, 'height', this.dom.inner.clientHeight) || resized;
}).bind(this));

queue.push((function () {
// apply new height
this.dom.background.style.height = height + 'px';
this.dom.foreground.style.height = height + 'px';
this.dom.label.style.height = height + 'px';
}).bind(this));

queue.push((function () {
// update vertical position of items after they are re-stacked and the height of the group is calculated
for (var i = 0, ii = this.visibleItems.length; i < ii; i++) {
var item = this.visibleItems[i];
item.repositionY(margin);
if (!this.isVisible && this.groupId != "__background__") {
if (item.displayed) item.hide();
}
}
}

this.stackDirty = false;
}
// recalculate the height of the group
var height = this._calculateHeight(margin);

// calculate actual size and position
foreground = this.dom.foreground;
this.top = foreground.offsetTop;
this.right = foreground.offsetLeft;
this.width = foreground.offsetWidth;
resized = util.updateProperty(this, 'height', height) || resized;
// recalculate size of label
resized = util.updateProperty(this.props.label, 'width', this.dom.inner.clientWidth) || resized;
resized = util.updateProperty(this.props.label, 'height', this.dom.inner.clientHeight) || resized;

// apply new height
this.dom.background.style.height = height + 'px';
this.dom.foreground.style.height = height + 'px';
this.dom.label.style.height = height + 'px';

// update vertical position of items after they are re-stacked and the height of the group is calculated
for (var i = 0, ii = this.visibleItems.length; i < ii; i++) {
var item = this.visibleItems[i];
item.repositionY(margin);
if (!this.isVisible && this.groupId != "__background__") {
if (item.displayed) item.hide();
}
}

if (!this.isVisible && this.height) {
return resized = false;
}
if (!this.isVisible && this.height) {
Copy link

Choose a reason for hiding this comment

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

Tabs?

return resized = false;
}

return resized;
};
return resized;
}).bind(this));

if (returnQueue) {
return queue;
} else {
var result;
queue.forEach(function (fn) {
result = fn();
});
return result;
}
};

/**
* recalculate the height of the subgroups
*
* @param {{item: vis.Item}} margin
* @private
*/
Group.prototype._calculateSubGroupHeights = function (margin) {
Group.prototype._updateSubGroupHeights = function (margin) {
if (Object.keys(this.subgroups).length > 0) {
var me = this;

Expand Down
36 changes: 30 additions & 6 deletions lib/timeline/component/ItemSet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,13 +681,37 @@ ItemSet.prototype.redraw = function() {
// redraw the background group
this.groups[BACKGROUND].redraw(range, nonFirstMargin, forceRestack);

// redraw all regular groups
util.forEach(this.groups, function (group) {
var groupMargin = (group == firstGroup) ? firstMargin : nonFirstMargin;
var groupResized = group.redraw(range, groupMargin, forceRestack);
resized = groupResized || resized;
height += group.height;
var redrawQueue = {};
var redrawQueueLength = 0;

// collect redraw functions
util.forEach(this.groups, function (group, key) {
if (key === BACKGROUND) return;
var groupMargin = group == firstGroup ? firstMargin : nonFirstMargin;
var returnQueue = true;
redrawQueue[key] = group.redraw(range, groupMargin, forceRestack, returnQueue);
redrawQueueLength = redrawQueue[key].length;
});

if (redrawQueueLength) {
var redrawResults = {};

for (var i = 0; i < redrawQueueLength; i++) {
util.forEach(redrawQueue, function (fns, key) {
Copy link

Choose a reason for hiding this comment

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

Oof this stuff is almost over the top of my head.

But shouldn't this be: util.forEach(redrawQueue[i]...?

redrawQueue is a hash, of which every value is a queue of functions to run. I'm inferring that you want to run all function in the queue for every key of redrawQueue.

If I got it wrong here, please explain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Im sorry for bad naming :(

group.redraw(range, groupMargin, forceRestack, returnQueue) returns array of functions.

So redrawQueue is an queue, and every element in this queue is array of functions.

And when we release this queue, we call all functions from every element of queue.

Copy link

Choose a reason for hiding this comment

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

Nice to see you react to a question on the code. I was expecting @yotamberk, but I understand the original comes from you.

Will try to wrap my brain around it again. Perhaps some commenting would be in order here.

Copy link

Choose a reason for hiding this comment

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

Ah, I think I got it. redrawQueue values are all arrays of the same size, containing functions.
In the loop, the first functions in the arrays of all redrawQueue values are called, then the second, etc.

The logic of this escapes me, but there must be a good reason for it. Perhaps I should not be reviewing.

redrawResults[key] = fns[i]();
Copy link

Choose a reason for hiding this comment

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

But then the [i] does not make sense. Why go through all the trouble of creating a Queue instance, when the separate items in it are meant for set individual items?

});
}

// redraw all regular groups
util.forEach(this.groups, function (group, key) {
if (key === BACKGROUND) return;
var groupResized = redrawResults[key];
resized = groupResized || resized;
height += group.height;
});
height = Math.max(height, minHeight);
}

height = Math.max(height, minHeight);

// update frame height
Expand Down