Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Better convenient patches#883

Merged
implausible merged 7 commits intomasternodegit/nodegit:masterfrom
better-convenient-patchesnodegit/nodegit:better-convenient-patchesCopy head branch name to clipboard
Feb 1, 2016
Merged

Better convenient patches#883
implausible merged 7 commits intomasternodegit/nodegit:masterfrom
better-convenient-patchesnodegit/nodegit:better-convenient-patchesCopy head branch name to clipboard

Conversation

@implausible
Copy link
Member

This is the solution I've come up with to fix the locking issues with patches. See #877.

@implausible implausible changed the title Better convenient patches [wip]Better convenient patches Jan 27, 2016
@implausible
Copy link
Member Author

This iteration is currently too slow, need to build a refcounter on hunks / lines and implement convenient line.

@implausible
Copy link
Member Author

Having difficulties tracking down where exactly the content pointer exists in libgit2 (the pointer that lines keep to the diff text). Instead, since this is a convenience wrapper, I am going to remove that pointer to the diff text in favor of the actual content of the line. This will make tidying up after the convenience wrapper a lot easier.

@implausible implausible changed the title [wip]Better convenient patches Better convenient patches Jan 28, 2016
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these please.

@johnhaley81
Copy link
Collaborator

This looks great! Could we get a summary of the API changes involved in this?

@implausible
Copy link
Member Author

Minor changes to API:

  • ConvenientPatch does not have a patch or a delta property associated with it, if you were using the delta, please just use prototype methods oldFIle, newFile, and Status, which are stripped directly from the delta.
  • ConvenientPatch.prototype.hunks returns a promise with an array of ConvenientHunks.
  • ConvenientPatch
  • ConvenientHunk does not have an exposed diffHunk associated with it, but does have the same members as diffHunk:
    • size() : number of lines in the hunk
    • oldStart() : old starting position
    • oldLines() : number of lines in old file
    • newStart() : new starting position
    • newLines() : number of lines in new file
    • headerLen() : length of header
    • header() : returns the header of the hunk
    • lines() : returns a promise containing DiffLines, not ConvenientLines.
  • DiffLine now contains the members rawContent() and content().
    • rawContent() contains the unformatted content of the line. This is no longer a string from the line to the end of the file.
    • content() contains the utf8 formatted content of the line.

@johnhaley81
Copy link
Collaborator

@implausible implausible force-pushed the better-convenient-patches branch from 93939bf to f31b1d3 Compare January 28, 2016 23:23
@implausible
Copy link
Member Author

@tbranyen thoughts before I merge this in?

@tbranyen
Copy link
Member

@implausible how's the performance looking now? Just did a brief scan of the code and the change looks fine to me, but I think the move to C++ might make the convenient api's a bit harder to maintain.

@implausible
Copy link
Member Author

It will make it a bit harder to follow for sure. The performance is comparable to before. Unsure if faster or slower because it's pretty close. One thing that is nice is that this completely removes the locking issues we had with the git_patch_from_diff issue.

@johnhaley81
Copy link
Collaborator

@implausible there was no perf increase?

@implausible
Copy link
Member Author

@johnhaley81: Unsure if there is a perf 'increase', but there is a certain stability increase. There is definitely a positive perf gain over alternative systems to fix this issue. I can run some tests vs the old system. What i know for sure is there is 0 perf decrease. I think this will be as fast as the system can transfer data between git_patch and git_hunks and git_lines. It still runs asynchronously while the system does transfer of data.

@tbranyen
Copy link
Member

👍

@johnhaley81
Copy link
Collaborator

I'm @johnhaley81. You just pinged some random guy. Sorry @johnh.

@johnhaley81
Copy link
Collaborator

Could we get a delta before we merge? I'd like to include that in the changelog.

@implausible
Copy link
Member Author

slack habits lol

@tbranyen
Copy link
Member

If none of the interfaces have changed, we could probably just mention that they are more stable now and moved to C++

@johnhaley81
Copy link
Collaborator

👍 I'm good with this. @tbranyen I'm going to do a release to 0.10.0 after this is merged.

@implausible
Copy link
Member Author

Wow. I am wrong. There is a HUGE perf improvement. @tbranyen @johnhaley81 @srajko

Result before move to C++ convenience wrapper:
Time to generate patches: 254
number of patches: 927
Time to generate hunks: 34
Number of hunks: 890
Time to generate lines: 3465
Number of lines: 105705

Result after move:
Time to generate patches: 417
number of patches: 927
Time to generate hunks: 33
Number of hunks: 890
Time to generate lines: 127
Number of lines: 105705

@johnhaley81
Copy link
Collaborator

🌟 💯 🌟

implausible added a commit that referenced this pull request Feb 1, 2016
@implausible implausible merged commit 47e0d20 into master Feb 1, 2016
@johnhaley81
Copy link
Collaborator

Time to calc patches is up but that's because we're copying a lot of stuff into memory from the old patch structure. Overall an order of magnitude perf increase. Nice job!

@johnhaley81 johnhaley81 deleted the better-convenient-patches branch February 1, 2016 17:23
@joshaber joshaber mentioned this pull request Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.