[fix #1091] performance boost for big repos #1275
Conversation
… are too aggressive
| @@ -71,9 +71,8 @@ exports.registerApi = (env) => { | |||
| }); | |||
| } | |||
| }).then(() => { | |||
| const watcher = fs.watch(pathToWatch, options || {}); | |||
There was a problem hiding this comment.
Please use the file watcher events which I added. See #1263 (comment)
There was a problem hiding this comment.
Ah.... why? API documentation does not mention error event.
There was a problem hiding this comment.
There was a problem hiding this comment.
The returned fs.FSWatcher does have the events: https://nodejs.org/api/fs.html#fs_class_fs_fswatcher
And it is important to listen for the error event otherwise the process would crash if the event isn't handled. This was a source of clicktest failures because we deleted the temp directory while the directory was still openend in ungit.
There was a problem hiding this comment.
You are right, I find this api design to be confusing... multiple places to register events and when you register change event listener, it would multiple sub event types? This is bit odd especially when fswatcher{} is not shared anywhere else.
It would be much more sensible if listener argument of fs.watch() has "error" events rather than expose it via entirely separate function
Yeah I will fix it up later
|
Nice opportunities to speed up ungit for both repos 👍 Though could you split the PR in different branches (PRs) so that they can be tested individually? |
|
Yeah you are right, let me break this one out to two: one with But inevitably |
| 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 |
There was a problem hiding this comment.
What is the "numeric format"?
Is that ISO or yyyyMMddhhmm?
|
I'm running the branch before the split for some days now and I often had this error :
Am I the only one with this glitch ? I should try one branch and the other now that the PR has been split |
|
This PR definitely changes and challenges how things are being calculated. Definitely needs lots testing and incubations. Will keep my eyes out for these kind of problems |
Few things are done here.
/gitlogapi callignore 'rename' filewatch event as it can cause constant update and refresh loop
This one is bit tricky as some OS, git and node version behave differently and the
fs.watch()doc itself does describe very inconsistent behavior.Here is a problem with that for some combination of OS, git and node versions:
/refsgit api call, git operations would continuously delete and create some files such as.git/refs/remotes/origin/masterfs.watch()remote eventgit-directory-changedsocket io event/gitlogand/refs/gitlogwill seriously bog down entire performance both and back end and frontend as it is more compute intense operation to recalculate and draw/refswill trigger everything back to step 1 and entire thing repeatsThere is no way to test and prepare for all combinations. We should stick with latest doc and latest API.
So, adopted to latest API and ignoring
renameevent.todo
https://github.com/git/git page load with old code: https://recordit.co/O1lzQO62Z2
https://github.com/git/git page load with new code: https://recordit.co/bHmV8QiA0I
Below changes are abstracted out to #1277
Prevent full gitlog history from server to client and load only what is neededUpon page load, client will make and API call to get list of git nodes. However, this call gets list of ALL nodes every time. i.e. if a pages is showing 300 nodes and user scrolls down asking for 25 more nodes, backend will return 325 nodes, not just the 25 nodes needed.This is now fixed and backend will return only the nodes client needs.Prevent redundant ref and node calculations per each/gitlogapi callOne of the biggest performance hit for big repos was due to git refs calculations because ungit loads ALL refs, local and remote branches and tags of it's history.This is needed to some degree to display them in the branch drop down and etc, but there were some inefficiencies on logic that thinking these refs had valid nodes and did unnecessary calculations.