Skip to content
This repository has been 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 all 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
159 changes: 108 additions & 51 deletions lib/timeline/component/Group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,47 +206,30 @@ Group.prototype.getLabelWidth = function() {
return this.props.label.width;
};


/**
* Repaint this group
* @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
*/
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)
Group.prototype._didMarkerHeightChange = function() {
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();
});
return true;
}
}

forceRestack = true;
}

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

// calculate actual size and position
Group.prototype._calculateGroupSizeAndPosition = function() {
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);
Group.prototype._redrawItems = function(forceRestack, lastIsVisible, margin, range) {
var restack = forceRestack || this.stackDirty || this.isVisible && !lastIsVisible;

this._updateSubgroupsSizes();
// if restacking, reposition visible items vertically
if(restack) {
// 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
Expand All @@ -255,7 +238,7 @@ Group.prototype.redraw = function(range, margin, forceRestack) {
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
if (!item.displayed) {
item.redraw();
me.visibleItems.push(item);
}
Expand All @@ -268,41 +251,41 @@ Group.prototype.redraw = function(range, margin, forceRestack) {
});
stack.stack(customOrderedItems, margin, true /* restack=true */);
this.visibleItems = this._updateItemsInRange(this.orderedItems, this.visibleItems, range);

}
else {
} 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
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);
}
}

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;
Group.prototype._didResize = function(resized, height) {
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;
var labelWidth = this.dom.inner.clientWidth;
var labelHeight = this.dom.inner.clientHeight;
resized = util.updateProperty(this.props.label, 'width', labelWidth) || resized;
resized = util.updateProperty(this.props.label, 'height', labelHeight) || resized;
return resized;
}

// apply new height
this.dom.background.style.height = height + 'px';
this.dom.foreground.style.height = height + 'px';
Group.prototype._applyGroupHeight = function(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
// update vertical position of items after they are re-stacked and the height of the group is calculated
Group.prototype._updateItemsVerticalPosition = function(resized, margin) {
for (var i = 0, ii = this.visibleItems.length; i < ii; i++) {
var item = this.visibleItems[i];
item.repositionY(margin);
Expand All @@ -312,10 +295,84 @@ Group.prototype.redraw = function(range, margin, forceRestack) {
}

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

return resized;
}

/**
* Repaint this group
* @param {{start: number, end: number}} range
* @param {{item: {horizontal: number, vertical: number}, axis: number}} margin
* @param {boolean} [forceRestack=false] Force restacking of all items
* @param {boolean} [returnQueue=false] return the queue or if the group resized
* @return {boolean} Returns true if the group is resized or the redraw queue if returnQueue=true
*/
Group.prototype.redraw = function(range, margin, forceRestack, returnQueue) {
var resized = false;
var lastIsVisible = this.isVisible;
var height;

var queue = [
// 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)
(function () {
forceRestack = this._didMarkerHeightChange.bind(this);
}).bind(this),

// recalculate the height of the subgroups
this._updateSubGroupHeights.bind(this, margin),

// calculate actual size and position
this._calculateGroupSizeAndPosition.bind(this),

// check if group is visible
(function() {
this.isVisible = this._isGroupVisible.bind(this)(range, margin);
}).bind(this),

// redraw Items if needed
(function() {
this._redrawItems.bind(this)(forceRestack, lastIsVisible, margin, range)
}).bind(this),

// update subgroups
this._updateSubgroupsSizes.bind(this),

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

// calculate actual size and position again
this._calculateGroupSizeAndPosition.bind(this),

// check if resized
(function() {
resized = this._didResize.bind(this)(resized, height)
}).bind(this),

// apply group height
(function() {
this._applyGroupHeight.bind(this)(height)
}).bind(this),

// update vertical position of items after they are re-stacked and the height of the group is calculated
(function() {
return resized = this._updateItemsVerticalPosition.bind(this)(resized, margin)
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding: this is the last function in the queue and it returns something. The return value is collected here. Is this the intention?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the assignment to resize isn't really necessary for the return, but it's kept in for completeness. I guess this is OK (I would probably do the same).

}).bind(this)
]

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

/**
Expand All @@ -324,7 +381,7 @@ Group.prototype.redraw = function(range, margin, forceRestack) {
* @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
Contributor

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
Contributor

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
Contributor

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
Contributor

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