-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[self-profiler] Make the profiler faster/more efficient #58425
Conversation
a569a0d
to
550146f
Compare
This comment has been minimized.
This comment has been minimized.
Once #58309 lands, we should do a perf run to measure the overhead. I suspect that emitting JSON instead of binary data is still more expensive than we'd like it to be. |
Probably, I haven't looked into doing binary serialization yet. Is there any existing infrastructure in the compiler for that or should I roll my own? |
(FYI, in case you were remembering the performance of serialization as it was at All-Hands, that was without buffering the writes. I added a |
There is rustc's libserialize which works OK for metadata and incremental caches. |
I don't know how easy or hard it is to use |
I think it's just a matter of |
☔ The latest upstream changes (presumably #58455) made this pull request unmergeable. Please resolve the merge conflicts. |
Yes, I think that's true as long as both the compiler and importing tool are compiled with the same compiler version. Given that there's just a couple of data structures that need to be serialized, doing something handwritten might be a viable option though. |
550146f
to
27d5873
Compare
This comment has been minimized.
This comment has been minimized.
27d5873
to
8a006f1
Compare
This comment has been minimized.
This comment has been minimized.
8a006f1
to
be88e76
Compare
This comment has been minimized.
This comment has been minimized.
be88e76
to
fff27a0
Compare
This comment has been minimized.
This comment has been minimized.
fff27a0
to
d30f158
Compare
Can you switch this to use a |
Sure, I can do that tonight. Are you trying to measure the record overhead or the save results overhead? I'll need to push a commit to enable whichever option you want to record by default since the entire feature is disabled by default. |
I think we should do both. |
Ok. Do you want to do seperate perf runs or just one with the option turned on by default? |
This comment has been minimized.
This comment has been minimized.
f50d3a1
to
855c2ef
Compare
This comment has been minimized.
This comment has been minimized.
855c2ef
to
2ba18f3
Compare
d1ac877
to
c0c7ac6
Compare
Rebased @bors r=michaelwoerister |
📌 Commit c0c7ac62b8c570ba581795d0182b720c48092b74 has been approved by |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
c0c7ac6
to
d2c2a67
Compare
This comment has been minimized.
This comment has been minimized.
This will allow us to send it across threads and measure things like LLVM time.
d2c2a67
to
f20ad70
Compare
Rebased @bors r=michaelwoerister |
📌 Commit f20ad70 has been approved by |
…erister [self-profiler] Make the profiler faster/more efficient Related to #58372 r? @michaelwoerister
☀️ Test successful - checks-travis, status-appveyor |
What is the perf-graph looking like today? |
This won't affect perf.r-l.o since it isn't run with self-profile enabled. |
I still expect it to be rather slow. Here are some thoughts on how to make it faster: #58372 |
Related to #58372
r? @michaelwoerister