Skip to content

[SourceKit] Use deep stack when parsing in the XPC service #40066

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

Merged
merged 1 commit into from
Nov 15, 2021

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Nov 5, 2021

Othwerise we were performing the syntactic parsing on a background queue that had a reduced stack size which could result in stack overflows.

rdar://84474387

@ahoppen ahoppen requested a review from bnbarham November 5, 2021 09:21
@ahoppen
Copy link
Member Author

ahoppen commented Nov 5, 2021

@swift-ci Please smoke test

Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

LGTM, nice work on finding that one.

@akyrtzi
Copy link
Contributor

akyrtzi commented Nov 5, 2021

Could we only wrap the parsing part in a deep-stack WorkQueue? Kicking off a new thread for every request is a bit heavyweight since we don't implement it as a pool (it was intended for heavyweight operations like typechecking, so optimizing out the overhead of starting a new thread for that didn't seem very important).

@ahoppen ahoppen changed the title [SourceKit] Use deep stack when serving request in the XPC service [SourceKit] Use deep stack when parsing in the XPC service Nov 8, 2021
@ahoppen
Copy link
Member Author

ahoppen commented Nov 8, 2021

@swift-ci Please smoke test

Othwerise we were performing the syntactic parsing on a background queue that had a reduced stack size which could result in stack overflows.

rdar://84474387
@ahoppen
Copy link
Member Author

ahoppen commented Nov 9, 2021

Changed it to only switch to a thread with a deep stack when actually performing the parsing and added a test case because we are no longer hitting an assertion failure now that #40065 is merged.

@ahoppen
Copy link
Member Author

ahoppen commented Nov 9, 2021

@swift-ci Please smoke test

@ahoppen ahoppen merged commit ac56bfb into swiftlang:main Nov 15, 2021
@ahoppen ahoppen deleted the pr/deep-stack branch November 15, 2021 08:17
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 this pull request may close these issues.

3 participants