-
Notifications
You must be signed in to change notification settings - Fork 515
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
Execute native git commands directly #48
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Avoids starting a /bin/sh and /usr/bin/env process on each native command invocation, and will allow exec'ing the command directly.
This removes some overhead from all native git calls in the following ways: - Removes a fork previously performed by Open3, which double forks to avoid needing to Process::wait. - Removes the need to shell escape arguments, since the git process's argv is passed explicitly as an array. - Removes the /bin/sh process (1 fork/exec) Additionally, these changes allow obtaining the git process's exit status, available as $? after any native git command invocations.
This is mostly so it works over RPC.
Pretty awesome. And the select(2) based implementation will fix a long-standing bug where the grit process will hang when a git process writes more than PIPE_BUF bytes to stderr or when the input written to the git process's stdin exceeds PIPE_BUF. The old popen3 based logic writes all of stdin, then reads all of stdout, then reads all of stderr so everything except stdout had to come in under PIPE_BUF. This hasn't been much of an issue but is critical to our plans on using `git cat-file --batch' and writing a bunch of SHA1s on stdin. Also moving toward using a common spawn method interface that's a compatible subset of the Process.spawn method built into Ruby >= 1.9.1. The hope is that most non-MRI platforms will eventually support Process.spawn out of the box and the ones that don't have backports.
This is ready:
Tested under various versions of MRI 1.8.7, REE 1.8.7, and 1.9.2-p0. Not all of these features work under JRuby yet but it works as well as it did previously. |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes as many as 3 fork/exec's from all native git calls, makes it possible to retrieve the git process's exit status, removes the need to shell escape command arguments, and allows setting environment variables on the child process without changing the parent process's environment.
Previously,
#native
calls created a process hierarchy like this:This
#native
implementation forks once and execs git directly:Some light benchmarks shows decent gains.
Linux:
Mac:
Even without the performance benefits, the ability to retrieve exit status and setup the child's environment are much needed additions that will let us clean up some janky code elsewhere.
The old Open3 based
Git#sh
andGit#run
methods remain and are used as a fallback for non-POSIX systems (I assume Open3 works on platforms withoutfork(2)
somehow) and also in cases where a#native
is called with a pipeline as the last argument.I'll be testing this out in GitHub staging/production environments this week.