Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance optimizations rebased #1461

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ We are following the [Keep a Changelog](https://keepachangelog.com/) format.

## [Unreleased](https://github.com/FredrikNoren/ungit/compare/v1.5.15...master)

### Fixed
- Performance optimizations for the big org [#1091](https://github.com/FredrikNoren/ungit/issues/1091)
- ignore 'rename' filewatch event as it can cause constant update and refresh loop
- Prevent redundant ref and node calculations per each `/gitlog` api call

## [1.5.15](https://github.com/FredrikNoren/ungit/compare/v1.5.14...v1.5.15)

### Changed
Expand Down
2 changes: 1 addition & 1 deletion components/graph/git-graph-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you use timestamps? Is it because numeric comparison will be a little faster?

);
});
}
Expand Down
53 changes: 28 additions & 25 deletions components/graph/git-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,22 @@ const components = require('ungit-components');
const programEvents = require('ungit-program-events');
const Animateable = require('./animateable');
const GraphActions = require('./git-graph-actions');
const _ = require('lodash');

const maxBranchesToDisplay = parseInt((ungit.config.numRefsToShow / 5) * 3); // 3/5 of refs to show to branches
const maxTagsToDisplay = ungit.config.numRefsToShow - maxBranchesToDisplay; // 2/5 of refs to show to tags

class GitNodeViewModel extends Animateable {
constructor(graph, sha1) {
super(graph);
this.hasBeenRenderedBefore = false;
this.graph = graph;
this.sha1 = sha1;
this.isInited = false;
this.title = ko.observable();
this.parents = ko.observableArray();
this.commitTime = undefined; // commit time in string
this.date = undefined; // commit time in numeric format for sort
this.timestamp = undefined; // commit time in numeric format for sort
this.color = ko.observable();
this.ideologicalBranch = ko.observable();
this.remoteTags = ko.observableArray();
Expand Down Expand Up @@ -113,6 +115,8 @@ class GitNodeViewModel extends Animateable {
new GraphActions.Revert(this.graph, this),
new GraphActions.Squash(this.graph, this),
];

this.render = _.debounce(this.render.bind(this), 50, { trailing: true });
}

getGraphAttr() {
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What case does this handle?

}

if (this.aboveNode && this.aboveNode.selected()) {
this.cy(this.aboveNode.cy() + this.aboveNode.commitComponent.element().offsetHeight + 30);
}

this.color(this.ideologicalBranch() ? this.ideologicalBranch().color : '#666');
if (!this.hasBeenRenderedBefore) {
// 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

}
this.animate();
}

setData(logEntry) {
this.title(logEntry.message.split('\n')[0]);
this.parents(logEntry.parents || []);
this.commitTime = logEntry.commitDate;
this.date = Date.parse(this.commitTime);
this.timestamp = logEntry.timestamp || Date.parse(this.commitTime);
this.commitComponent.setData(logEntry);
this.signatureMade(logEntry.signatureMade);
this.signatureDate(logEntry.signatureDate);
Expand All @@ -175,38 +185,31 @@ class GitNodeViewModel extends Animateable {
showRefSearchForm(obj, event) {
this.refSearchFormVisible(true);

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', '');
Comment on lines -178 to -208
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code seems unrelated to optimization, not sure if this is an improvement or an accidental revert

}
messages: {
noResults: '',
results: () => {},
},
})
.focus(() => {
$(this).autocomplete('search', $(this).val());
})
.data('ui-autocomplete')._renderItem = (ul, item) =>
$('<li></li>').append(`<a>${item.dom}</a>`).appendTo(ul);
$(textBox).autocomplete('search', '');
}

createBranch() {
Expand Down
5 changes: 4 additions & 1 deletion components/graph/git-ref.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ class RefViewModel extends Selectable {
// This optimization is for autocomplete display
this.value = splitedName[splitedName.length - 1];
this.label = this.localRefName;
this.dom = `${this.localRefName}<span>${octicons[this.isTag ? 'tag' : 'git-branch'].toSVG({
height: 18,
})}</span>`;

this.displayHtml = (largeCurrent) => {
const size = largeCurrent && this.current() ? 26 : 18;
Expand Down Expand Up @@ -120,7 +123,7 @@ class RefViewModel extends Selectable {
operation = '/branches';
}

if (!rewindWarnOverride && this.node().date > toNode.date) {
if (!rewindWarnOverride && this.node().timestamp > toNode.timestamp) {
promise = components
.create('yesnodialog', {
title: 'Are you sure?',
Expand Down
45 changes: 29 additions & 16 deletions components/graph/graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const _ = require('lodash');
const moment = require('moment');
const octicons = require('octicons');
const components = require('ungit-components');
const programEvents = require('ungit-program-events');
const GitNodeViewModel = require('./git-node');
const GitRefViewModel = require('./git-ref');
const EdgeViewModel = require('./edge');
Expand Down Expand Up @@ -40,7 +41,7 @@ class GraphViewModel {
this.scrolledToEnd = _.debounce(
() => {
this.limit(numberOfNodesPerLoad + this.limit());
this.loadNodesFromApi();
this.loadNodesFromApiThrottled();
},
500,
true
Expand All @@ -49,13 +50,13 @@ class GraphViewModel {
() => {
if (this.skip() <= 0) return;
this.skip(Math.max(this.skip() - numberOfNodesPerLoad, 0));
this.loadNodesFromApi();
this.loadNodesFromApiThrottled();
},
500,
true
);
this.commitOpacity = ko.observable(1.0);
this.heighstBranchOrder = 0;
this.highestBranchOrder = 0;
this.hoverGraphActionGraphic = ko.observable();
this.hoverGraphActionGraphic.subscribe(
(value) => {
Expand All @@ -73,15 +74,20 @@ class GraphViewModel {
this.hoverGraphActionGraphic(null);
}
});

this.loadNodesFromApiThrottled = _.throttle(this.loadNodesFromApi.bind(this), 1000);
this.updateBranchesThrottled = _.throttle(this.updateBranches.bind(this), 1000);
this.loadNodesFromApi();
this.updateBranches();
this.loadNodesFromApiThrottled = _.throttle(this.loadNodesFromApi.bind(this), 1000, {
leading: true,
trailing: false,
});
this.updateBranchesThrottled = _.throttle(this.updateBranches.bind(this), 1000, {
leading: true,
trailing: false,
});
this.graphWidth = ko.observable();
this.graphHeight = ko.observable(800);
this.searchIcon = octicons.search.toSVG({ height: 18 });
this.plusIcon = octicons.plus.toSVG({ height: 18 });
this.loadNodesFromApiThrottled();
this.updateBranchesThrottled();
}

updateNode(parentElement) {
Expand Down Expand Up @@ -143,7 +149,8 @@ class GraphViewModel {
if (nodes.length > 0) {
this.graphHeight(nodes[nodes.length - 1].cy() + 80);
}
this.graphWidth(1000 + this.heighstBranchOrder * 90);
this.graphWidth(1000 + this.highestBranchOrder * 90);
programEvents.dispatch({ event: 'init-tooltip' });
})
.catch((e) => this.server.unhandledRejection(e))
.finally(() => {
Expand All @@ -164,7 +171,7 @@ class GraphViewModel {
computeNode(nodes) {
nodes = nodes || this.nodes();

this.markNodesIdeologicalBranches(this.refs(), nodes, this.nodesById);
this.markNodesIdeologicalBranches(this.refs());

const updateTimeStamp = moment().valueOf();
if (this.HEAD()) {
Expand Down Expand Up @@ -197,7 +204,7 @@ class GraphViewModel {
node.branchOrder(ideologicalBranch.branchOrder);
}

this.heighstBranchOrder = branchSlotCounter - 1;
this.highestBranchOrder = branchSlotCounter - 1;
let prevNode;
nodes.forEach((node) => {
node.ancestorOfHEAD(node.ancestorOfHEADTimeStamp == updateTimeStamp);
Expand All @@ -219,9 +226,15 @@ class GraphViewModel {
return edge;
}

markNodesIdeologicalBranches(refs, nodes, nodesById) {
refs = refs.filter((r) => !!r.node());
refs = refs.sort((a, b) => {
markNodesIdeologicalBranches(refs) {
const refNodeMap = {};
refs.forEach((r) => {
if (!r.node()) return;
if (!r.node().timestamp) return;
if (refNodeMap[r.node().sha1]) return;
refNodeMap[r.node().sha1] = r;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this actually speed up things? Are there that many duplicates that the overhead of deduplicating is worth it?

});
refs = Object.values(refNodeMap).sort((a, b) => {
if (a.isLocal && !b.isLocal) return -1;
if (b.isLocal && !a.isLocal) return 1;
if (a.isBranch && !b.isBranch) return -1;
Expand All @@ -230,8 +243,8 @@ class GraphViewModel {
if (!a.isHEAD && b.isHEAD) return -1;
if (a.isStash && !b.isStash) return 1;
if (b.isStash && !a.isStash) return -1;
if (a.node() && a.node().date && b.node() && b.node().date)
return a.node().date - b.node().date;
if (a.node() && a.node().timestamp && b.node() && b.node().timestamp)
return a.node().timestamp - b.node().timestamp;
return a.refName < b.refName ? -1 : 1;
});
const stamp = this._markIdeologicalStamp++;
Expand Down
5 changes: 0 additions & 5 deletions components/graph/graph.less
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@

.graphLog {
left: 575px;

.loadAhead {
cursor: pointer;
animation: throb 1s ease alternate infinite;
}
}

@keyframes throb {
Expand Down
14 changes: 13 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -97,5 +97,17 @@
"node": "^10.18.0 || >=11.14.0"
},
"license": "MIT",
"main": "public/main.js"
"main": "public/main.js",
"window": {
"title": "Ungit",
"icon": "icon.png",
"toolbar": false,
"frame": false,
"show": false,
"width": 1000,
"height": 600,
"position": "center",
"min_width": 400,
"min_height": 200
}
}
4 changes: 2 additions & 2 deletions source/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ const defaultConfig = {
// Always load with active checkout branch (deprecated: use `maxActiveBranchSearchIteration`)
alwaysLoadActiveBranch: false,

// Max search iterations for active branch. ( value means not searching for active branch)
maxActiveBranchSearchIteration: -1,
// Max search iterations for active branch.
maxActiveBranchSearchIteration: 25,

// number of nodes to load for each git.log call
numberOfNodesPerLoad: 25,
Expand Down
2 changes: 1 addition & 1 deletion source/git-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ exports.registerApi = (env) => {
.then(() => {
const watcher = watch(pathToWatch, options || {});
watcher.on('change', (event, filename) => {
if (!filename) return;
if (event === 'rename' || !filename) return;
const filePath = path.join(subfolderPath, filename);
winston.debug(`File change: ${filePath}`);
if (isFileWatched(filePath, socket.ignore)) {
Expand Down
1 change: 1 addition & 0 deletions source/git-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const gitLogHeaders = {
},
CommitDate: (currentCommmit, date) => {
currentCommmit.commitDate = date;
currentCommmit.timestamp = Date.parse(date);
},
Reflog: (currentCommmit, data) => {
currentCommmit.reflogId = /\{(.*?)\}/.exec(data)[1];
Expand Down
7 changes: 7 additions & 0 deletions test/spec.git-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ describe('git-parser parseGitLog', () => {
authorEmail: '[email protected]',
authorName: 'Test ungit',
commitDate: 'Fri Jan 4 14:54:06 2019 +0100',
timestamp: 1546610046000,
committerEmail: '[email protected]',
committerName: 'Test ungit',
additions: 176,
Expand Down Expand Up @@ -260,6 +261,7 @@ describe('git-parser parseGitLog', () => {
authorEmail: '[email protected]',
authorName: 'Test ungit',
commitDate: 'Fri Jan 4 14:03:56 2019 +0100',
timestamp: 1546607036000,
committerEmail: '[email protected]',
committerName: 'Test ungit',
additions: 32,
Expand All @@ -286,6 +288,7 @@ describe('git-parser parseGitLog', () => {
authorEmail: '[email protected]',
authorName: 'Test ungit',
commitDate: 'Fri Jan 4 14:02:56 2019 +0100',
timestamp: 1546606976000,
committerEmail: '[email protected]',
committerName: 'Test ungit',
additions: 0,
Expand All @@ -303,6 +306,7 @@ describe('git-parser parseGitLog', () => {
authorEmail: '[email protected]',
authorName: 'Test ungit',
commitDate: 'Fri Jan 4 14:01:56 2019 +0100',
timestamp: 1546606916000,
committerEmail: '[email protected]',
committerName: 'Test ungit',
additions: 14,
Expand Down Expand Up @@ -370,6 +374,7 @@ describe('git-parser parseGitLog', () => {
authorEmail: '[email protected]',
authorName: 'Test ungit',
commitDate: 'Fri Jan 4 14:03:56 2019 +0100',
timestamp: 1546607036000,
committerEmail: '[email protected]',
committerName: 'Test ungit',
additions: 32,
Expand Down Expand Up @@ -413,6 +418,7 @@ describe('git-parser parseGitLog', () => {
authorEmail: '[email protected]',
authorName: 'Test ungit',
commitDate: 'Fri Jan 4 14:03:56 2019 +0100',
timestamp: 1546607036000,
committerEmail: '[email protected]',
committerName: 'Test ungit',
additions: 32,
Expand Down Expand Up @@ -521,6 +527,7 @@ describe('git-parser parseGitLog', () => {
committerName: 'Test ungit',
committerEmail: '[email protected]',
commitDate: 'Fri Jan 4 14:03:56 2019 +0100',
timestamp: 1546607036000,
message: 'submodules parser',
});
});
Expand Down