Skip to content

Conversation

@artemcm
Copy link
Contributor

@artemcm artemcm commented Dec 8, 2025

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.

…ncy module queries and a simple test for it

This adds a compiler statistic for # of named lookups the scanner performed.
@artemcm
Copy link
Contributor Author

artemcm commented Dec 8, 2025

@swift-ci smoke test

@artemcm artemcm enabled auto-merge December 8, 2025 22:54
Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a 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?

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a 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.

@artemcm
Copy link
Contributor Author

artemcm commented Dec 8, 2025

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?

There are a couple of goals to these stats.

  • Prevent regressions where we introduce redundant queries (e.g. [Dependency Scanning] Do not re-query a given clang module identifier more than once #85849).
  • Adding these stats also helped me see that we currently actually have some redundancy in our Swift module dependency queries which I intend to address in a follow-up PR.
  • I 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.

@cachemeifyoucan
Copy link
Contributor

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.

@artemcm
Copy link
Contributor Author

artemcm commented Dec 9, 2025

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.

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.

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'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.

@cachemeifyoucan
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants