Skip to content

convert git2 to gix #23

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Byron
Copy link
Contributor

@Byron Byron commented Sep 9, 2023

This converts git2 to gix for performance and compatibility improvements.

Please note that due to a lack of tests, there might be differences in
the details, so careful manual verification is advised.

Further, I definitely recommend to use anyhow and ? instead of unwrapping everywhere.

Lastly, a simple start for automatic protection from regression would be something like
a command mode gitql -c "select *..." in a well-known repository so one can compare
its output with a known good output.

For that to work though, one would ahve to avoid using hashsets or hashmaps for column names
so these stop changes their order each time.

@AmrDeveloper
Copy link
Owner

AmrDeveloper commented Sep 9, 2023

@Byron I surprised how easily to migrate to gix :D, It's a great work.

I did simple benchmark between the two branches and the first result is using -a flag on GQL repository itself

select * from commits limit 5
git2: 29.5447ms
gix: 50.9478ms

select * from tags
git2: 11.5943ms
gix: 14.981ms

select * from tags
git2: 1.443983s
gix: 1.8547632s

select * from branches
git2: 18.3779ms  // Local and remote branches
gix: 12.4025ms   // Local branches only

The result are not stable so as you said we need to set a command like -c so we can automate the benchmark.

One point is that in git2 i can got remote and local branch together and in gix we will need to create function that do that but if we gain more performance this is okey for me.

I am still doing more tests and of course must write unit tests function to make it easier to migrate,

Note: I am still searching why each time selecting commits in gix is slower than git2

@Byron
Copy link
Contributor Author

Byron commented Sep 9, 2023

Ah, I wasn't aware that -a is good for timings. I think values this low, and gix performing worse than git2 must mean these values where measured in a debug-binary? Otherwise, can you share more information so I can reproduce these? In my --release tests gix is 25% faster for commit iteration for instance.

Please note that due to a lack of tests, there might be differences in
the details.

It's definitely advised to validate this carefully.
@AmrDeveloper
Copy link
Owner

Ah, I wasn't aware that -a is good for timings. I think values this low, and gix performing worse than git2 must mean these values where measured in a debug-binary? Otherwise, can you share more information so I can reproduce these? In my --release tests gix is 25% faster for commit iteration for instance.

Yes i tested them on debug binary just using cargo run command, i will try again with release

BTW, -a command will print time of each pass so we can got exactly engine time

Comment on lines -324 to +352
let local_branches = repo.branches(None).unwrap();
let platform = repo.references().unwrap();
let local_branches = platform.local_branches().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

We need function to get all branches (Local and Remote) to iterate on them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a separate method to iterate remote branches. To to their layout on disk it's fine to do that in two steps. Otherwise, iterate all of them and categorize later.

@AmrDeveloper
Copy link
Owner

AmrDeveloper commented Sep 12, 2023

Hello @Byron, Sorry for delay, I tested them now with release binaries.

Gix is faster 10 to 25% for almost all queries except calculating diffs it's very fast maybe 4X

Select * from diffs

git2: 2.29s
gix : 554ms

Note: On windows it's require Cmake installed i think i should update the release action to install cmake too

@Byron
Copy link
Contributor Author

Byron commented Sep 12, 2023

Note: On windows it's require Cmake installed i think i should update the release action to install cmake too

This is due to some C dependencies that come in with the max-performance feature toggle. You could try max-performance-safe, or turn off the performance mode entirely on windows for a pure-Rust build. That one does not need cmake. Interestingly, cmake is installed on the CI (GitHub Actions) that I am using by default.

@AmrDeveloper
Copy link
Owner

Good @Byron, What you think about getting all branches

@Byron
Copy link
Contributor Author

Byron commented Sep 14, 2023

I believe everything is said in this comment - it contains links to all methods that could be used to deal with branches in various forms.

@AmrDeveloper
Copy link
Owner

AmrDeveloper commented Sep 14, 2023

I believe everything is said in this comment - it contains links to all methods that could be used to deal with branches in various forms.

Yup, sorry don't realized it on the first time :D

@AmrDeveloper
Copy link
Owner

Sorry for delay, i was trying to improve the design and replace the map with DoD and testing the performance, i will finish the design and we can test it with gix

Thank you

@Byron
Copy link
Contributor Author

Byron commented Nov 26, 2023

I should probably close this PR as it's degenerating. Maybe you have suggestions about this?

@AmrDeveloper
Copy link
Owner

I should probably close this PR as it's degenerating. Maybe you have suggestions about this?

Sorry for delay, We can know handle local and remote branch iteration and merge this PR, because update the object structure will take time,

@Byron
Copy link
Contributor Author

Byron commented Nov 26, 2023

Thanks for letting me know.

And also, I am sorry to say that I am out of time here. Maybe I just missed a cue on how to complete this work in time before everything else moves on without it.

You will probably be in the best position to adjust this PR to fit on to the main development branch, as well, and it's probably for the best if you create a new PR for this, probably based on this one.

Please feel free to CC me there so I can provide feedback once this work commences.

@Byron Byron closed this Nov 26, 2023
@AmrDeveloper
Copy link
Owner

Thanks for letting me know.

And also, I am sorry to say that I am out of time here. Maybe I just missed a cue on how to complete this work in time before everything else moves on without it.

You will probably be in the best position to adjust this PR to fit on to the main development branch, as well, and it's probably for the best if you create a new PR for this, probably based on this one.

Please feel free to CC me there so I can provide feedback once this work commences.

Thank you for your time and sorry again for delay.

I will create a new PR with this code and handle branches, once i open it i will mention your for review and feedback

Thank you

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.

2 participants