Group redraw performance #3409

yotamberk · 2017-09-02T14:31:32Z

Addressing some of the group performance issues and implementation of discussion from #3248 (using gist https://gist.github.com/grimalschi/df9a44ac12b5420e0cc6f2d42f1febed/revisions#diff-32e31f368549caca635e81cdb8a0a82dR21132)
This is the best implementation in the meantime - I've tried using fasdom and some other methods but concluded that it's not what we need. Fastdom allows prioritization of "read" operations over "write" operations, whilst our problem has to do with paralleling the operations done in a group redraw for all groups.
Further analysis that will be done:

The exact operations done in the group redraw - we might be able to use fasdom/combining "reads" and "writes" to fewer operations for every redraw queue.
Implementation like this should also be done in items' redraws.

Shout out to @grimalschi for supplying the solution.

yotamberk · 2017-09-03T17:18:37Z

@wimrijnders @macleodbroad-wf can one of you please review and approve this? It is extremely important

grimalschi · 2017-09-03T17:38:16Z

Actually I did not think you will use my code :)

That was just demo to show the idea.

yotamberk · 2017-09-03T17:41:56Z

if the shoe fits, wear it.

The code is good. It might be written in a cleaner manner but for now it's good. I've tried using fasdom and some other techniques but always came back to your code as being clearer than what I did. I'm working on further work on items redraw. I hope to have anther refactor iteration to clean this code

macleodbroad-wf · 2017-09-03T18:20:13Z

lib/timeline/component/Group.js

-          item.redraw();
-          me.visibleItems.push(item);
+Group.prototype.redraw = function(range, margin, forceRestack, returnQueue) {
+  var queue = [];


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; } ...

I agree but I was planning doing it in another PR

How often does redraw get called?

every zoom/scroll/drag - alot

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

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

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

wimrijnders · 2017-09-04T06:04:48Z

lib/timeline/component/ItemSet.js

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


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.

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.

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.

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.

wimrijnders · 2017-09-04T06:06:31Z

lib/timeline/component/ItemSet.js

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


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?

wimrijnders · 2017-09-04T06:07:23Z

lib/timeline/component/Group.js

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


wimrijnders

Just react to the comments for my understanding.

wimrijnders · 2017-09-05T12:31:43Z

lib/timeline/component/ItemSet.js

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


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.

yotamberk · 2017-09-16T16:29:21Z

I've reacted to the comments. Please address.
@wimrijnders @macleodbroad-wf

wimrijnders · 2017-09-18T06:30:21Z

lib/timeline/component/Group.js

+}
+
+Group.prototype._updateItemsVerticalPosition = function(resized, margin) {
+  // update vertical position of items after they are re-stacked and the height of the group is calculated


Might be better as function header comment

wimrijnders · 2017-09-18T06:30:48Z

lib/timeline/component/Group.js

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


Why this construct? I think return false would be enough here

wimrijnders · 2017-09-18T06:33:51Z

lib/timeline/component/Group.js

+  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


wimrijnders · 2017-09-18T06:38:59Z

lib/timeline/component/Group.js

+
+    // 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)


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?

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).

wimrijnders

Overall a big improvement in readability and therefore understanding. Just some small remarks.

yotamberk · 2017-09-18T07:38:50Z

@wimrijnders done! Thanks!

This reverts commit 9830616.

@grimalschi

* Implement group redraw @grimalschi performance enhancement * Fix JSDoc for redraw * Remove commented out in itemset * Remove fasdom * Clean up queue functions in group redraw * Fix mistake in comments * Remove extra read-write from _didResize * Resolve review comments

yotamberk added 4 commits September 2, 2017 17:18

Implement group redraw @grimalschi performance enhancement

0b4dc73

Fix JSDoc for redraw

845a01a

Remove commented out in itemset

6b8e2b3

Remove fasdom

329d981

yotamberk added PRIORITY Timeline labels Sep 3, 2017

macleodbroad-wf reviewed Sep 3, 2017

View reviewed changes

wimrijnders reviewed Sep 4, 2017

View reviewed changes

wimrijnders suggested changes Sep 4, 2017

View reviewed changes

wimrijnders previously approved these changes Sep 14, 2017

View reviewed changes

Clean up queue functions in group redraw

c00ebf2

yotamberk dismissed wimrijnders’s stale review via c00ebf2 September 16, 2017 16:26

yotamberk added 2 commits September 16, 2017 19:40

Fix mistake in comments

3ed7477

Remove extra read-write from _didResize

d7b770c

wimrijnders reviewed Sep 18, 2017

View reviewed changes

wimrijnders suggested changes Sep 18, 2017

View reviewed changes

Resolve review comments

6dba6bd

wimrijnders approved these changes Sep 18, 2017

View reviewed changes

yotamberk merged commit 9830616 into almende:develop Sep 18, 2017

yotamberk added a commit that referenced this pull request Sep 18, 2017

Revert "Group redraw performance (#3409)"

d9e407b

This reverts commit 9830616.

This was referenced Sep 18, 2017

Revert "Group redraw performance" #3451

Closed

Improve Item redraw and initial draw performance #3475

Merged

wimrijnders mentioned this pull request Sep 28, 2017

Timeline: Performance issues #3248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group redraw performance #3409

Group redraw performance #3409

yotamberk commented Sep 2, 2017

yotamberk commented Sep 3, 2017

grimalschi commented Sep 3, 2017

yotamberk commented Sep 3, 2017 •

edited

Loading

macleodbroad-wf Sep 3, 2017 •

edited

Loading

yotamberk Sep 3, 2017

macleodbroad-wf Sep 3, 2017

yotamberk Sep 3, 2017

macleodbroad-wf Sep 3, 2017

yotamberk Sep 3, 2017

macleodbroad-wf Sep 3, 2017

wimrijnders Sep 4, 2017

grimalschi Sep 4, 2017

wimrijnders Sep 4, 2017

wimrijnders Sep 5, 2017

wimrijnders Sep 4, 2017

wimrijnders Sep 4, 2017

wimrijnders left a comment

wimrijnders Sep 5, 2017

yotamberk commented Sep 16, 2017

wimrijnders Sep 18, 2017

wimrijnders Sep 18, 2017

wimrijnders Sep 18, 2017

wimrijnders Sep 18, 2017

wimrijnders Sep 18, 2017

wimrijnders left a comment

yotamberk commented Sep 18, 2017

Group redraw performance #3409

Group redraw performance #3409

Conversation

yotamberk commented Sep 2, 2017

yotamberk commented Sep 3, 2017

grimalschi commented Sep 3, 2017

yotamberk commented Sep 3, 2017 • edited Loading

macleodbroad-wf Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yotamberk commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders left a comment

Choose a reason for hiding this comment

yotamberk commented Sep 18, 2017

yotamberk commented Sep 3, 2017 •

edited

Loading

macleodbroad-wf Sep 3, 2017 •

edited

Loading