-
-
Notifications
You must be signed in to change notification settings - Fork 849
Remove threading and use caching #653
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
Remove threading and use caching #653
Conversation
threads << Thread.new do | ||
issues_slice.each do |issue| | ||
find_closed_date_by_commit(issue) | ||
end | ||
end | ||
threads.each(&:join) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still wrong since the thread array and joining is inside of the each_slice
loop. This means each thread is blocked by the join, but not the group of threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But when I move the threading outside of the each_slice I get memory bloat again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the original code wasn't so terrible. It just did 25 threads for the 25 issue slices, waiting for each slice. The problem seems to be threading at all because as I reduce the number of threads the memory usage and time to complete goes down. Removing threading makes it faster and takes less memory? Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! Because adding caching for fetching commits made find_closed_date_by_commit()
slow for the first fetch and fast for following queries, but adding threading and increasing the number of threads increased the number of caches needed... >_<
The octo_fetcher caches the commits, so removing the threading around methods requesting commits from the cache reduces CPU time and memory usage. Closes github-changelog-generator#651
d7b3daa
to
1876043
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The octo_fetcher caches the commits, so removing the threading around methods requesting commits from the cache reduces CPU time and memory usage.
Closes #651