Avoid unnecessary closures/delegates in Process #50496
Conversation
|
Tagging subscribers to this area: @carlossanlop Issue Details
|
| { | ||
| return Id == possibleChildProcess.ParentProcessId; | ||
| } | ||
| catch (Exception e) when (IsProcessInvalidException(e)) |
There was a problem hiding this comment.
Could the code also be changed to not use exception filters
There was a problem hiding this comment.
I agree with @jkotas that going through and removing exception filters is a step in the wrong direction.
#50281 (comment)
There was a problem hiding this comment.
What is the benefit of using generic Exception with filters over catch for specific exceptions?
There was a problem hiding this comment.
All of the places that handle these exceptions then need to duplicate the list. And each of them needs its own catch block with the logic duplicated.
There was a problem hiding this comment.
I think that only hides the real problem here. The better improvement would be not to use exceptions for normal flow execution here. For example by changing ParentProcessId to return int? and catch the only exceptions which can really happen on specific platforms instead of generic IsProcessInvalidException.
There was a problem hiding this comment.
The better improvement would be not to use exceptions for normal flow execution here.
It's not using exceptions for "normal flow" here. It's using exceptions for exceptional situations, e.g. a race condition between a process being alive one moment and then not the next. Making a blanket statement that exceptions shouldn't be used for control flow is akin to saying code should never have a try/catch and all throws should instead be fail-fasts. That ship sailed over two decades ago.
It's also entirely orthogonal to this PR. This PR is not introducing any new exceptions in any new places, nor is it eating exceptions any more or any less than before. It's only changing the manner in which those same exceptions are caught.
There was a problem hiding this comment.
It's also entirely orthogonal to this PR.
Fair enough. I didn't want to block this just pointing out possible better implementation. I might give it a shot once this lands.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
Show resolved
Hide resolved
ILSpy, searching for DisplayClass across all of netcoreapp, and then walking through them looking for ones that looked easily avoidable / unnecessary / mistakes.
I don't think a tool can say whether a closure should be removed or not; sometimes they're the right tool for the job. It becomes a judgement call around ease of removal / maintenance impact, performance benefits/costs, and hotness of the path. I submitted a handful of PRs to fix the ones across netcoreapp that on quick look met my threshold: |
* upstream/main: (568 commits) [wasm] Set __DistroRid on Windows to browser-wasm (dotnet#50842) [wasm] Fix order of include paths, to have the obj dir first (dotnet#50303) [wasm] Fix debug build of AOT cross compiler (dotnet#50418) Fix outdated comment (dotnet#50834) [wasm][tests] Add properties to allow passing args to xharness (dotnet#50678) Vectorized common String.Split() paths (dotnet#38001) Fix binplacing symbol files. (dotnet#50819) Move type check to after the null ref branch in out marshalling of blittable classes. (dotnet#50735) Remove extraneous CMake version requirement. (dotnet#50805) [wasm] Remove unncessary condition for EMSDK (dotnet#50810) Add loop alignment stats to JitLogCsv (dotnet#50624) Resolve ILLink warnings in System.Diagnostics.DiagnosticSource (dotnet#50265) Avoid unnecessary closures/delegates in Process (dotnet#50496) Fix for field layout verification across version bubble boundary (dotnet#50364) JIT: Enable CSE for VectorX.Create (dotnet#50644) [main] Update dependencies from mono/linker (dotnet#50779) [mono] More domain cleanup (dotnet#50771) Race condition in Mock reference tracker runtime with GC. (dotnet#50804) Remove IAssemblyName (and various fusion remnants) (dotnet#50755) Disable failing test for GCStress. (dotnet#50828) ...
ghost
locked as resolved and limited conversation to collaborators
No description provided.