[browser][file system] GetLogicalDrives implementation #39763
Conversation
- Directory.GetLogicalDrives threw PNSE. Follows existing code paths. - Add common DriveInfoInternal.Browser that is common code path for other implementations - Environment.GetLogicalDrives - DriveInfo.Drives Fix for FileSystemTest https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/tests/Directory/GetLogicalDrives.cs
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
kjpou1
requested review from
mdh1418,
akoeplinger,
marek-safar,
lewing and
steveisok
| @@ -15,6 +15,6 @@ public sealed partial class DriveInfo | |||
| public long TotalFreeSpace => 0; | |||
| public long TotalSize => 0; | |||
|
|
|||
| private static string[] GetMountPoints() => Environment.GetLogicalDrives(); | |||
| private static string[] GetMountPoints() => DriveInfoInternal.GetLogicalDrives(); | |||
There was a problem hiding this comment.
Is this change required since Environment.GetLogicalDrives returns the same value?
There was a problem hiding this comment.
No not required but removes an extra call because Environment.GetLogicalDrives() calls DriveInfoInternal.GetLogicalDrives();
There was a problem hiding this comment.
Why does this extra call matter in this case?
This is adding a second copy of DriveInfoInternal.Browser.cs that is much worse for Browser in particular than an extra call.
There was a problem hiding this comment.
@jkotas not sure what you mean, this follows the existing pattern for DriveInfoInternal.Unix.cs that is already there.
The extra call doesn't really matter but I see no harm in getting rid of it?
There was a problem hiding this comment.
When having a choice, I think we should be optimizing Browser for smaller footprint, not for tiny bit better throughput of APIs that are slow by design and are very unlikely to be used in Browser.
I believe that the extra copy of DriveInfoInternal.Browser.cs is causing more harm here for browser than the extra call.
This does not matter much since the difference is small and all this code is going to be removed by the linker for browser anyway.
There was a problem hiding this comment.
Oh I see, you meant the extra copy of the implementation that is compiled into the library, I thought you meant some duplicate file :)
| @@ -15,6 +15,6 @@ public sealed partial class DriveInfo | |||
| public long TotalFreeSpace => 0; | |||
| public long TotalSize => 0; | |||
|
|
|||
| private static string[] GetMountPoints() => Environment.GetLogicalDrives(); | |||
| private static string[] GetMountPoints() => DriveInfoInternal.GetLogicalDrives(); | |||
There was a problem hiding this comment.
Why does this extra call matter in this case?
This is adding a second copy of DriveInfoInternal.Browser.cs that is much worse for Browser in particular than an extra call.
|
Addressed in recent commits |
|
The wasm test failure was due to #39473 |
- Directory.GetLogicalDrives threw PNSE. Follows existing code paths. - Add common DriveInfoInternal.Browser that is common code path for other implementations - Environment.GetLogicalDrives - DriveInfo.Drives Fix for FileSystemTest https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/tests/Directory/GetLogicalDrives.cs
ghost
locked as resolved and limited conversation to collaborators
Fix for FileSystemTest https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/tests/Directory/GetLogicalDrives.cs