Skip to content

Fixing 'stack level too deep error' in commits_in_tag #936

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 19, 2021
Merged

Fixing 'stack level too deep error' in commits_in_tag #936

merged 1 commit into from
Feb 19, 2021

Conversation

douglasmiller
Copy link
Contributor

Fixes the issue outlined here.

The commits_in_tag method was using a recursive solution to iterate over the entire graph of commits. This caused a stack level too deep error when a sufficiently large number of commits exist in the repository. This solution will produce the same results without the need for recursion/stack usage.

I also observed that the commits_in_tag method was being called multiple times for the same SHA and was very inefficient. I made a couple of optimizations which make use of a cache of already processed SHAs.

Baseline

real	1m1.836s
user	1m1.283s
sys	0m0.257s

Adding @commits_in_tag_cache

real	0m45.565s
user	0m29.704s
sys	0m0.430s

Reversing tags in fetch_tag_shas and using @commits_in_tag_cache

real	0m22.471s
user	0m7.947s
sys	0m0.416s

olleolleolle
olleolleolle approved these changes Feb 19, 2021
View reviewed changes
Copy link
Collaborator

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Yes!

♥️ 💛 💚 💙 💜

lib/github_changelog_generator/octo_fetcher.rb
# Fetch all SHAs occurring in or before a given tag and add them to
# "shas_in_tag"
#
# @param [Array] tags The array of tags.
# @return [Nil] No return; tags are updated in-place.
def fetch_tag_shas(tags)
tags.each do |tag|
tag["shas_in_tag"] = commits_in_tag(tag["commit"]["sha"])
# Reverse the tags array to gain max benefit from the @commits_in_tag_cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for leaving this code comment here, guiding other readers.

@olleolleolle olleolleolle merged commit f218bc7 into github-changelog-generator:master Feb 19, 2021
@olleolleolle
Copy link
Collaborator

Ha, I was hasty, but we have circled the issue more, now: perhaps you get a clue by these notes?

Fetching events for issues and PR: 465
Fetching closed dates for issues: 465/465
Traceback (most recent call last):
	7: from /Users/olle/.rvm/gems/ruby-2.7.1/gems/async-1.28.9/lib/async/task.rb:265:in `block in make_fiber'
	6: from /Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/generator/generator.rb:55:in `block in compound_changelog'
	5: from /Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/generator/generator.rb:152:in `fetch_issues_and_pr'
	4: from /Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/generator/generator_fetcher.rb:48:in `add_first_occurring_tag_to_prs'
	3: from /Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/generator/generator_fetcher.rb:65:in `associate_tagged_prs'
	2: from /Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/octo_fetcher.rb:353:in `fetch_tag_shas'
	1: from /Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/octo_fetcher.rb:353:in `reverse_each'
/Users/olle/opensource/github-changelog-generator/lib/github_changelog_generator/octo_fetcher.rb:354:in `block in fetch_tag_shas': undefined method `[]' for nil:NilClass (NoMethodError)

@douglasmiller douglasmiller mentioned this pull request Feb 19, 2021
Merged
@douglasmiller
Copy link
Contributor Author

Gah! In the last minute, I changed how the tag keys were accessed to make it easier to write tests. I initially tested the commits_in_tag method directly, but moved it to a private method at the end and had to adjust the testing strategy. I feel silly for not running the script again to ensure that it was still working properly 😳.

@olleolleolle
Copy link
Collaborator

Hey, thanks for taking the time to measure speed, to fix the thing, to change it to working!

@olleolleolle olleolleolle mentioned this pull request Feb 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olleolleolle olleolleolle olleolleolle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@douglasmiller @olleolleolle