perfview
perfview copied to clipboard
microsoft
Fix detection of PerHeapHistories events presence
Trivial fix.
The presence of per heap history events is detected by comparing PerHeapHistories with null, but this property is never null, since the list is allocated in the constructor.
https://github.com/microsoft/perfview/blob/dec5b67a215a446863285169d4be09b4f91180d1/src/TraceEvent/Computers/TraceManagedProcess.cs#L2507
As a result, even though we can fallback to allocation ticks, we never fallback and if the history events are missing, the allocation rate is unavailable or incorrect.
We can check for Count == 0 instead and then fallback works.
There's a bunch of other places where the null comparison is used for PerHeapHistories. Worth changing it in all places? Also, I guess, because it's never null, the ?. is not needed, just . would do, wouldn't it?
There's a bunch of other places where the null comparison is used for PerHeapHistories.
Right. There are many. Every use checks for null. It does look like it was intended to be a lazy-allocated collection. Perhaps a better fix would be to make it lazy-allocated.
@brianrob Do you agree with the change to have the collection lazily initialized?
It's not clear to me that this change is safe. I do see a number of places that check for null and Count > 0. I also see several that just check for null. And then I see some that do not. For example: https://github.com/microsoft/perfview/blob/main/src/TraceEvent/Computers/TraceManagedProcess.cs#L2989.
I also wonder about the fact that the PerHeapHistories field is public, and so this may also be a breaking change. It does seem that we'd want to fix the detection of these events, but I am wondering if that just needs to be done by checking for Count > 0 in more places?
I am wondering if that just needs to be done by checking for Count > 0 in more places?
In the last iteration I added checks for both null and Count > 0, whichever was missing. That would be the most conservative way to check, but perhaps the safest too.