search

microsoft / perfview

PerfView is a CPU and memory performance-analysis tool
http://channel9.msdn.com/Series/PerfView-Tutorial
MIT License
4.14k stars 707 forks source link

Fix detection of PerHeapHistories events presence #1979

Closed VSadov closed 6 months ago

VSadov commented 8 months ago

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.

cincuranet commented 8 months ago

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?

VSadov commented 8 months ago

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.

cincuranet commented 8 months ago

@brianrob Do you agree with the change to have the collection lazily initialized?

brianrob commented 8 months ago

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?

VSadov commented 7 months ago

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.