handles, merge, odb: changes for Go 1.6 pointer passing rules#282
Merged
carlosmn merged 5 commits intolibgit2:masterfrom Feb 18, 2016
ianlancetaylor:master
Merged
handles, merge, odb: changes for Go 1.6 pointer passing rules#282carlosmn merged 5 commits intolibgit2:masterfrom ianlancetaylor:master
carlosmn merged 5 commits intolibgit2:masterfrom
ianlancetaylor:master
Conversation
| if input.Contents != nil { | ||
| c.ptr = (*C.char)(unsafe.Pointer(&input.Contents[0])) | ||
| toFree = C.CString(string(input.Contents)) | ||
| c.ptr = toFree |
Member
There was a problem hiding this comment.
This makes us allocate an extra buffer. We also don't actually know that what we have is a string. It's some buffer, so making it go through the string type seems wrong.
If there's no way to work around this in Go, I'd rather we write some C so we're passing &input.Contents[0] to a C function so the rules make it OK for us to re-use this buffer.
Member
|
There's a bunch of unrelated whitespace change that makes this harder to read, could you drop that? |
Contributor
Author
|
Pull request updated. I dropped the whitespace changes, but they were entirely from gofmt. I would suggest running gofmt on the Go files. |
Member
|
The Thanks! |
carlosmn
added a commit
that referenced
this pull request
Feb 18, 2016
handles, merge, odb: changes for Go 1.6 pointer passing rules
navytux
added a commit
to navytux/git-backup
that referenced
this pull request
May 25, 2018
Currently for every file -> blob, and blob -> file we invoke git
subprocess (cat-file or hash-object). We also invoke git subprocess for
every tag read/write and the same for commits and this 1-subprocess per
1 object has very high overhead.
The ways to avoid such overhead could be:
1) for every kind of operation spawn git service process, like e.g.
`git cat-file --batch` for reading files, and only do request/reply
per object with it.
2) use some go library to work with git repository ourselves.
"1" can work but:
- at present there is no counterpart of `cat-file --batch` for
e.g. `hash-object` - i.e. we cannot write objects without quirks
or patching git.
- even if we add support for hashing via request/reply, as all
requests are processed sequentially on git side by e.g. `git
cat-file --batch`, we won't be able to leverage parallelism.
- request/reply has also latency attached.
For "2" we have roughly the following choices:
- use cgo bindings to libgit2 (git2go)
- use some pure-go git library
Pure-go approach has pros that it by design avoids problems related to
tricky CGo pointer C <-> Go passing rules. The fact that this was sorted
out by go team itself only during 1.6 cycle
golang/go#12416
tells a lot. The net is full of examples where those were hard to get,
and git2go in particular has a story of e.g. heap corruption (the bug
was on golang itself side and fixed only for 1.5)
libgit2/git2go#223
https://groups.google.com/forum/#!topic/golang-nuts/Vi1HD-54BTA/discussion
However there is no good (to my knowledge) pure-go git library, and the
family of forks around github.com/speedata/gogit either:
- works 3x slower compared to git2go
( or the same 3x in serial mode compared to e.g. `git cat-file --batch`
as in serial mode git subservice and git2go has roughly similar performance )
- or does not work at all (e.g. barfing out on REF_DELTA pack
entries, etc)
So because of 3x slowdown, pure-go way is currently a no-runner.
Since one person from golang team cared to update git2go to properly
follow the CGo rules
libgit2/git2go#282
we can be relatively confident about git2go bindings quality and try to
use it.
This commit only hooks git2go into the build, subcommands and to Sha1
for to/from Oid conversion. We'll be switching places to git2go
incrementally in upcoming patches.
NOTE for now we need git2go from next branch for
libgit2/git2go@cf7553e7
The plan is to eventually switch to
gopkg.in/libgit2/git2go.v25
once it is out.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
See http://tip.golang.org/cmd/cgo/#hdr-Passing_pointers .