-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Dependency Scanning] Add a compiler statistic for # of dependency module queries and a simple test for it #85912
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
base: main
Are you sure you want to change the base?
Conversation
…ncy module queries and a simple test for it This adds a compiler statistic for # of named lookups the scanner performed.
|
@swift-ci smoke test |
cachemeifyoucan
left a comment
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 am not quite sure the purpose of this. Is the number supposed to be 1 per module? Is the number always equal to the number of module in the dependencies?
If so, why not just assert when the number is not matching, rather than hide behind stats?
cachemeifyoucan
left a comment
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.
Change to request change to block auto merge.
There are a couple of goals to these stats.
|
|
I prefer just collect both number of modules and queries, assert when they do not match. The assert can be added for clang module now, swift module later. For incremental build, I think it might be better to have a remark, rather than a stats, because that is only available in assert build. |
I do not think this is a good approach. In expectation, the number of named queries of Swift modules is higher than the number of discovered Swift modules because many named Swift module queries fail and require a Clang module query for the same identifier. We will not be able to have such an assert for Swift modules at all. For Clang modules such an assert would trigger on any failure to resolve a dependency and that is also not desirable, even in a debug compiler, because we would like the scanner to emit the expected error diagnostics and output. Even if the assert were to be placed after these error diagnostics are emitted, this is a failure scenario which does not justify having the compiler assert exit any time a module dependency cannot be resolved, it is already going to result in a scanning action failure and in most cases we want to be able to capture the scanner output, still. Having tests in place which use these metrics to ensure we cover common simple scenarios provides the required coverage without complicating the scanner's failure mode (depending on debug build or not). This will still help ensure that for scanning actions which do not fail we have a guard in place to prevent code changes which introduce redundant scanning queries.
I'm aware that these are only available in assert build, but since my primary motivation for these is to facilitate testing and debugging by compiler engineers I believe that to be just fine. I do not see what value these metrics would carry to a compiler's end user to justify a new remark kind here. The statistics infrastructure exists for use cases very much like this one. |
|
You can always add computation in assert build to make sure a successful dependency scanning has expected number of queries comparing to the final output. I just don't think one test case here can prevent any regression other than the case listed here. Unless you think it is impossible to come up a rule, I feel just asserting when query number is too big is the better approach. The reason I want remark is you can easily ask user to pass in the flag to know if incremental scanning is doing anything or not when you got a reported problem. |
This adds a compiler statistic for # of named lookups the scanner performed.
This change is a follow-up to #85849 to ensure we do not encounter a similar regression in the future.
Furthermore, these stats have shown that we have some redundancy in our Swift module dependency queries which I intend to address in a follow-up PR.
I also intend to begin using these statistics to add more-concrete verification of incremental dependency scanning. They will help ensure in the lit test suite that we successfully minimize re-queries on a given incremental scan.