Move git refresh status task to long running task (Bug #9544) by Xapphire13 · Pull Request #14456 · atom/atom

Xapphire13 · 2017-05-17T18:01:59Z

Description of the Change

This PR addresses #9544. The issue here is that upon gaining focus, Atom spins off a new task to update the repository status. Profiling Atom during this time shows that there is a lengthy (~ 2 seconds on my machine, longer on others') delay on the rendering thread due to the child_process.fork(). This PR changes it so that ~~each repository has its own~~ there is a single long running task that receives a message upon needing to update repo status. This moves the overhead of the child_process.fork() into a single hit ~~per repo~~ (I am experiencing zero noticeable delay upon focus now).

Alternate Designs

An alternate design would be to have a single task that handled messages for all repositories. I didn't go this route as I didn't want to change the method signatures in the areas I touched. If we did it at the workspace level, we would need to allow for passing in a reference to the task. And potentially add a few .destroy() methods to the layer that housed the task. I figure that it would be rare to have a number of repositories open at any given time that would cause negative effects due to the number of helper tasks alive.

I managed to implement the alternate design without making a change to the API

Why Should This Be In Core?

This fixes an issue within core itself.

Benefits

Usability of Atom is increased as you spend less time per day waiting on a blocked rendering thread.

Possible Drawbacks

~~If you have a high number of repos open on Atom, you will have a high number of child processes.~~

n/a

Applicable Issues

n/a

winstliu · 2017-05-17T19:25:58Z

Awesome! Looks like there's quite a few lint and test failures though - could you please address those?

Xapphire13 · 2017-05-17T19:46:18Z

Yeah sorry about that. Forgot to run the lint script. Fixing now =]

…s during tests

…ies (without changing API)

…Task

… for some reason.

Xapphire13 · 2017-05-19T06:46:18Z

Finally fixed the tests =]. Seems that window.requestIdleCallback() was causing the tests to fail. Not sure why though. The experience was useful as I found a few areas where tests weren't cleaning themselves up correctly, fixed that too. All tests are passing locally and in the CIs, with the exception of AppVeyor which seems to be having issues right now (same apm issue on other people's builds too)

…Task

Xapphire13 · 2017-06-01T23:33:51Z

@50Wliu, what is the usual turnaround time for an Atom PR?

winstliu · 2017-06-02T00:50:47Z

It depends. There's a lot of changes happening right now, so I can't give you an estimate. It is in the todo list however. (I know that's not the answer you were looking for, but it's the best I can give.)

…Task

BinaryMuse · 2017-07-10T18:43:27Z

Thanks for the PR, and thanks for being patient as we worked through it!

There are a few concerns I have regarding this change as it stands. Items 1 and 2 are addressable, but 3 and 4 I'm not sure about for the short term. Definitely open to feedback especially from @smashwilson and @kuychaco.

It doesn't look like we do any handling of the status handler instance if it crashes unexpectedly — we'd want to detect this and clear the appropriate state in the helper so the next call to refreshStatus properly restarts it.
It also doesn't look like we do any handling in the task itself of any errors. I know this was all just copy/pasted from the old code, but I think wrapping at least the call to repo.release() in a try/finally would be a good idea.
This creates a new long-running sidecar process for every Atom window. Probably not the biggest deal, but the GitHub package already does this for its own Git handling; a proper solution to the problem being addressed here (and, in general, for providing Git information to other parts of Atom) is to expose a service from the GitHub package that core and other packages can consume. Unfortunately this isn't trivial and probably not a viable short-term solution.
The Task API uses ChildProcess#send for communication, which (as we discovered while working on the GitHub package) has a bug that causes a quadratic growth in time as the size of the input grows. Eventually the version of Node contained in Electron will provide a fix for this, but in the meantime we were still seeing a lot of UI jank and freezing during large repositories simply due to the size of the output when calling status. @kuychaco fixed this by using a sidecar Electron process/IPC instead of native Node IPC — again, exposing a service from the GitHub package, which already has this feature, makes the most sense to address this.

…Task

…its on its own (e.g. crashes). Safeguarding repo.release() with a try/finally

Xapphire13 · 2017-07-10T19:06:21Z

Thanks for the review! I have updated with changes addressing items 1 and 2. I'm unsure of how to move forward with items 3 and 4 however.

BinaryMuse · 2017-07-10T19:55:47Z

src/repository-status-handler-helper.coffee

 startHandler = ->
  if not handlerInstance?
    handlerInstance = new Task require.resolve('./repository-status-handler')
+    terminatedSub = handlerInstance.on "exit", status ->


Looks like a minor typo here: (status) -> instead of status ->

Oops. Removed the param entirely as it's unused

Xapphire13 · 2017-07-30T08:14:05Z

Anything else I can add to the PR? I'm eager to work on this to address any concerns. It seems like the bug im addressing is getting a lot of attention from people this PR could help.

Zireael07 · 2017-08-08T06:36:33Z

What's the holdup on merging this?

BinaryMuse · 2017-08-10T22:10:29Z

I'm still concerned about the remaining issues I commented on above. Pinging @atom/core for additional 💭 s around this feature.

Xapphire13 · 2017-08-11T15:32:39Z

No worries, if it gets merged, I want it to be the right code for the job =]

iolsen · 2017-08-11T16:45:35Z

My opinion, based on 3 and 4 in @BinaryMuse's comment, is that we should hold off merging this until the GitHub package exposes a service it can call. We've begun thinking about the right way to implement that.

Xapphire13 · 2017-08-23T01:57:03Z

Abandoning until #1089 is resolved. The new PR at that time will likely share little code with this.

BinaryMuse · 2017-09-20T20:09:42Z

Alright, after considerable discussion with the rest of the team, I'm opting to reopen this PR, get it up to date with the new changes in #15415, and get it merged in. Apologies for the back and forth as we've explored the various ways to fix the underlying issue, and thanks again @Xapphire13 for the PR.

Xapphire13 added 2 commits May 17, 2017 00:11

Modifying to use long running task for GitRepository.refreshStatus()

727d621

Removing duplicate reference to Project

5b4746e

winstliu added the requires-changes label May 17, 2017

Xapphire13 added 8 commits May 17, 2017 13:34

Fixing lint errors

78f28bc

Fixing tests broke by timing of creating the long running task

e68310c

Fixing git-repository.coffee so that it doesn't orphan child processe…

eec7f44

…s during tests

Making it so that only a single process is used for ALL git repositor…

9d3c8ae

…ies (without changing API)

Merge branch 'master' of https://github.com/atom/atom into gitRefresh…

674ca3b

…Task

Fixing specs that didn't clean themselves up properly

c7a247f

Adding ability to skip Main Process tests

fccd0e4

Removing window.requestIdleCallback() which was causing tests to fail…

44b1488

… for some reason.

winstliu added needs-review and removed requires-changes labels May 19, 2017

Merge branch 'master' of https://github.com/atom/atom into gitRefresh…

8b2108e

…Task

hillam approved these changes Jun 15, 2017

View reviewed changes

Xapphire13 mentioned this pull request Jun 16, 2017

Atom is slow regaining focus on Windows and Linux with git repo #9544

Closed

Xapphire13 added 2 commits June 19, 2017 15:22

Merging upstream

c63e4f3

Merge branch 'master' of https://github.com/atom/atom into gitRefresh…

ccaa635

…Task

Xapphire13 added 2 commits July 10, 2017 11:55

Merge branch 'master' of https://github.com/atom/atom into gitRefresh…

d7bb9cc

…Task

Adding event listener to let the handler helper know when the task ex…

2d6e9d0

…its on its own (e.g. crashes). Safeguarding repo.release() with a try/finally

BinaryMuse reviewed Jul 10, 2017

View reviewed changes

Removing unused param

f25a95d

Merge branch 'master' into gitRefreshTask

7848fd8

Merge branch 'master' into gitRefreshTask

32bd1fb

iolsen mentioned this pull request Aug 11, 2017

Expose services for other packages to consume atom/github#1089

Open

Xapphire13 closed this Aug 23, 2017

Xapphire13 mentioned this pull request Aug 23, 2017

Adding option to skip main process tests. Cleaning up resources in tests #15415

Merged

BinaryMuse reopened this Sep 20, 2017

BinaryMuse mentioned this pull request Sep 20, 2017

Finish up #14456 (Git refresh backgrounding) #15702

Closed

Xapphire13 closed this Sep 28, 2017

Xapphire13 deleted the gitRefreshTask branch November 7, 2017 00:57

Conversation

Xapphire13 commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the Change

Alternate Designs

Why Should This Be In Core?

Benefits

Possible Drawbacks

Applicable Issues

Uh oh!

winstliu commented May 17, 2017

Uh oh!

Xapphire13 commented May 17, 2017

Uh oh!

Xapphire13 commented May 19, 2017

Uh oh!

Xapphire13 commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winstliu commented Jun 2, 2017

Uh oh!

BinaryMuse commented Jul 10, 2017

Uh oh!

Xapphire13 commented Jul 10, 2017

Uh oh!

BinaryMuse Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

Xapphire13 Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

Xapphire13 commented Jul 30, 2017

Uh oh!

Zireael07 commented Aug 8, 2017

Uh oh!

BinaryMuse commented Aug 10, 2017

Uh oh!

Xapphire13 commented Aug 11, 2017

Uh oh!

iolsen commented Aug 11, 2017

Uh oh!

Xapphire13 commented Aug 23, 2017

Uh oh!

BinaryMuse commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Xapphire13 commented May 17, 2017 •

edited

Loading

Xapphire13 commented Jun 1, 2017 •

edited

Loading