Skip to content
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

Stack overflow in pulls(...).get(...) #136

Closed
karyon opened this issue Oct 27, 2021 · 5 comments · Fixed by #148
Closed

Stack overflow in pulls(...).get(...) #136

karyon opened this issue Oct 27, 2021 · 5 comments · Fixed by #148

Comments

@karyon
Copy link

karyon commented Oct 27, 2021

edit: I can reproduce this with older version as well now. I'm thoroughly confused and would appreciate any advice.

With current git, the following code results in thread 'main' has overflowed its stack.

    octocrab::instance()
        .pulls("XAMPPRocky", "octocrab")
        .get(129u64)
        .await?;

I have reduced this to


    let repo_json = r#"{ "user": { }, }"#;

    let pr_deserialized: PullRequest = serde_json::from_str(repo_json).unwrap();

Deserializing a User is fine, it's just the User in the PullRequest (note the additional {} around user) that reproduces the problem.

edit: I now cannot repro this anymore with the snippet above, but rather this makes it crash now.


    let repo_json = r#"{
        "head": {
          "repo": {
          }
        }
      }
      "#;
    let pr_deserialized: PullRequest = serde_json::from_str(repo_json).unwrap();
@karyon karyon changed the title Stackoverflow in pulls().get() Stackoverflow in pulls(...).get(...) Oct 27, 2021
@karyon karyon changed the title Stackoverflow in pulls(...).get(...) Stack overflow in pulls(...).get(...) Oct 27, 2021
@zhongsp
Copy link

zhongsp commented Nov 2, 2021

The same issue in 0.13.

let pr = octocrab.pulls(owner, repo).get(1).await?;
println!("{}", pr.title);

Result:

thread 'main' has overflowed its stack
error: process didn't exit successfully: `target\debug\test.exe` (exit code: 0xc00000fd, STATUS_STACK_OVERFLOW)

@wayofthepie
Copy link
Contributor

I can look at this over the weekend. Something is using a lot of stack space when deserializing. I came across a similar issue when writing the deserializer for events. It only seems to really be a problem when run in debug, it gets optimized away in release. At least in the cases I found.

@wayofthepie
Copy link
Contributor

wayofthepie commented Nov 12, 2021

This one has been on my mind for a while, as it's come up in other things also. So going to put all the info I uncovered here. Here is a minimum program to reproduce:

#[tokio::main(worker_threads = 1)]
async fn main() -> octocrab::Result<()> {
    let x = octocrab::instance()
        .pulls("XAMPPRocky", "octocrab")
        .get(129u64)
        .await?;
    println!("{:#?}", x.id);
    Ok(())
}

Stack usage in debug (using https://github.com/d99kris/stackusage):

$ stackusage target/debug/stackoverflow 1>/dev/null
stackusage log at 2021-11-12 19:55:06 ----------------------------------------
  pid  id    tid  requested     actual     maxuse  max%    dur               funcP name
175997   0  175997    8388608    8384512    1889704    22      0               (nil) stackoverflow
175997   1  175998    2097152    2097152      68424     3      0      0x557526111b30 tokio-runtime-w
175997   2  175999    2097152    2097152      27016     1      0      0x557526111b30 tokio-runtime-w

Max stack usage is 1889704 bytes, ~1.8MiB. But when built in release mode:

$ stackusage target/release/stackoverflow 1>/dev/null
stackusage log at 2021-11-12 19:59:37 ----------------------------------------
  pid  id    tid  requested     actual     maxuse  max%    dur               funcP name
176208   0  176208    8388608    8376320     600080     7      1               (nil) stackoverflow
176208   1  176209    2097152    2097152      24040     1      1      0x55b922b9b310 tokio-runtime-w
176208   2  176210    2097152    2097152      23160     1      1      0x55b922b9b310 tokio-runtime-w

This drops to ~600KiB. We are only deserializing a PullRequest, so what does that look like on the stack:

struct octocrab::models::pulls::PullRequest
	size: 25528
	members:
		0[24]	url: struct alloc::string::String
		24[8]	id: struct octocrab::models::PullRequestId
		32[24]	node_id: struct core::option::Option<alloc::string::String>
		56[88]	html_url: struct core::option::Option<url::Url>
		144[88]	diff_url: struct core::option::Option<url::Url>
		232[88]	patch_url: struct core::option::Option<url::Url>
		320[88]	issue_url: struct core::option::Option<url::Url>
		408[88]	commits_url: struct core::option::Option<url::Url>
		496[88]	review_comments_url: struct core::option::Option<url::Url>
		584[88]	review_comment_url: struct core::option::Option<url::Url>
		672[88]	comments_url: struct core::option::Option<url::Url>
		760[88]	statuses_url: struct core::option::Option<url::Url>
		848[8]	number: u64
		856[24]	title: struct core::option::Option<alloc::string::String>
		880[1168]	user: struct core::option::Option<octocrab::models::User>
		2048[24]	body: struct core::option::Option<alloc::string::String>
		2072[24]	body_text: struct core::option::Option<alloc::string::String>
		2096[24]	body_html: struct core::option::Option<alloc::string::String>
		2120[24]	labels: struct core::option::Option<alloc::vec::Vec<octocrab::models::Label, alloc::alloc::Global>>
		2144[1640]	milestone: struct core::option::Option<octocrab::models::Milestone>
		3784[24]	active_lock_reason: struct core::option::Option<alloc::string::String>
		3808[24]	merge_commit_sha: struct core::option::Option<alloc::string::String>
		3832[1168]	assignee: struct core::option::Option<octocrab::models::User>
		5000[24]	assignees: struct core::option::Option<alloc::vec::Vec<octocrab::models::User, alloc::alloc::Global>>
		5024[24]	requested_reviewers: struct core::option::Option<alloc::vec::Vec<octocrab::models::User, alloc::alloc::Global>>
		5048[24]	requested_teams: struct core::option::Option<alloc::vec::Vec<octocrab::models::teams::RequestedTeam, alloc::alloc::Global>>
		5072[6936]	head: struct octocrab::models::pulls::Head
		12008[6936]	base: struct octocrab::models::pulls::Base
		18944[792]	links: struct core::option::Option<octocrab::models::pulls::Links>
		19736[24]	author_association: struct core::option::Option<alloc::string::String>
		19760[5696]	repo: struct core::option::Option<octocrab::models::Repository>
		25456[16]	created_at: struct core::option::Option<chrono::datetime::DateTime<chrono::offset::utc::Utc>>
		25472[16]	updated_at: struct core::option::Option<chrono::datetime::DateTime<chrono::offset::utc::Utc>>
		25488[16]	closed_at: struct core::option::Option<chrono::datetime::DateTime<chrono::offset::utc::Utc>>
		25504[16]	merged_at: struct core::option::Option<chrono::datetime::DateTime<chrono::offset::utc::Utc>>
		25520[1]	state: struct core::option::Option<octocrab::models::IssueState>
		25521[1]	locked: bool
		25522[1]	maintainer_can_modify: bool
		25523[1]	mergeable: struct core::option::Option<bool>
		25524[1]	mergeable_state: struct core::option::Option<octocrab::models::pulls::MergeableState>
		25525[1]	rebaseable: struct core::option::Option<bool>
		25526[1]	draft: struct core::option::Option<bool>
		25527[1]	<padding>

This output was from ddbug. Can search for '^struct octocrab::models::pulls::PullRequest$' in the output of:

$ ddbug target/debug/stackoverflow 

Head, Base, User, and Repository are some fields that take up the most space. Boxing Head and Base in PullRequest reduces the stack usage of PullRequest by over half:

struct octocrab::models::pulls::PullRequest
	size: 11672
...

And the overall stack usage in debug drops to ~1.1MiB:

$ stackusage target/debug/stackoverflow 1>/dev/null
stackusage log at 2021-11-12 20:18:01 ----------------------------------------
  pid  id    tid  requested     actual     maxuse  max%    dur               funcP name
179643   0  179643    8388608    8384512    1121304    13      1               (nil) stackoverflow
179643   1  179644    2097152    2097152      68424     3      1      0x55842a6b93c0 tokio-runtime-w
179643   2  179645    2097152    2097152      27016     1      1      0x55842a6b93c0 tokio-runtime-w

If we also Box User and Repository in PullRequest, usage drops to ~700KiB:

$ stackusage target/debug/stackoverflow 1>/dev/null
stackusage log at 2021-11-12 20:20:18 ----------------------------------------
  pid  id    tid  requested     actual     maxuse  max%    dur               funcP name
181200   0  181200    8388608    8380416     743640     8      0               (nil) stackoverflow
181200   1  181201    2097152    2097152      68424     3      0      0x556c6ad7bb80 tokio-runtime-w
181200   2  181202    2097152    2097152      27016     1      0      0x556c6ad7bb80 tokio-runtime-w

There are a few more fields which look useful to box, Milestone, Assignee, Links, boxing those:

$ stackusage target/debug/stackoverflow 1>/dev/null
stackusage log at 2021-11-12 20:36:10 ----------------------------------------
  pid  id    tid  requested     actual     maxuse  max%    dur               funcP name
190475   0  190475    8388608    8380416     548040     6      1               (nil) stackoverflow
190475   1  190476    2097152    2097152      68424     3      1      0x559e3ef9c040 tokio-runtime-w
190475   2  190477    2097152    2097152      27016     1      1      0x559e3ef9c040 tokio-runtime-w

Usage drops to 548KiB. The usage in release mode drops to 182KiB:

$ stackusage target/release/stackoverflow 1>/dev/null
stackusage log at 2021-11-12 20:56:58 ----------------------------------------
  pid  id    tid  requested     actual     maxuse  max%    dur               funcP name
197421   0  197421    8388608    8384512     194672     2      2               (nil) stackoverflow
197421   1  197422    2097152    2097152      24040     1      2      0x55ff48966710 tokio-runtime-w
197421   2  197423    2097152    2097152      23160     1      2      0x55ff48966710 tokio-runtime-w

The size of PullRequest on the stack after all this boxing (in debug mode):

struct octocrab::models::pulls::PullRequest
	size: 1248

I don't really get how PullRequest's size translates into such a large overall stack usage in debug mode, but boxing the larger fields has dropped overall usage by a massive amount!

There are likely other structs which have the same issue, but I can open a PR with these fields boxed to fix this one at least. I'll probably try dig deeper into the underlying cause too at some point.

@XAMPPRocky
Copy link
Owner

@wayofthepie Thank you for investigating! Yeah, I'm not sure why it's such a large amount in debug either. Maybe it's because in debug it's not removing all the stack frames from the nested calls and each of the fields in each stack frame is adding up to a lot. But that's just me guessing. Feel free to open a PR boxing those fields.

@zhongsp
Copy link

zhongsp commented Nov 16, 2021

How about releasing a new version for this fix?

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 a pull request may close this issue.

4 participants