Log RAR "size" in ETW #6410
Log RAR "size" in ETW #6410
Conversation
Add lightweight information about the size/complexity of a RAR invocation by logging counts of the most interesting input/output lists.
rainersigwald
added
performance
Performance-Scenario-Build
There was a problem hiding this comment.
Looks good. Didn't I have something similar to this for ComputeClosure? I don't remember how that one ended.
Since this is perf oriented, I doubt we'd use it much pre-dev17, so I think it may as well go there. Open to counterarguments.
It looks like it's pointing to main, though? I thought main was already 17.0?
😠 GitHub reset the branch somehow before I hit submit I think. Retargeted to where I intended to have it all along. |
| @@ -2596,7 +2596,7 @@ out _copyLocalFiles | |||
| } | |||
| } | |||
|
|
|||
| MSBuildEventSource.Log.RarOverallStop(); | |||
| MSBuildEventSource.Log.RarOverallStop(_resolvedFiles.Length, _resolvedDependencyFiles.Length, _copyLocalFiles.Length); | |||
There was a problem hiding this comment.
Please consider to use null check/propagation, for example _copyLocalFiles?.Length at both Stop events callings. It is not guaranteed to be not null - especially in catch scenarios.
There was a problem hiding this comment.
Yes, great idea! That has bitten us before . . .
| @@ -2579,7 +2579,7 @@ out _copyLocalFiles | |||
| } | |||
| } | |||
| } | |||
| MSBuildEventSource.Log.RarOverallStop(); | |||
| MSBuildEventSource.Log.RarOverallStop(_assemblyNames.Length, _assemblyFiles.Length, _resolvedFiles?.Length ?? 0, _resolvedDependencyFiles?.Length ?? 0, _copyLocalFiles?.Length ?? 0); | |||
There was a problem hiding this comment.
(not crazy familiar with RAR)
_assemblyNames is guaranteed not to be null here? There's a null check at the beginning of the method
There was a problem hiding this comment.
I believe it is because it's initialized to an Array.Empty, but the null check is fast and easy so I'm just going to do it here too.
| @@ -2109,7 +2109,7 @@ ReadMachineTypeFromPEHeader readMachineTypeFromPEHeader | |||
| ) | |||
| { | |||
| bool success = true; | |||
| MSBuildEventSource.Log.RarOverallStart(); | |||
| MSBuildEventSource.Log.RarOverallStart(_assemblyNames?.Length ?? 0, _assemblyFiles?.Length ?? 0, _findDependencies); | |||
There was a problem hiding this comment.
Maybe do this with ?? -1 to indicate null?
Also, things like _assemblyNames and _assemblyFiles shouldn't change over the course of an execution, right? So only log them at Stop? Can probably move _findDependencies there, too, so everything is together.
There was a problem hiding this comment.
I'm also wondering if we care about the difference between _assemblyNames and _assemblyFiles. Only one should be defined, right? And if they have the same meaning (resolve this many things), maybe have like _assemblyNames?.Length ?? _assemblyFiles?.Length ?? -1
There was a problem hiding this comment.
Did you decide yea or nay on these two?
There was a problem hiding this comment.
I had not!
Looks pretty good now
I'm also wondering if we care about the difference between _assemblyNames and _assemblyFiles. Only one should be defined, right? And if they have the same meaning (resolve this many things), maybe have like
_assemblyNames?.Length ?? _assemblyFiles?.Length ?? -1
They're not the same thing and many/most builds pass into both.
Forgind
added
the
merge-when-branch-open
Context
I'm poking around the question "why do builds using the .NET SDK take longer than older non-SDK builds?" and RAR time is one major factor. It'd be nice to have a complexity estimate in the ETW events: is this RAR instance resolving 3 files, or 300?
Changes Made
Add lightweight information about the size/complexity of a RAR
invocation by logging counts of the most interesting input/output lists.
Testing
Collected updated traces manually and confirmed data present.
⚠ Note this is currently targeting 16.11. Open to discussion on pushing it to main/17.0 instead.