Improve redraw performance with non-visible groups (related to #2835) #2848
Conversation
There was a problem hiding this comment.
Awesome! I was tying to improve this issue and couldn't find an elegant way.
I will note that there is a small glitch dragging the timeline revealing a group after zooming. It is much faster but the origin bug\glitch I solved with calculating the height is kind of back (much less, but still reoccurring). Adding verticalScroll: true to the timeline options causes really bad glitches... Please try to deal with that before we can approve this PR
|
Thanks @yotamberk . I thought it was too simple :). Unfortunately, there doesn't seem to be a way to prevent the vertical scroll jumping without restacking non-visible groups. I had another look at the issue, and decided the problem might not be "non-visible groups are restacking" but that "groups which haven't been changed are restacking". All groups were being forced to restack when any item in the ItemSet changed (as this was tracked in ItemSet.stackDirty). I've had a go at changing this to be tracked at a group level, ie. Group.stackDirty, so that only the necessary groups are restacked. The assumptions I've had to make are:
This isn't as strong an improvement (zooming and scrolling will be as slow as before), but is still as effective for the case where an item is being dragged around. |
|
@nathanbennett Can you supply a new jsbin to show the improvements? |
|
@yotamberk No worries. See here. |
|
Thanks! Looks good to me |
|
Thank you for this nathan! I merged this in locally with develop and this fixes most of my performance issues I was having with having hundreds of groups! Will be looking forward to this being merged in to the main branch. |
|
@nathanbennett can you pull from the develop branch and push again? It won;t let me merge this PR for some odd reason |
| * @return {boolean} Returns true if the group is resized | ||
| */ | ||
| BackgroundGroup.prototype.redraw = function(range, margin, restack) { | ||
| BackgroundGroup.prototype.redraw = function(range, margin, forceRestack) { |
There was a problem hiding this comment.
Why is forceRestack defined as a parameter but not used?
| * @return {boolean} Returns true if the group is resized | ||
| */ | ||
| BackgroundGroup.prototype.redraw = function(range, margin, restack) { | ||
| BackgroundGroup.prototype.redraw = function(range, margin, forceRestack) { |
There was a problem hiding this comment.
Why is this parameter defined but never used?
There was a problem hiding this comment.
To me it looked like the idea was to keep its signature consistent with Group.prototype.redraw, so I left this as it was (other than renaming the parameter in line with my changes).
|
Hi, what time the new version will be released which fix redraw performance? |
|
@qwe852147 We are having trouble merging this PR. I've asked the submitter @nathanbennett to resubmit this, but yet have had a response |
|
Closed for #3078 (due to merging problems) |
7 participants
Related to issue #2835. I found that having a large number of non-visible groups was causing performance issues as the items for these groups are being constantly added then removed from the DOM (so they can be restacked).
The fix I made was to only restack visible groups, and to always restack a group when it becomes visible (as it may have missed out previously). Tested these changes with Chrome DevTools and it reduces a lot of "forced reflow" hammering when dragging a timeline item.
I modified an existing example to demo this (https://jsfiddle.net/nathanbennett/cbprq1vt/). This isn't a very intensive case, so you might need to use CPU throttling to see an appreciable difference :).