8000 [WIP] Cache repository / owner values in persistent handle by srajko · Pull Request #962 · nodegit/nodegit · GitHub 8000
[go: up one dir, main page]

Skip to content

[WIP] Cache repository / owner values in persistent handle#962

Closed
srajko wants to merge 3 commits intonodegit:masterfrom
srajko:cache-repository
Closed

[WIP] Cache repository / owner values in persistent handle#962
srajko wants to merge 3 commits intonodegit:masterfrom
srajko:cache-repository

Conversation

@srajko
Copy link
Collaborator
@srajko srajko commented Mar 23, 2016

This works towards #955. Once we free repositories on wrapper GC, any object that can return a repository (for example, a GitCommit) can potentially return a freed repository. This PR adds persistent handles to any object that returns a repository. Once we enforce a 1-1 relationship between git_repository objects and GitRepository wrappers, this will ensure that the wrapper does not get GCd until all objects that can return the repository can get GCed as well.

The stand-alone benefit of this PR is just that we don't create a new wrapper each time we return a repository from an object.

Also, the caching mechanism might be helpful beyond holding a handle - we can extend it to cache any similar value and avoid calling into the libgit2 layer for sync functions (this can help with thread safety without requiring locking).

@srajko srajko force-pushed the cache-repository branch 3 times, most recently from ef7992d to 36d6a31 Compare March 24, 2016 04:51
@srajko srajko changed the title [WIP] Cache repository / owner values in persistent handle Cache repository / owner values in persistent handle Mar 24, 2016
@johnhaley81
Copy link
Collaborator

@johnhaley81
Copy link
Collaborator

Same on appveyor

@srajko
Copy link
Collaborator Author
srajko commented Mar 24, 2016

Looks like it's creating a wrapper around a NULL raw value (a null tree in this case), and crashing trying to get NULL's owner. I can put a check around it.

@srajko srajko force-pushed the cache-repository branch from 36d6a31 to 6cc808e Compare March 24, 2016 06:05
@srajko
Copy link
Collaborator Author
srajko commented Mar 24, 2016

That took care of the hangs. There is consistent failure now with one of the submodule tests, but that's because it's using the defective repo. Switching to a different one should help.

@srajko
Copy link
Collaborator Author
srajko commented Mar 24, 2016

Actually, no - it looks like it might be a problem with taking the result of reference.peel, which returns a git_object and then feeding it into createBranch which then does a getCommit. It seems like it's interpreting the git_object as a git_oid inside git_commit_lookup. I'm not sure why this behavior is different than before. I'll take a closer look at it in the morning.

@srajko
Copy link
Collaborator Author
srajko commented Mar 24, 2016

I fixed the test to pass the ID since NodeGit is expecting an ID there. Briefly looked at the behavior on master, it does the exact same type misinterpretation, but for some reason it works there.

srajko added 3 commits March 24, 2016 09:05
Affects:
git_blob_owner
git_commit_owner
git_filter_source_repo
git_index_owner
git_object_owner
git_reference_owner
git_remote_owner
git_revwalk_repository
git_submodule_owner
git_tag_owner
git_tree_owner
@srajko srajko force-pushed the cache-repository branch from 2523569 to cd21a0f Compare March 24, 2016 16:06
@srajko srajko changed the title Cache repository / owner values in persistent handle [WIP] Cache repository / owner values in persistent handle Mar 28, 2016
@srajko srajko force-pushed the master branch 2 times, most recently from a78275a to 206d27d Compare April 27, 2016 21:05
@implausible
Copy link
Member

I've completed this in master.

@implausible implausible deleted the cache-repository branch December 10, 2019 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0