https://github.com/o2sh/onefetch/issues/310 #341

HallerPatrick · 2020-12-10T23:33:00Z

Resolve Issue #310 and #338

Please review, because this includes a lot of changes. Sorry for that.

Merge pull request #337 from HallerPatrick/master

Merge o2sh/onefetch master into fork

spenserblack

Looks awesome! Thanks for your hard work!

src/onefetch/git_utils.rs

src/onefetch/info.rs

o2sh · 2020-12-11T20:01:39Z

Very impressive @HallerPatrick,

I tried running your branch on a few repos, everything seems to work fine except for the linux repo where I got a:

hread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/onefetch/git_utils.rs:40:45
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

if failed here: let author_name = author.name().unwrap().to_string(); I don't see how a commit can exist without an author name and why it works when using git sys calls..but anyway, I fixed it locally and it worked just fine.

However, the response time has increased quite a bit, still on the linux repo, we went from:

$ time onefetch_master >

real	0m15,742s
user	0m21,599s
sys	0m3,985s

to:

$ time onefetch >

real	0m28,633s
user	0m33,951s
sys	0m1,097s

By the way, the git_utils.rs file was created to isolate functions that were still relying on git sys calls, now everything can be moved into the repo.rs file.

spenserblack · 2020-12-12T00:00:18Z

if failed here: let author_name = author.name().unwrap().to_string();

git2::Signature::name returns None if the name isn't valid UTF-8. Perhaps name_bytes should be used instead (and also email ➡️ email_bytes)?

…nd name due to invalid utf8

HallerPatrick · 2020-12-12T16:49:06Z

Very impressive @HallerPatrick,

I tried running your branch on a few repos, everything seems to work fine except for the linux repo where I got a:
hread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/onefetch/git_utils.rs:40:45
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
if failed here: let author_name = author.name().unwrap().to_string(); I don't see how a commit can exist without an author name and why it works when using git sys calls..but anyway, I fixed it locally and it worked just fine.

Alright also fixed the Signature utf-8 problem with using from_utf8_lossy which should return � for invalid utf-8 chars.

However, the response time has increased quite a bit, still on the linux repo, we went from:
$ time onefetch_master >

real	0m15,742s
user	0m21,599s
sys	0m3,985s
to:
$ time onefetch >

real	0m28,633s
user	0m33,951s
sys	0m1,097s

I also tested this now with Hyperfine (which seems to be a good tool) and had the same results. Its interesting that using git sys calls seem to be much faster than using git2. Maybe commands like object-count are more efficiently implemented in the git cli tools, then I did.

I definitely wanted to dig deeper into this.

By the way, the git_utils.rs file was created to isolate functions that were still relying on git sys calls, now everything can be moved into the repo.rs file.

I would probably move this to another pull requests to keep the diff small and separation of purpose. If that's okay.

o2sh · 2020-12-13T12:35:24Z

Maybe commands like object-count are more efficiently implemented in the git cli tools.

It's the get_git_history function that takes most of the response time:

before (linux repo):

get_git_history --> 11.77949631s
get_files_count --> 18.192162ms
get_repo_size --> 1.709682ms

now:

get_git_history --> 25.771483865s
get_repo_size --> 49.508897ms

By the way, if you remove revwalk.set_sorting(git2::Sort::TIME)?; from get_git_history, its RT drops to:

get_git_history --> 15.868057149s

HallerPatrick · 2020-12-14T12:02:04Z

@o2sh Yeah , I think we reached a little bit of a dead end with git2. With the sys call we just used the logs to parse the commits, but with git2 we can only use the actual refs/commit, which is much more expensive.

IMO, we are dealing with the linux repo with a edge case. With something like tensorflow the performance diff is not really noticeable, which is already a big repo with 100k commits and 2,6 M LOC.

$ time onefetch
3.69s user 1.86s system 212% cpu 2.605 total

$ time ../../Projects/onefetch/target/release/onefetch  
3.84s user 1.34s system 200% cpu 2.586 total

But in the end it is your decision if that is enough or if performance is more important to you. We would have to wait for git2 bindings to logs.

o2sh · 2020-12-14T12:34:27Z

what about this part:

By the way, if you remove revwalk.set_sorting(git2::Sort::TIME)?; from get_git_history, its RT drops to:

get_git_history --> 15.868057149s

I think it can be removed without issue: hence getting us pretty close to git sys calls RT.

We can start from there and look for other optimizations afterwards - if possible.

HallerPatrick · 2020-12-14T12:39:44Z

what about this part:

By the way, if you remove revwalk.set_sorting(git2::Sort::TIME)?; from get_git_history, its RT drops to:
get_git_history --> 15.868057149s

I think it can be removed without issue: hence getting us pretty close to git sys calls RT.

It looks like the commit list is already sorted, at least are creation date and last commit the same with the removed line. So yeah I removed it

- Merge git_utils with repo into a single file - Move get_repo_size_field into info file - Remove blocking dependency with Git binary from onefetch

o2sh · 2020-12-14T20:16:32Z

I took the liberty to make some minor changes e99c56f

HallerPatrick · 2020-12-15T00:44:33Z

I took the liberty to make some minor changes e99c56f

The cleaner the code the happier I am.

Is anything now holding us back from merging?

o2sh · 2020-12-15T11:04:07Z

We'll wait for @spenserblack to finish his review.

On my side, I'm very happy with this PR.

Indeed, even if we loose in performance, I still believe that getting rid of the git sys calls - we still have one call in get_git_version - is the way to go as it makes for a more homogeneous code base and allows onefetch to be used even if the user doesn't have the Git binary installed on his machine.

HallerPatrick · 2020-12-15T11:11:11Z

We'll wait for @spenserblack to finish his review.

On my side, I'm very happy with this PR.

Indeed, even if we loose in performance, I still believe that getting rid of the git sys calls - we still have one call in get_git_version - is the way to go as it makes for a more homogeneous code base and allows onefetch to be used even if the user doesn't have the Git binary installed on his machine.

I am on your side.

get_git_versaion/ is_git_installedare only there to get infos about the git binary, which cold never be replaced by git2.

spenserblack

We'll wait for @spenserblack to finish his review.

Sorry, I didn't realize that you were still waiting for me.

This looks great, @HallerPatrick, thanks for all your help!
Once again, I left a suggested change that shouldn't change the logic at all, just the code style. Optional if you want to implement it.

Overall, LGTM 👍

spenserblack · 2020-12-15T13:39:39Z

src/onefetch/repo.rs

+                    if let Ok(commit) = repo.find_commit(r) {
+                        if no_merges {
+                            let parents = commit.parents().len();
+                            if parents > 1 {
+                                return None;
+                            }
+                        }
+                        Some(commit)
+                    } else {
+                        None
+                    }


Since this is basically converting a Result to an Option that has been filtered, I think this can be simplified with Result::ok and Option::filter.
Something like

repo.find_commit(r).ok().filter(|commit| !(no_merges && commit.parents().len() > 1))

Once again, didn't test the above code, though 🙂

Alright worked directly.

HallerPatrick added 4 commits December 10, 2020 00:21

Replaced os git command with git2

cc1f29c

Merge pull request #1 from o2sh/master

929ea96

Merge pull request #337 from HallerPatrick/master

Put all git call functions into a GitClient impl, minor refactoring

d673169

Merge pull request #2 from o2sh/master

764dea5

Merge o2sh/onefetch master into fork

spenserblack reviewed Dec 11, 2020

View reviewed changes

src/onefetch/git_utils.rs Outdated Show resolved Hide resolved

src/onefetch/info.rs Outdated Show resolved Hide resolved

Minor refactoring, now using a more 'safe' conversion of autho mail a…

3943231

…nd name due to invalid utf8

Removed time sorting for faster RT

826ac93

Multiple changes linked to git2-rs migration

e99c56f

- Merge git_utils with repo into a single file - Move get_repo_size_field into info file - Remove blocking dependency with Git binary from onefetch

don't panic when commit not found in get_logs

9207c6c

spenserblack approved these changes Dec 15, 2020

View reviewed changes

Code style change

ae68b53

o2sh merged commit 80424f4 into o2sh:master Dec 15, 2020

o2sh mentioned this pull request Nov 7, 2021

Use time crate instead of chrono #526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

https://github.com/o2sh/onefetch/issues/310 #341

https://github.com/o2sh/onefetch/issues/310 #341

HallerPatrick commented Dec 10, 2020

spenserblack left a comment

o2sh commented Dec 11, 2020 •

edited

Loading

spenserblack commented Dec 12, 2020

HallerPatrick commented Dec 12, 2020

o2sh commented Dec 13, 2020 •

edited

Loading

HallerPatrick commented Dec 14, 2020

o2sh commented Dec 14, 2020 •

edited

Loading

HallerPatrick commented Dec 14, 2020

o2sh commented Dec 14, 2020

HallerPatrick commented Dec 15, 2020

o2sh commented Dec 15, 2020 •

edited

Loading

HallerPatrick commented Dec 15, 2020

spenserblack left a comment

spenserblack Dec 15, 2020

HallerPatrick Dec 15, 2020

https://github.com/o2sh/onefetch/issues/310 #341

https://github.com/o2sh/onefetch/issues/310 #341

Conversation

HallerPatrick commented Dec 10, 2020

spenserblack left a comment

Choose a reason for hiding this comment

o2sh commented Dec 11, 2020 • edited Loading

spenserblack commented Dec 12, 2020

HallerPatrick commented Dec 12, 2020

o2sh commented Dec 13, 2020 • edited Loading

HallerPatrick commented Dec 14, 2020

o2sh commented Dec 14, 2020 • edited Loading

HallerPatrick commented Dec 14, 2020

o2sh commented Dec 14, 2020

HallerPatrick commented Dec 15, 2020

o2sh commented Dec 15, 2020 • edited Loading

HallerPatrick commented Dec 15, 2020

spenserblack left a comment

Choose a reason for hiding this comment

spenserblack Dec 15, 2020

Choose a reason for hiding this comment

HallerPatrick Dec 15, 2020

Choose a reason for hiding this comment

o2sh commented Dec 11, 2020 •

edited

Loading

o2sh commented Dec 13, 2020 •

edited

Loading

o2sh commented Dec 14, 2020 •

edited

Loading

o2sh commented Dec 15, 2020 •

edited

Loading