Run git commands in dedicated renderer process #688

kuychaco · 2017-04-20T00:51:46Z

Fixes #386.
Fixes #643.

This PR addresses the issue of Atom freezing due to multiple blocking spawn calls when shelling out to Git in process (#386).

The Options

The solution is to make Git calls in a separate process, options being a Node child process, Electron renderer process, or Chromium web worker.

This PR implements a dedicated renderer process for shelling out to Git.

Basic testing with a Node child process revealed quadratic growth in IPC time relative to message size so this approach was dismissed due to the negative performance implications. The fix for this won't be released until Node v7.5.0.

Web workers are the ideal solution, but will have to wait until Atom is upgraded to Electron v1.6.4 which adds Node integration to web workers.

Current Approach

This PR introduces a WorkerManager which creates a Worker that wraps a RendererProcess which shells out to Git and sends results back over IPC. If the renderer process is not yet ready, we fall back to shelling out in process.

The renderer process keeps a running average of the time it takes to make a spawn call and if this exceeds 20ms (and the minimum number of operations specified have been completed), the WorkerManager creates a new RendererProcess and routes all new Git data requests to it. Once the "sick" renderer process finishes its operations it will be destroyed. The new renderer process may have a higher operationCountLimit, so that if the spawn calls continue to take longer than 20ms, we'll wait longer before creating a new RendererProcess in order to prevent process thrashing on slow machines.

If a renderer process crashes, the WorkerManager will create a new RendererProcess with the same operationCountLimit and send all incomplete operations to the new process.

If the WorkerManager process is destroyed, all RendererProcess instances will immediately destroy themselves.

If a Worker has it's destroy method called and there are remaining operations to be completed, we wait until completion before destroying the associated RendererProcess. This is in the event that there is a write operation that is still in progress and we want to allow it to reach disk.

Things to consider for future PRs:

Avoid duplicate write operations when a renderer process crashes mid-write and a new one is created that picks up the unfinished operation. This can be done by marking an Operation as "in-progress" by sending an ipc message after GitProcess.exec has been called.

Only need one RendererProcessManager for each Atom window. Also reset RendererProcessManager in afterEach.

Since we're using a renderer process for spawning git processes right now

... to go along with hostWebContentsId

The sourceWebContentsId is now encoded in the message data

…u-renderer-shell-out

smashwilson

This is really, really cool. I'll install this branch and give it a shot over the next few days but I'd ❤️ to get this merged and out to our staff users as soon as we can 😁

smashwilson · 2017-04-27T01:08:28Z

lib/git-shell-out-strategy.js

      });
    }, {parallel: !writeOperation});
    /* eslint-enable no-console */
  }

+  executeGitCommand(args, options, marker = null) {
+    if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC || !WorkerManager.getInstance().isReady()) {


👍 for offering an env var to opt out of the workers for when someone runs this on their Raspberry Pi 😉

If I'm reading it right it'll prevent the first worker from spawning too?

If I'm reading it right it'll prevent the first worker from spawning too?

Yup 👍 . Second expression after || doesn't get evaluated if first expression evaluates to true

smashwilson · 2017-04-27T01:10:06Z

lib/git-shell-out-strategy.js

@@ -220,6 +239,7 @@ export default class GitShellOutStrategy {

  async isGitRepository() {
    try {
+      await fsStat(this.workingDir); // fails if folder doesn't exist


Cool, cool. This fixes that uncaught-exception-in-Promise from the test suite I presume.

smashwilson · 2017-04-27T01:11:12Z

lib/renderer.html

+  <body>
+    <h1>GitHub Package Git Execution Window</h1>
+    <p>
+      Hi there! I'm a window used by the GitHub pacakge to execute Git commands in the background. My PID is <script>document.write(process.pid)</script>.


✍️ "GitHub pacakge"

(... Nobody will ever see this 😛 )

smashwilson · 2017-04-27T01:15:12Z

lib/worker-manager.js

+  static status = {
+    PENDING: Symbol('pending'),
+    INPROGRESS: Symbol('in-progress'),
+    COMPLETE: Symbol('complete'),


Hooray for Symbols 🎉

smashwilson · 2017-04-27T01:16:06Z

lib/worker-manager.js

+    this.startTime = performance.now();
+    return execFn({...this.data, id: this.id});
+  }
+}


This WorkerManager/Operation setup is downright elegant 😄

🙇‍♀️

smashwilson · 2017-04-27T01:18:21Z

lib/worker.js

+    // For now we won't do this to avoid clogging up ipc channel
+    // event.sender.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'exec-started', data: {id}});
+
+    if (averageTracker.enoughData() && averageTracker.getAverage() > 20) {


I'll be sure to give this a shot on the Windows box to see what the spawn latency is like here. If 20ms is enough here it should be enough for anywhere.

smashwilson · 2017-04-27T01:19:48Z

test/helpers.js

@@ -158,6 +164,16 @@ export function createRenderer() {
  return renderer;
 }

+export function isProcessAlive(pid) {


I'll be honest: before I clicked that link, I really expected "Stayin' Alive." Should have known better 😆

smashwilson · 2017-04-27T01:20:49Z

test/worker-manager.test.js

+      workerManager.request();
+      workerManager.request();
+      const worker1OperationsInFlight = worker1.getRemainingOperations();
+      assert.equal(worker1OperationsInFlight.length, 3);


There's an assert.lengthOf(...) too... I think it shows you the array contents if the length is wrong but it isn't too long?

smashwilson · 2017-04-27T01:21:10Z

test/worker-manager.test.js

+      assert.equal(worker1OperationsInFlight.length, 3);
+
+      const worker1Pid = worker1.getPid();
+      process.kill(worker1Pid, 'SIGKILL');


Haha, to clarify, this was my attempt at a SIGKILL emoji, not an indication that this line shouldn't be here or anything.

smashwilson · 2017-04-27T01:23:23Z

lib/git-shell-out-strategy.js

+          const headerStyle = 'font-weight: bold; color: blue;';
+
+          console.groupCollapsed(`git:${formattedArgs}`);
+          console.log('%cexit status%c %d', headerStyle, 'font-weight: normal; color: black;', exitCode);


The day I discovered how %c works in console.log() was a dangerous day.

kuychaco and others added 30 commits April 12, 2017 18:39

WIP compare renderer and child process IPC times

c07d65b

WIP shell out to git in renderer process

53384de

WIP Create RendererProcessManager

519ebfc

⬆️ atom-mocha-test-runner@1.0.1

c9b2978

Make RendererProcessManager a singleton

f91e8cd

Only need one RendererProcessManager for each Atom window. Also reset RendererProcessManager in afterEach.

Actually import GitError

8fe00d5

Use import instead of require

e9aa22d

Don't show browser window for git renderer process

169ab20

WIP wtf is up w/ the tests

72b5ccc

Wait for renderer process to be ready before sending Git commands

761ff7d

Destroy any Repositories created during tests after each test

54c1653

No need to reset singleton RendererProcessManager after each test

09fdc01

Conditionally choose exec strategy

197b332

Fix global test setup

364b086

Extract Git execution into a separate method for stubbing

ed4c5ed

Un-pend test

ca3411c

Fix flaky test

bc7ae77

🎨

3bda0a9

🔥 git-child-process.js

2228185

Since we're using a renderer process for spawning git processes right now

🎨 and clean up

325f48b

More clean up

2bd9e37

🔥 comments

684ca4a

Remove type property from ipc data and rely on channel to identify type

6494404

🔥 unused changes to GitTimingsView

e7074b7

Actually, we want to test that...

5d2fef3

Put those timeouts back

9b3a288

Mo 🎨

0187c4b

Communicate on ipc channel based on renderer's web contents id

de1918b

Delete entry in RendererProcessManager map after promise is resolved

b86ad95

rendererWebContentsId -> childWebContentsID

66a1a08

... to go along with hostWebContentsId

kuychaco and others added 14 commits April 25, 2017 15:47

Use static channelName for all ipc communication

652459d

The sourceWebContentsId is now encoded in the message data

Dispose of webContents listeners before destroying manager window

7e7d4ce

Fix test

aa09a96

Fix other test

863209d

Merge branch 'ku-renderer-shell-out' of github.com:atom/github into k…

bc3073c

…u-renderer-shell-out

Include sourceWebContentsId in git data ipc message

55f9882

Don't show browser window

6502cbf

Don't reject promise for remaining operations during destroy

c3b875e

🔥 RendererProcessManager

b7f4b20

Force reset of WorkerManager after tests complete

5e34e0e

Implement WorkerManager.prototype.isReady

6359895

Shell out in process if WorkerManager instance isn't ready

c39aaf6

Don't await ready promise in RendererProcess#executeOperation

95fb731

First hacky pass at IPC-based Git timings

7a95446

kuychaco requested a review from smashwilson April 26, 2017 17:33

kuychaco added 3 commits April 26, 2017 13:51

👕

5d309d1

Don't clog up IPC channel with 'exec-started' events

3066270

Destroy WorkerManager in deactivate

4084aec

BinaryMuse approved these changes Apr 27, 2017

View reviewed changes

smashwilson approved these changes Apr 27, 2017

View reviewed changes

kuychaco and others added 5 commits April 26, 2017 18:40

Add console.warn to indicate sick worker

8727017

⌨️

7bab3da

Use assert.lengthOf

1740e9b

Merge branch 'master' into ku-renderer-shell-out

c3b4b23

Only log about sick worker if not in spec mode

e708668

kuychaco merged commit 9b8ed5d into master Apr 27, 2017

kuychaco deleted the ku-renderer-shell-out branch April 27, 2017 20:33

kuychaco removed the work-in-progress label May 12, 2017

BinaryMuse mentioned this pull request Jul 10, 2017

Move git refresh status task to long running task (Bug #9544) atom/atom#14456

Closed

smashwilson mentioned this pull request Feb 28, 2019

RFC: use git ls to crawl all files in the project atom/fuzzy-finder#367

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run git commands in dedicated renderer process #688

Run git commands in dedicated renderer process #688

kuychaco commented Apr 20, 2017 •

edited

Loading

smashwilson left a comment

smashwilson Apr 27, 2017

kuychaco Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

kuychaco Apr 27, 2017

smashwilson Apr 27, 2017

kuychaco Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

smashwilson Apr 27, 2017

Run git commands in dedicated renderer process #688

Run git commands in dedicated renderer process #688

Conversation

kuychaco commented Apr 20, 2017 • edited Loading

The Options

Current Approach

smashwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuychaco commented Apr 20, 2017 •

edited

Loading