Optimise logs #111
Optimise logs #111
Conversation
Usage of logs slows down significantly generation of the docs, adding a cache mechanism
|
@timvink I realise that logs slows down significantly generation of document, adding a caching mechanism on page level, let me know if you would prefer it somewhere else. |
| self._authors.append(co_author) | ||
| co_author.add_lines(self, commit) | ||
|
|
||
| def _get_git_log(self, sha, commit): |
There was a problem hiding this comment.
Can we rename this function? It's only used for parsing co-authors?
Also, we should have already parsed all the commit earlier:
It's better to extract and add the co-author information in the Commit class:
then use it here. You won't need to have the caching in that case, and it should be even faster as you're not running git log again.
There was a problem hiding this comment.
Good point, although that raises the question on which class should be responsible for calling git log? Updated using the repository to deduce git logs, let me know if this is not desirable
There was a problem hiding this comment.
Alternative I can pass a function callback "fetch_co_authors" in repo.get_commit or "break it down" between calling constructor and returning cached commit.
There was a problem hiding this comment.
Yeah I didn't read up enough, my bad. Your approach seems very reasonable. We're running git blame, and you added (a cached) git_log to get the co-author information. Either way, we're going to have to make a lot more git calls, which is a lot slower.
Maybe the better option is to not only have the caching, but also make processing co-authors optional (and disabled by default). The reason being is that the mainstream user flows will take a big performance hit to support a niche feature (git co-authors are not very common).
Thoughts?
There was a problem hiding this comment.
good call, added option add_co_authors with false by default.
I noticed that repo already uses a git command, maybe it's fine to do git log as well?
There was a problem hiding this comment.
Nice one, left one comment.
I noticed that repo already uses a git command, maybe it's fine to do git log as well?
We need git blame for line-by-line authors. We need git log to find co-authors. So we'd have to do two git calls, which is expensive (compute time). Or maybe you have a better, more efficient way in mind?
There was a problem hiding this comment.
unfortunately no, need to call git log in addition but with cache it is much faster :) (unless each line is a different commit ofc)
- moving responsibility of deducing co-author to repo - added co-author inside commit
docs/options.md
Outdated
| @@ -8,6 +8,7 @@ plugins: | |||
| show_contribution: true | |||
| show_line_count: true | |||
| show_email_address: true | |||
| add_co_authors: true | |||
There was a problem hiding this comment.
can you also add the documentation of the option lower down in this file?
| self._authors.append(co_author) | ||
| co_author.add_lines(self, commit) | ||
|
|
||
| def _get_git_log(self, sha, commit): |
There was a problem hiding this comment.
Nice one, left one comment.
I noticed that repo already uses a git command, maybe it's fine to do git log as well?
We need git blame for line-by-line authors. We need git log to find co-authors. So we'd have to do two git calls, which is expensive (compute time). Or maybe you have a better, more efficient way in mind?
|
@timvink thanks a lot for your fast support on this change (and sorry I didn't see it before :) ) , will you release a new version for this change? |
|
Yes, doing it now. |
Usage of logs slows down significantly generation of the docs, adding a cache mechanism