Skip to content
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

System.Diagnostics.PerformanceCounter crashes on SurfacePro X #41144

Closed
JensNordenbro opened this issue Aug 21, 2020 · 20 comments
Closed

System.Diagnostics.PerformanceCounter crashes on SurfacePro X #41144

JensNordenbro opened this issue Aug 21, 2020 · 20 comments
Labels
arch-arm64 area-System.Diagnostics.PerformanceCounter enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@JensNordenbro
Copy link

JensNordenbro commented Aug 21, 2020

Description

We have created a .NET5 preview7 application where we utilize Performance counters.

On a development machine this works well, but on SurfaceProX the application crashes.

Looking into the PerformanceCounter code in the netcoreapp2.0 folder inside the nuget, it seems to be reliant on registry keys for .NET Framework to be installed in order to load the native dll perfcounter.dll from a directory specified by that those keys.

image

We expected three things:

  1. No reliance on .NET Framework
  2. No reliance on Windows registry
  3. Performance counters to work on SurfaceProX.

Is this a bug or a limitation to how far you want to support System.Diagnostics.

This has apparently been a problem before: https://developercommunity.visualstudio.com/content/problem/310772/module-not-found-exception-when-calling-countersam.html?childToView=394219#comment-394219

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Diagnostics.Tracing untriaged New issue has not been triaged by the area owner labels Aug 21, 2020
@ghost
Copy link

ghost commented Aug 21, 2020

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Aug 21, 2020

PerformanceCounter is explicitly a Windows thing that isn't supported on other OSes - since .NET Framework is part of Windows it makes sense for me that it tries to use .NET Framework assembly / registry.

Looking at the linked issue on the developer community it looks like that particular issue might have been fixed in .NET Core 3.0 by dotnet/corefx#29009 - This looks like a similar, but separate issue, perhaps specific to Windows 10 on ARM.

Do you have any more details on how the app crashes (stack trace, exception thrown etc)?

(Speculation) I'm curious if .NET Framework is built for ARM, or if it runs on the x86 emulation layer - perhaps the library itself exists on the path, but it fails to load because .NET Core is built for ARM64 - since it's not on the emulation layer and it doesn't want to load x86 libraries?

@tarekgh
Copy link
Member

tarekgh commented Aug 21, 2020

@JensNordenbro this is limitation in the performance counter implementation. It depends on the registry and registry API. not only to load the native library but also to read the performance data too https://docs.microsoft.com/en-us/dotnet/api/microsoft.win32.registry.performancedata?view=dotnet-plat-ext-3.1. The code is accessing the registry to get the native library path only because we support the remote scenarios which can read perf counters from remote servers. Although this is currently limitation for .NEt Core but I think this can be enhanced in the next releases.

@Anipik please triage this accordingly.

CC @danmosemsft @ericstj I think we need to track enhancing this in the next releases.

@JensNordenbro
Copy link
Author

Maybe assemblies like these should be provided through Microsoft.Windows.Compatibilty nuget or only as an individual package If there is no intention of making it cross platform.

@JensNordenbro
Copy link
Author

Also it is strange to require installation of netfw for a net core app to work since netfw is optional on Windows 10 If I remember correctly.
It is also strange that this would make win-arm64 a second grade citizen just because netfw does not exist and is not planned for that architecture.

@tarekgh
Copy link
Member

tarekgh commented Aug 21, 2020

Also it is strange to require installation of netfw for a net core app to work since netfw is optional on Windows 10 If I remember correctly.
It is also strange that this would make win-arm64 a second grade citizen just because netfw does not exist and is not planned for that architecture.

that is why I said we need to track enhancing this in the next releases..

@tarekgh tarekgh added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Aug 21, 2020
@tarekgh tarekgh added this to the 6.0.0 milestone Aug 21, 2020
@danmoseley
Copy link
Member

On which SKU's is .NET Framework optional?

I'm wonder what other places we rely on .NET Framework - the only one I know of is System.Management, where the native part of the implementation is in .NET Framework. That's inevitable.

@Anipik
Copy link
Contributor

Anipik commented Aug 22, 2020

We have new windows api(pdh.dll) which don't require registry to get the performance counter values.
However we currently don't have .Net Wrapper over them, there have been couple of problems in the past with these registry apis.

@ericstj
Copy link
Member

ericstj commented Nov 20, 2020

I'm wonder what other places we rely on .NET Framework - the only one I know of is System.Management, where the native part of the implementation is in .NET Framework. That's inevitable.

So this just came up and I noticed System.Management, System.Diagnostics.PerformanceCounter, and System.Diagnostics.EventLog all try to load binaries from the .NETFramework directory.

@ghost
Copy link

ghost commented Nov 24, 2020

Tagging subscribers to this area: @tommcdon, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

We have created a .NET5 preview7 application where we utilize Performance counters.

On a development machine this works well, but on SurfaceProX the application crashes.

Looking into the PerformanceCounter code in the netcoreapp2.0 folder inside the nuget, it seems to be reliant on registry keys for .NET Framework to be installed in order to load the native dll perfcounter.dll from a directory specified by that those keys.

image

We expected three things:

  1. No reliance on .NET Framework
  2. No reliance on Windows registry
  3. Performance counters to work on SurfaceProX.

Is this a bug or a limitation to how far you want to support System.Diagnostics.

This has apparently been a problem before: https://developercommunity.visualstudio.com/content/problem/310772/module-not-found-exception-when-calling-countersam.html?childToView=394219#comment-394219

Author: JensNordenbro
Assignees: -
Labels:

area-System.Diagnostics, enhancement

Milestone: 6.0.0

@ericstj
Copy link
Member

ericstj commented Nov 24, 2020

I took a look at this perfcounter case and it looks to me like the managed code is unnecessarily using that .NETFramework DLL. The FormatRawValue function is the only method called and this is just a pass through to PdhFormatRawValue. I think we can just remove this dependency and call directly into pdh.dll.

PerfCounter.dll is more important for reading .NET Performance counters. What happens in this case is the .NET Performance counter is registered as implemented in performance dll netfxperf.dll. NetFxPerf.dll just decides which version of perfcounter.dll to load which is in the .NETFramework. When a .NET Performance counter is created from a .NET application for writing the managed side of this code opens up a shared memory file in a known location and writes the counter information to it. When a counter is read from any type of application, the consumer goes through the registry as has been mentioned, which loads up netfxperf.dll which loads perfcounter.dll (into their own process) and perfcounter.dll knows how to access and read the shared memory that is written by the .NET process.

So this means that we might be able to easily make reading non-.NETFramework data work, and writing of .NETFramework counters would work (by writing to shared memory that no-one is reading), but actual .NET performance counter reading will not likely work unless we are able to install netfxperf.dll and perfcounter.dll on the system.

I'd like to hear more from @tommcdon or @noahfalk but think the recommendation for folks investing new code is to use EventCounters, so perhaps the minimal fix I suggested will help?

@tommcdon
Copy link
Member

I'd like to hear more from @tommcdon or @noahfalk but think the recommendation for folks investing new code is to use EventCounters, so perhaps the minimal fix I suggested will help?

Adding @sywhang

@sywhang
Copy link
Contributor

sywhang commented Nov 25, 2020

Yes, it is recommended folks writing new counters use the EventCounters API instead of the System.Diagnostics.PerformanceCounter because it makes the counters cross-platform by default and makes it easier to integrate with the diagnostics pipeline we built around EventPipe in .NET Core. The runtime and several other parts of the stack (ASP.NET Core, BCL, EFCore, etc.) are already doing this, so it fits into the rest of the ecosystem better as well.

@JensNordenbro
Copy link
Author

For me, usage of the other api would be clear If System.Diagnostics.PerforamceCounter was part of https://www.nuget.org/packages/Microsoft.Windows.Compatibility

@ericstj
Copy link
Member

ericstj commented Dec 2, 2020

With #45142 it is now possible to use this library on ARM64. It will function correctly for reading non-.NET perfcounters. .NET application provided perfcounters will still not work, but it will not fail: the instances will just not appear due to lack of the perfcounter.dll which exposes them to Windows.

@danmoseley
Copy link
Member

With #45142 it is now possible to use this library on ARM64. It will function correctly for reading non-.NET perfcounters. .NET application provided perfcounters will still not work, but it will not fail: the instances will just not appear due to lack of the perfcounter.dll which exposes them to Windows.

@ericstj what about counters exposed by .NET runtime itself - GC and so forth - will those work - my understanding is yes?

@sywhang
Copy link
Contributor

sywhang commented Dec 19, 2020

The counters exposed by the runtime are all EventCounter implementations and already works on ARM64 with dotnet-counters or EventListeners.

@danmoseley
Copy link
Member

Ah, that makes sense.

@ericstj
Copy link
Member

ericstj commented Jan 5, 2021

I opened #46589 to track the remaining production of performance counters. This issue will track the request of reading counters, and porting that back to 5.0.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 5, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 14, 2021
@ericstj
Copy link
Member

ericstj commented Jan 21, 2021

Fix has been backported to 5.0 and will release in Feb.

@ericstj ericstj closed this as completed Jan 21, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-System.Diagnostics.PerformanceCounter enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

10 participants