Better convenient patches#883
Better convenient patches#883implausible merged 7 commits intomasternodegit/nodegit:masterfrom better-convenient-patchesnodegit/nodegit:better-convenient-patchesCopy head branch name to clipboard
Conversation
|
This iteration is currently too slow, need to build a refcounter on hunks / lines and implement convenient line. |
|
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. |
|
This looks great! Could we get a summary of the API changes involved in this? |
|
Minor changes to API:
|
|
⭐ |
93939bf to
f31b1d3
Compare
|
@tbranyen thoughts before I merge this in? |
|
@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. |
|
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. |
|
@implausible there was no perf increase? |
|
@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. |
|
👍 |
|
I'm @johnhaley81. You just pinged some random guy. Sorry @johnh. |
|
Could we get a delta before we merge? I'd like to include that in the changelog. |
|
slack habits lol |
|
If none of the interfaces have changed, we could probably just mention that they are more stable now and moved to C++ |
|
👍 I'm good with this. @tbranyen I'm going to do a release to 0.10.0 after this is merged. |
|
Wow. I am wrong. There is a HUGE perf improvement. @tbranyen @johnhaley81 @srajko Result before move to C++ convenience wrapper: Result after move: |
|
🌟 💯 🌟 |
|
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! |
This is the solution I've come up with to fix the locking issues with patches. See #877.