-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log RAR "size" in ETW #6410
Log RAR "size" in ETW #6410
Changes from 3 commits
90127f9
da9133a
3c2e6be
2970a6b
54d6a87
01bcea2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2109,7 +2109,7 @@ ReadMachineTypeFromPEHeader readMachineTypeFromPEHeader | |
) | ||
{ | ||
bool success = true; | ||
MSBuildEventSource.Log.RarOverallStart(); | ||
MSBuildEventSource.Log.RarOverallStart(_assemblyNames?.Length ?? 0, _assemblyFiles?.Length ?? 0, _findDependencies); | ||
{ | ||
try | ||
{ | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (not crazy familiar with RAR) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it is because it's initialized to an |
||
return success && !Log.HasLoggedErrors; | ||
} | ||
catch (ArgumentException e) | ||
|
@@ -2596,7 +2596,7 @@ out _copyLocalFiles | |
} | ||
} | ||
|
||
MSBuildEventSource.Log.RarOverallStop(); | ||
MSBuildEventSource.Log.RarOverallStop(_assemblyNames.Length, _assemblyFiles.Length, _resolvedFiles?.Length ?? 0, _resolvedDependencyFiles?.Length ?? 0, _copyLocalFiles?.Length ?? 0); | ||
|
||
return success && !Log.HasLoggedErrors; | ||
} | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you decide yea or nay on these two?
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.
I had not!
Looks pretty good now
They're not the same thing and many/most builds pass into both.