- Notifications
You must be signed in to change notification settings - Fork5.2k
Remove unnecessary realpath calls from host probing logic#50671
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedApr 3, 2021
Tagging subscribers to this area:@vitek-karas,@agocke,@VSadov Issue DetailsIn an effort to improve startup time, this PR gets rid of three different file-accessing calls inside the host:
Which performs a file existence check on each of the assets inside a deps.json. And:
Which purpose is to unroll the symbolic link of a given path and, in the later case, to later normalize the path.
|
vitek-karas 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.
Basically just some more interesting test cases, otherwise looks good.
| if(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| // Set code page to output unicode characters. | ||
| Command.Create("chcp 65001").Execute(); | ||
| } |
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.
This test is disabled no Windows as per above if - maybe just leave a comment, that enabling this on Windows will require running this command.
src/native/corehost/deps_entry.cpp Outdated
| else | ||
| { | ||
| trace::verbose(_X(" %s path query exists %s"), query_type, candidate.c_str()); | ||
| trace::verbose(_X(" %s path query %s"), query_type, candidate.c_str()); |
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.
Nit - maybe include(skipped existence check) or something like that, so that it's easy to determine what happened (I know the strings are different already, but they're very similar).
Uh oh!
There was an error while loading.Please reload this page.
| StandaloneAppFixture_Localized=localizedFixture; | ||
| StandaloneAppFixture_Published=publishFixture; |
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.
Couple more tests we might want to add:
- Framework dependent app - run it from a symlink
- Framework dependent app where runtime install location is a symlink (basically
DOTNET_ROOTpoints to a symlink) - Running an app via
dotnet app.dllwhere thedotnetis a symlink. (I think this is close to the snap scenario)
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.
Some more
- Make sure we have a test to validate additional probing paths (that they continue to work even after this change)
src/native/corehost/deps_entry.h Outdated
| // Given a "base" dir, yield the file path within this directory or single-file bundle. | ||
| boolto_dir_path(constpal::string_t&base,boollook_in_bundle,pal::string_t*str,bool&found_in_bundle)const; | ||
| boolto_dir_path(constpal::string_t&base,boollook_in_bundle,pal::string_t*str,bool&found_in_bundle,boolcheck_file_existence)const; |
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.
Would strongly recommend keeping the last parameter as the out. In this case we now have "in in out out in". Also, we now have two booleans in these cases which means we are close to warranting an enumeration. At the very least though all of these functions should be updated to keep the outs as the last parameters.
Uh oh!
There was an error while loading.Please reload this page.
In an effort to improve startup time, this PR gets rid of three different file-accessing calls inside the host:
runtime/src/native/corehost/hostpolicy/deps_resolver.cpp
Line 654 in5265305
Which performs a file existence check on each of the assets inside a deps.json.
And:
runtime/src/native/corehost/hostpolicy/deps_resolver.cpp
Line 593 in8ad1d7d
runtime/src/native/corehost/hostpolicy/deps_resolver.cpp
Line 48 in8ad1d7d
Which purpose is to unroll the symbolic link of a given path and, in the later case, to later normalize the path.