-
Notifications
You must be signed in to change notification settings - Fork 4
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
Type inference profiling #16
Conversation
Error in CI:
In some Julia versions but not others? Wut? 🤔 |
src/ProfileEndpoints.jl
Outdated
|
||
precompile(typeinf_start_endpoint, (HTTP.Request,)) || error("precompilation of package functions is not supposed to fail") | ||
precompile(typeinf_stop_endpoint, (HTTP.Request,)) || error("precompilation of package functions is not supposed to fail") |
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 don't know why we did all of these this way, but it seems a bit silly now...
Can we maybe switch them all to @assert precompile(...)
instead of precompile(...) || error(...)
? It's shorter, easier to read, and easier to copy/paste. If you don't want to do that change in this PR tho, we can do it later 👍
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.
@vilterp: This is such a good idea! I'm really glad you thought of this - we should have started with this from the beginning! 💡
It'll make it so much easier to grab profiles, and it'll let us farm out the work to anyone interested, and it can unblock the work we did so that at least we have something enabled. Great idea. :)
I'd love to also do it for LLVM opt profiling - maybe we can do that in a followup once this one is merged. Thanks again, great idea 😊
Huh yeah i'm not sure how to explain the CI failure.. :/ it should go away if we move the flamegraph conversion out of this package and then we can maybe restrict our client code to a working version of the SnoopCompile package if that's the problem |
86e3a24
to
0f81977
Compare
Ah okay actually, i think the test failure was because you weren't finishing the cleanup on the ROOT timing node in your version. It's fixed now, because we switched to just calling the functions directly from SnoopCompile. I think this is good to go then! |
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.
Thanks @vilterp! LGTM
TODO: