Improve Item redraw and initial draw performance #3475
Conversation
yotamberk
changed the title
yotamberk
changed the title
| var groupCount = 20; | ||
| var itemCount = 400; | ||
|
|
||
| function genName(length) { |
There was a problem hiding this comment.
I would expect a short description around here, about what this example is about.
There was a problem hiding this comment.
Previous comment is misplaced; it should have been in the HTML section.
| // create a data set with groups | ||
| var groups = new vis.DataSet(); | ||
| for (var g = 0; g < groupCount; g++) { | ||
| groups.add({ |
There was a problem hiding this comment.
Indent, we're sort of standardized on two spaces, can we keep it like that? Comment applies to whole file.
| @@ -0,0 +1,78 @@ | |||
| <!DOCTYPE HTML> | |||
There was a problem hiding this comment.
I tested this example, works fine. So I won't put too much effort in trying to understand the details.
| var container = document.getElementById('visualization'); | ||
| var options = { | ||
| width: '100%', | ||
| //autoResize: true, |
There was a problem hiding this comment.
Any reason why this commented stuff is here? If so, add a clarification. If not, remove.
| //groupOrder: 'content', | ||
| }; | ||
|
|
||
| var timeline = new vis.Timeline(container); |
There was a problem hiding this comment.
If this is a stress-test, as the name implies, how about adding timing here? To understand the gain.
| this.dom.box.className = this.baseClassName + className; | ||
| } | ||
| } | ||
| BackgroundItem.prototype._getDomComponentsSizes = function() { |
There was a problem hiding this comment.
Put empty lines between methods. Please review the whole PR for this.
| // recalculate size | ||
| this.props.content.width = this.dom.content.offsetWidth; | ||
| this.height = 0; // set height zero, so this item will be ignored when stacking items | ||
| BackgroundItem.prototype._repaintDomAdditionals = function() { |
There was a problem hiding this comment.
Why is the empty method needed?
There was a problem hiding this comment.
because all the other items contain 6 actions, I wanted to insure that all items have the same number of actions (and that the same function type is done in parallel - this is a preparation to making the redraw queue for all the items defined in the Item.js prototype) + it's a space keeper for future implementations of dom additionals to background items (I've been planning to add)
|
|
||
| (function() { | ||
| if (this.dirty) { | ||
| sizes = this._getDomComponentsSizes.bind(this)(); |
There was a problem hiding this comment.
? Why the bind here? Isn't it bound already (after function def)?
There was a problem hiding this comment.
because _getDomComponentsSizes needs the this
| * @return {boolean} the redraw queue if returnQueue=true | ||
| */ | ||
| RangeItem.prototype.redraw = function(returnQueue) { | ||
| var sizes |
There was a problem hiding this comment.
; - If you had DRY'd this before, you'd need to change it only once.
| * @param {boolean} [returnQueue=false] return the queue | ||
| * @return {boolean} the redraw queue if returnQueue=true | ||
| */ | ||
| RangeItem.prototype.redraw = function(returnQueue) { |
There was a problem hiding this comment.
Strike four: Image Samuel L. Jackson holding a gun against your head and what he would say about DRY-ing this.
| // update dirty DOM. An item is marked dirty when: | ||
| // - the item is not yet rendered | ||
| // - the item's data is changed | ||
| // - the item is selected/deselected |
There was a problem hiding this comment.
That's actually not what I meant. I meant a comment block like at line 132. Oh well, at least you DRY'd it.
Edit: No, you didn't. You just moved the blocks. But it's in one of the duplicated parts so I guess it will work out in the end.
|
As a reviewer, I'm not too happy with the resolution of this issue. I really do think that 4xduplication is a bug. But I do realize that you're anxious to get this in.....and I already said that the fix is good. |
|
@wimrijnders Thanks. |
|
@yotamberk @wimrijnders Since I've been working with the Here is the old method. If I revert back to just this (leaving all other changes intact) then the vertical positioning is stable again: util.forEach(this.items, function (item, key) {
if (!item.displayed) {
item.redraw();
me.visibleItems.push(item);
}
item.repositionX(limitSize);
});If I make one adjustment to this to move back towards the code from the PR, the vertical position starts jumping around: util.forEach(this.items, function (item, key) {
if (!item.displayed) {
item.redraw();
me.visibleItems.push(item);
}
})
for (var i = 0; i < this.items.length; i++) {
this.items[i].repositionX(limitSize);
} |
|
Great! Can you please submit a PR with this revert? |
* Make items redraws return queues * Parallel initial items redraw * Seperate read and write actions in items * Parallel all items redraws * Remove comments * Fix linting comments * Fix redraws on actions * Add stress example * Fix example files * Explain and fix example * Fix comment issues
Based on the work done in #3409, I've improved the performance of the timeline redraw time by 2-5 times faster! Initial draw has been reduced to less than 1 seconds for 1,000 items across 10 groups.
This PR includes parallel redrawing of items in the same group avoiding "read,write,read,write" reflow/repaint delays of the browser, reducing it to a single repaint per all items' action in redrawing queue.
Further work needs to been done for:
Item.jsfile (since the redraw function is identical to all.)