-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Proper handling $crate Take 2 #7145
Conversation
Couple of quick observations before review:
|
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.
Looks reasonable!
Whats the impact on memory use? per_query_memory_usage
needs to account for new query.
fe0101a
to
76f2b9d
Compare
I added
IIUC, we only store token range in
We store the However, I changed it to just store the starting position (which is |
Memory used : (
|
yeah, that's quite high there. But we can't really not do this, so I think it's better to merge and optimize |
Then bite the bullet now :) bors r+ |
Similar to previous PR (#7133) , but improved the following things :
ExpansionInfo
, we store a similar but stripped versionHygieneInfo
.SyntaxNode
(because every token we are interested are IDENT), we store theTextRange
only.The overall speed compared to previous PR is much faster (65s vs 45s) :
HOWEVER, it is still a perf regression (35s vs 45s):