Remove unnecessary realpath calls from host probing logic #50671
Conversation
ghost
added
the
|
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
vitek-karas
left a comment
There was a problem hiding this comment.
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.
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.
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).
| StandaloneAppFixture_Localized = localizedFixture; | ||
| StandaloneAppFixture_Published = publishFixture; |
There was a problem hiding this comment.
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.
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. | ||
| bool to_dir_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str, bool& found_in_bundle) const; | ||
| bool to_dir_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str, bool& found_in_bundle, bool check_file_existence) const; |
There was a problem hiding this comment.
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.
5 participants
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 in 5265305
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 in 8ad1d7d
runtime/src/native/corehost/hostpolicy/deps_resolver.cpp
Line 48 in 8ad1d7d
Which purpose is to unroll the symbolic link of a given path and, in the later case, to later normalize the path.