Performance optimizations rebased #1461
Conversation
… are too aggressive
Co-Authored-By: campersau <[email protected]>
|
@jung-kim Could you check if this rebase is correct? Feel free to copy this to your PR. |
|
@jung-kim it's a bunch faster, but when I'm switching branches, I see commits that shouldn't be there. Known issue or wrong rebase? |
|
I changed the render debounce to trailing and now it seems to work fine. |
|
@jung-kim It seems that the git api changes were already merged, so now it looks like it's mostly some optimizations on rendering. I'll add comments for the things I don't understand. |
| @@ -95,7 +95,7 @@ class Reset extends ActionBase { | |||
| context && | |||
| context.node() && | |||
| remoteRef.node() != context.node() && | |||
| remoteRef.node().date < context.node().date | |||
| remoteRef.node().timestamp < context.node().timestamp | |||
There was a problem hiding this comment.
Why do you use timestamps? Is it because numeric comparison will be a little faster?
| @@ -141,22 +145,28 @@ class GitNodeViewModel extends Animateable { | |||
| } else { | |||
| this.r(15); | |||
| this.cx(610 + 90 * this.branchOrder()); | |||
| this.cy(this.aboveNode ? this.aboveNode.cy() + 60 : 120); | |||
| this.cy(this.aboveNode && !isNaN(this.aboveNode.cy()) ? this.aboveNode.cy() + 60 : 120); | |||
There was a problem hiding this comment.
What case does this handle?
| // push this nodes into the graph's node list to be rendered if first time. | ||
| // if been pushed before, no need to add to nodes. | ||
| this.hasBeenRenderedBefore = true; | ||
| this.graph.nodes.push(this); |
There was a problem hiding this comment.
I don't understand how this can push here when it wasn't rendered before and there's no removal of a push elsewhere.
When I amend the latest commit, the current branch no longer shows up and I need to reload the page. Is that due to this?
| const textBox = event.currentTarget.parentElement.querySelector('input[type="search"]'); | ||
| const $textBox = $(textBox); | ||
|
|
||
| if (!$textBox.autocomplete('instance')) { | ||
| const renderItem = (ul, item) => $(`<li><a>${item.displayHtml()}</a></li>`).appendTo(ul); | ||
| $textBox.autocomplete({ | ||
| classes: { | ||
| 'ui-autocomplete': 'dropdown-menu', | ||
| }, | ||
| const textBox = event.currentTarget.nextElementSibling.firstElementChild; // this may not be the best idea... | ||
| $(textBox) | ||
| .autocomplete({ | ||
| source: this.refs().filter((ref) => !ref.isHEAD), | ||
| minLength: 0, | ||
| create: (event) => { | ||
| $(event.target).data('ui-autocomplete')._renderItem = renderItem; | ||
| }, | ||
| select: (_event, ui) => { | ||
| select: (event, ui) => { | ||
| const ref = ui.item; | ||
| const ray = ref.isTag ? this.tagsToDisplay : this.branchesToDisplay; | ||
|
|
||
| // if ref is in display, remove it, else remove last in array. | ||
| ray.splice(ray.indexOf(ref), 1); | ||
| ray.unshift(ref); | ||
| this.refSearchFormVisible(false); | ||
|
|
||
| // Clear search input on selection | ||
| return false; | ||
| }, | ||
| }); | ||
| $textBox.focus((event) => { | ||
| $(event.target).autocomplete('search', event.target.value); | ||
| }); | ||
| $textBox.autocomplete('search', ''); |
There was a problem hiding this comment.
this code seems unrelated to optimization, not sure if this is an improvement or an accidental revert
| if (!r.node()) return; | ||
| if (!r.node().timestamp) return; | ||
| if (refNodeMap[r.node().sha1]) return; | ||
| refNodeMap[r.node().sha1] = r; |
There was a problem hiding this comment.
Does this actually speed up things? Are there that many duplicates that the overhead of deduplicating is worth it?
|
Closing this, #1315 is much faster |
Rebase of #1277
Seems to work nicely :)