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

Hotfix #9110 by reverting #9021 #9169

Merged
merged 1 commit into from Aug 8, 2022
Merged

Conversation

vlad20012
Copy link
Member

@vlad20012 vlad20012 commented Aug 8, 2022

Fixes #9110

#9021 leads to a stack overflow through RsExpandedElement.getContextImpl and RsModulesIndex.getDeclarationsFor.
I think the proper fix is stopping using RsModulesIndex in RsFile.doGetCachedData, I'll do it later

changelog: Fix possible Stack overflow introduced in the previous release of the plugin

#9021 leads to a stack overflow through `RsExpandedElement.getContextImpl` and `RsModulesIndex.getDeclarationsFor`.
I think the proper fix is stopping using `RsModulesIndex` in `RsFile.doGetCachedData`
@vlad20012 vlad20012 added the fix Pull requests that fix some bug(s) label Aug 8, 2022
@vlad20012 vlad20012 added this to the v176 milestone Aug 8, 2022
@vlad20012 vlad20012 self-assigned this Aug 8, 2022
@vlad20012 vlad20012 changed the title Hotfix #9110 by reverting #9021 b53d37086a40ff50a65a1ff7a533560286294abe Hotfix #9110 by reverting #9021 Aug 8, 2022
@vlad20012
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Aug 8, 2022
9169: Hotfix #9110 by reverting #9021 r=vlad20012 a=vlad20012

Fixes #9110

#9021 leads to a stack overflow through `RsExpandedElement.getContextImpl` and `RsModulesIndex.getDeclarationsFor`.
I think the proper fix is stopping using `RsModulesIndex` in `RsFile.doGetCachedData`, I'll do it later

changelog: Fix possible Stack overflow introduced in the previous release of the plugin

Co-authored-by: vlad20012 <beskvlad@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 8, 2022

Timed out.

@vlad20012
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 8, 2022

Build succeeded:

@bors bors bot merged commit 2a985a7 into master Aug 8, 2022
@bors bors bot deleted the revert-perf-speedup-get-including-mod branch August 8, 2022 22:56
@Undin
Copy link
Member

Undin commented Aug 9, 2022

@vlad20012 don't forget to cherry-pick this change into release branch

bors bot added a commit that referenced this pull request Aug 24, 2022
9236: PERF: re-revert #9021 speed up `getIncludingMod` usages r=vlad20012 a=vlad20012

#9021 has been reverted in #9169 because of #9110. Now it can be enabled again because we got rid of recursion in `RsFile.getCachedData()` in #9171

changelog: Enable again [the optimization](#9021) reverted in the previous release. Slightly speed up name resolution

Co-authored-by: vlad20012 <beskvlad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull requests that fix some bug(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Most recent update consumes 100% (on all cores)
2 participants