-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Profile queries #43345
Merged
Merged
Profile queries #43345
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
d7ec3e1
inc comp: -Z profile-queries support; see also https://github.com/rus…
28cb03d
profiling with -Z profile-queries recognizes -Z time-passes
3c24fea
-Z profile-queries: remove panic when channel is unset
4251032
-Z profile-queries includes dep_graph.with_task uses in output
43335ae
-Z profile-query-and-key, separate from -Z profile-query; query key i…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
Do we need a separate thread? Isn't the overhead of synchronization more than that of pushing to a
Vec
or something?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.
Good question. The separate thread design was @nikomatsakis's suggestion, so I'll let him respond first.
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.
Ah, that explains a bunch. If I get a satisfactory explanation out of him this may be good to go.
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.
Well, we can measure. Just to be sure we all share the same concerns: I presume you are worried that the overhead of making and sending messages will distort the measurements, right? I admit my inclination is that separate threads are basically useful and good overall, if we can fit them in easily enough. =)
I think the reason I suggested a thread was that, with incremental compilation, we found that when we moved processing of the dependency graph into one thread, there was a significant hit, and that copying data to a separate thread was cheaper. But it's not obviously comparable -- in the incremental case, we did some effort to keep overhead low, such as pushing a series of messages into a vec and then sending the entire vec over to the thread to be processed (and using a double-buffering scheme).
In this case, in contrast, using a separate thread causes some difficulty since our keys contain region pointers and hence have to be converted to strings. And of course it's just using a channel rather than a vector, so there may be some overhead there (I'm not sure).
One question also: how many events would we wind up accumulating in this vector? I could imagine it being quite a few! But I guess we just accumulating in the current code too so that doesn't make much difference. I think I was imagining before that we would be doing some "consolidation" (e.g., summing up data or whatever) in the separate thread.
Regardless, seems like we can measure the "distortion" of a separate thread (versus a vector) just by measuring how much time the "main thread" of compilation takes, right?
TL;DR: I am happy either way really, just want to do the thing that gives us the best results.
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.
Ah, another point --
There are things we'd eventually like to profile that occur before the tcx is created, or after it is destroyed. I suspect that we will eventually refactor all the things that occur before the tcx is created away completely, but I'm not sure so sure about the stuff that comes after it is destroyed -- in particular, I think it'll always be an important win to drop the arenas before we do LLVM translation, to reduce peak memory usage. So if we have the vector containing references into those arenas, that will make it harder to include those LLVM optimizations into our measurements, presumably (but not impossible, I suppose).