-
Notifications
You must be signed in to change notification settings - Fork 57
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
Polish: inlay hints and codelens performance #634
Polish: inlay hints and codelens performance #634
Conversation
Having a look at this soon, sorry for the wait! |
@aspeddro we don't have any real benchmarks unfortunately. When I've tested I've clocked how long refreshing takes for us in that large file vs TS in a large TS file. |
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.
I think this looks good. @cristianoc might want to have a glance. @aspeddro mind pulling in the latest master too? There's a conflict.
analysis/src/Hint.ml
Outdated
}; | ||
}) | ||
| _ -> None)) | ||
match Cmt.fullFromPath ~path with |
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.
I think this is great. Question for the future: does it make sense to have some sort of cache inside of Cmt.fullFromPath
directly?
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.
The change looks good. Perhaps it's possible to rename fullFromPath
to something that makes it clear it's an expensive operation. E.g. loadFullCmtFromPath
.
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.
Done in 0f26053
Cmt.fullFromPath
for each hint.A noticeable change to
analysis/examples/large-project/src/res_core.res
Is there any way to measure?
Part of #511
cc @zth