-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Reduce file existence checks when servicing directory exists #123568
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?
Reduce file existence checks when servicing directory exists #123568
Conversation
Previously, the presence of a servicing directory would enable file existence checks for all probed files (app, framework, lookup, and servicing directories). This change limits file existence checks to only when probing the servicing directory itself. This reduces I/O operations during startup for applications that have a servicing directory present but don't use additional probe paths or shared stores.
|
Tagging subscribers to this area: @agocke, @jeffschwMSFT, @elinor-fung |
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.
Pull request overview
Reduces hostpolicy startup I/O when a servicing directory exists by limiting file existence checks to servicing probes instead of enabling them globally.
Changes:
- Stop setting the global
m_needs_file_existence_checksflag solely due to the presence of the servicing directory. - When probing a servicing config, explicitly enable
is_servicing+file_existencesearch options for that probe only.
| if (entry.to_library_package_path(config.probe_dir, candidate, search_options | (config.is_servicing() ? deps_entry_t::search_options::is_servicing : 0))) | ||
| if (config.is_servicing()) | ||
| { | ||
| search_options |= deps_entry_t::search_options::is_servicing | deps_entry_t::search_options::file_existence; |
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.
Why is the file_existence flag needed now? Was it added elsewhere previously?
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.
It is also added if additional probe paths or a shared store is specified (m_needs_file_existence_check). In those cases, it is added for all probe configs.
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.
So what you're saying is setting m_needs_file_existence_checks to true sets the file_existence flag and we could technically assert the old code that that flag was set? If so, does is_servicing imply file_existence?
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.
setting
m_needs_file_existence_checksto true sets thefile_existenceflag
Yes. We initialize search_options based on m_needs_file_existence_checks.
we could technically assert the old code that that flag was set
Do you mean assert that m_needs_file_existence_checks is the same as whether search_options has the file_existence flag? Then yes. But it is initialized exactly to a couple lines above in the same function, so I don't know that it would be useful.
does
is_servicingimplyfile_existence?
Yes. I do think is_servicing could go away - it doesn't seem so much a search option as just added in there specifically for a single-file scenario, which I think we could do differently.
The presence of a servicing directory (even empty) would enable file existence checks for all files in the deps.json. We always look in the servicing directory first (if it exists) - and only care about the file existence there, not in the regular app directory. This change limits file existence checks to only when probing the servicing directory itself.
This reduces I/O operations during startup for applications that have a servicing directory present (and don't use additional probe paths or shared stores).
For the staticconsoletemplate startup test in dotnet/performance, running locally on Windows, if an empty servicing directory exists:
@dotnet/appmodel @AaronRobinsonMSFT