Parallelize Git operations #518

BinaryMuse · 2017-02-15T07:56:12Z

This PR introduces a new AsyncQueue that has the ability to parallelize tasks. These parallelized tasks won't block other operations at the queue level, though of course JS semantics may still cause the underlying operations to block one another.

The queue also has the ability to add tasks that are non-parallelizable; currently, this is used to ensure that writes always execute alone, waiting for the queue to drain before they run, and running to completion before normal parallel queue semantics are resumed.

Additionally, a new segment in the timings view has been added called nexttick; it is colored yellow and shows the time after prepare has run but before the actual call to exec runs. In particular, this can help show time growing in a stepwise fashion waiting for the JS event loop to finish with the previous task invocation before the next one can start, indicating we can save some time by optimizing this or making it fully async:

TODO:

Variable names
Convert exec args to an option object

smashwilson

Overall summary:

smashwilson · 2017-02-15T14:43:14Z

lib/async-queue.js

+  }
+
+  execute() {
+    return this.fn.call(undefined).then(this.resolve, this.reject);


Any reason to clobber this with .call() rather than just calling it directly with this.fn().then(...)?

Yeah, I specifically don't want the Task to be the context in the function.

smashwilson · 2017-02-15T14:45:31Z

lib/async-queue.js

-      this.commandQueue.shift();
+  constructor(options = {}) {
+    this.parallelism = options.parallelism || 1;
+    this.nonParallelizableOperation = false;


This PR is a great exercise in spelling different forms of "parallelizable" 😂

Or saying it out loud 😏 @kuychaco

smashwilson · 2017-02-15T14:47:18Z

lib/async-queue.js

+  constructor(options = {}) {
+    this.parallelism = options.parallelism || 1;
+    this.nonParallelizableOperation = false;
+    this.threadsInUse = 0;


threadsInUse feels like a misleading name to me because there aren't really "threads" in play. How about tasksInProgress?

smashwilson · 2017-02-15T14:52:41Z

lib/async-queue.js

+    try {
+      await task.execute();
+    } catch (err) {
+      // nothing


Will this swallow any important errors... ? I'm assuming that we expect most task failures to manifest as Promise rejections 🤔

The Promise inside the Task — the one returned from getPromise — will be rejected, but we don't want the task failure itself to affect the control flow of the queue.

smashwilson · 2017-02-15T14:53:08Z

lib/git-shell-out-strategy.js


  // Execute a command and read the output using the embedded Git environment
-  exec(args, stdin = null, useGitPromptServer = false) {
+  exec(args, stdin = null, useGitPromptServer = false, writeOperation = false) {


Yah, I can see why you want to turn this into an options map. 👍

smashwilson · 2017-02-15T14:53:46Z

lib/git-shell-out-strategy.js

  constructor(workingDir) {
    this.workingDir = workingDir;
-    this.commandQueue = new AsyncQueue();
+    this.commandQueue = new AsyncQueue({parallelism: 5});


Hmm, maybe inject as a package config option?

Yeah, this is temporary. I plan on using the number of CPUs or other similar things to determine this number.

smashwilson · 2017-02-15T14:56:39Z

lib/git-shell-out-strategy.js

-          return stdout;
+      return new Promise(resolve => {
+        timingMarker.mark('nexttick');
+        setImmediate(() => {


Is there a specific reason to use setImmediate vs. setTimeout(..., 0) or process.nextTick for this? I can never remember what the implications of those are.

I had used nextTick at first but switched to setImmediate. I'll need to look into it to see what the difference is.

Use setImmediate if you want to queue the function behind whatever I/O event callbacks that are already in the event queue. Use process.nextTick to effectively queue the function at the head of the event queue so that it executes immediately after the current function completes.

So in a case where you're trying to break up a long running, CPU-bound job using recursion, you would now want to use setImmediate rather than process.nextTick to queue the next iteration as otherwise any I/O event callbacks wouldn't get the chance to run between iterations.

I'm not sure it matters a ton here but I'm going to stick with setImmediate for now.

smashwilson · 2017-02-15T15:01:05Z

lib/async-queue.js

+class Task {
+  constructor(fn, parallel = true) {
+    this.fn = fn;
+    this.parallel = parallel;


Thinking more about the terminology here: would it be clearer to invert this flag and have a Task be "serial" or "exclusive" rather than "parallel"? Or would that lead to a bunch of double negatives elsewhere.

Haha yeah @kuychaco and I debated a bit on this. I think she was right. We'll make it better.

smashwilson · 2017-02-15T15:01:30Z

test/async-queue.test.js

+  }
+}
+
+describe.only('AsyncQueue', function() {


🔥 .only( 😉

smashwilson

👍

smashwilson · 2017-02-15T21:11:16Z

lib/controllers/git-panel-controller.js

      aheadCount: null,
      behindCount: null,
-      remoteName: await repository.getRemoteForBranch(this.branchName),
+      remoteName: await dataPromises.remoteName,


smashwilson · 2017-02-15T21:12:36Z

test/runner.js


 // Give tests that rely on filesystem event delivery lots of breathing room.
-until.setDefaultTimeout(5000);
+until.setDefaultTimeout(3000);


Hmm will this cause problems on AppVeyor?

I hope not. But it has to be lower than the mocha timeout or else test-until can never timeout with an appropriate message because the whole test times out instead.

We can keep an eye on it and go from there.

But it has to be lower than the mocha timeout or else test-until can never timeout with an appropriate message because the whole test times out instead.

Ohhh that's what's been happening. Good catch.

shiftkey · 2017-02-16T19:44:37Z

lib/git-shell-out-strategy.js

   */
  async getStatusesForChangedFiles() {
-    const output = await this.exec(['status', '--untracked-files=all', '-z']);
+    const output = await this.exec(['status', '--untracked-files=all', '-z'], {writeOperation: true});


Lots of these operations seem to be writeOperation: true unnecessarily. Is there some context here I'm missing?

@shiftkey These are all status calls and can create index.locks (as far as I understand). We're converting some of them to nodegit calls in #524

BinaryMuse added 4 commits February 14, 2017 17:03

Create new parallel async queue

72ea857

Parallelize all non-write Git commands

e25b009

Capture JS event loop processing time in timing view waterfalls

a2282c1

🔥 console.warn

7f23487

BinaryMuse requested review from smashwilson and removed request for smashwilson February 15, 2017 08:08

smashwilson approved these changes Feb 15, 2017

View reviewed changes

Michelle Tilley added 10 commits February 15, 2017 10:07

🔥 .only

ff96a4d

Make queue resilient to sync errors

43a17cb

🎨

77dab9a

Oops forgot a thing

afb363e

Remove GSOS tests covered already by AsyncQueue tests

34e7b82

Better parallelize Git operations in GitPanelController

609379f

Drop test-until default timeout to under the mocha timeout

2b43e07

Experiment with optimizing Git calls

3902ad2

Base GSOS queue parallelism on number of CPUs

b3eb449

Convert all GSOS#exec calls to use options object parameter

98ea7b1

smashwilson approved these changes Feb 15, 2017

View reviewed changes

BinaryMuse merged commit 55fc31f into master Feb 15, 2017

BinaryMuse deleted the mkt-ku-parallelize-git-ops branch February 15, 2017 21:21

BinaryMuse mentioned this pull request Feb 16, 2017

🎬 nodegit 2: The Revenge of nodegit! 😱 #524

Closed

shiftkey reviewed Feb 16, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Parallelize Git operations #518

Parallelize Git operations #518

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Parallelize Git operations #518

Parallelize Git operations #518

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!