Add System.Diagnostics.PerformanceData namespace by MarcoRossignoli · Pull Request #31474 · dotnet/corefx

MarcoRossignoli · 2018-07-30T14:05:44Z

closes https://github.com/dotnet/corefx/issues/30188

danmoseley · 2018-07-30T17:13:23Z

+            {
+                if (s_counterSetList.ContainsKey(counterSetGuid))
+                {
+                    throw new ArgumentException(SR.Format(SR.Perflib_Argument_CounterSetAlreadyRegister, counterSetGuid), "CounterSetGuid");


Please change argument name strings to nameof eg CounterSetGuid to nameof(CounterSetGuid)

sorry missed that...i'll do another pass

fixed i did a pass and the other should not be method arguments.

danmoseley · 2018-07-30T17:27:23Z

@brianrob could someone on your team please take a look (especially at the tests)?

danmoseley · 2018-07-30T17:28:36Z

In the NETFX build:

D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Diagnostics.PerformanceData.CounterData' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Diagnostics.PerformanceData.CounterSet' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Diagnostics.PerformanceData.CounterSetInstance' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Diagnostics.PerformanceData.CounterSetInstanceCounterDataSet' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Diagnostics.PerformanceData.CounterSetInstanceType' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Did not find type 'T:System.Diagnostics.PerformanceData.CounterType' in any of the seed assemblies. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): warning : Errors were encountered while generating the facade. [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]
D:\j\workspace\windows-TGrou---2a8f9c29\Tools\partialfacades.task.targets(64,5): error : Errors were encountered when generating facade(s). [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj]

@safern any idea what this could be? The types are in System.Core in NETFX.

safern · 2018-07-30T17:34:03Z

@safern any idea what this could be? The types are in System.Core in NETFX.

The csproj has this references for NETFX

<ItemGroup Condition="'$(TargetsNetFx)' == 'true'">
    <Reference Include="mscorlib" />
     <Reference Include="System" />
</ItemGroup>

We need to add System.Core reference to both, ref and src csproj.

safern · 2018-07-30T17:35:03Z

    <ProjectGuid>{5BDC8641-E3EE-47B5-BE7B-2D2275402412}</ProjectGuid>
    <Configurations>net461-Debug;net461-Release;netfx-Debug;netfx-Release;netstandard-Debug;netstandard-Release</Configurations>
  </PropertyGroup>
+  <PropertyGroup>


Instead of adding a new PropertyGroup just add this property in the group above.

danmoseley · 2018-07-30T18:48:19Z

Ugh I missed that thanks @safern.

brianrob · 2018-07-30T18:53:38Z

Adding @adiaaida, who owns performance counters.

Anipik · 2018-07-30T18:55:33Z

+{
+    internal static class UnsafeNativeMethods
+    {
+        internal const string ADVAPI32 = "advapi32.dll";


Delete this and use Libraries.advapi

Anipik · 2018-07-30T18:57:32Z

+
+        [DllImport(ADVAPI32, ExactSpelling = true, EntryPoint = "PerfStopProvider", CharSet = CharSet.Unicode)]
+        internal static extern unsafe uint PerfStopProvider(
+            [In] IntPtr hProvider


can remove [In], [Out] from all the interop definitions https://github.com/Anipik/corefx/blob/master/Documentation/coding-guidelines/interop-pinvokes.md

Anipik · 2018-07-30T18:58:39Z

+        internal struct PerfCounterSetInfoStruct
+        {
+            // PERF_COUNTERSET_INFO structure defined in perflib.h
+            [FieldOffset(0)] internal Guid CounterSetGuid;


I am not sure if [FieldOffset] attributes are required here.

cc @JeremyKuhne

You should remove the explicit and offset attributes here. You can then assert in a test that sizeof(PerfCountSetInfoStruct) is 40 if you like.

Or Debug.Assert somewhere in the code.

added Asserts before usage

Anipik · 2018-07-30T19:02:51Z

+
+namespace Microsoft.Win32
+{
+    internal static class UnsafeNativeMethods


Follow interop naming conventions https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-guidelines.md

Anipik · 2018-07-30T19:04:28Z

+    public class CounterSet : IDisposable
+    {
+        private static readonly bool s_platformNotSupported = (Environment.OSVersion.Version.Major < 6);
+        internal PerfProvider m_provider;


remove m_ from all the names

Anipik · 2018-07-30T19:05:03Z

+        /// <param name="providerGuid">ProviderGuid identifies the provider application. A provider identified by ProviderGuid could publish several CounterSets defined by different CounterSetGuids</param>
+        /// <param name="counterSetGuid">CounterSetGuid identifies the specific CounterSet. CounterSetGuid should be unique.</param>
+        /// <param name="instanceType">One of defined CounterSetInstanceType values</param>        
+        [SuppressMessage("Microsoft.Naming", "CA1720:IdentifiersShouldNotContainTypeNames", MessageId = "guid", Justification = "Approved")]


@danmosemsft can we remove this and [System.Security.SecuritySafeCritical]?

[System.Security.SecuritySafeCritical] yes..i'm missed that...i don't know about [SuppressMessage]

removed [System.Security.SecuritySafeCritical]

You can remove all SuppressMessage as well I think. They refer to old fxcop rules that we do not run in corefx.

Anipik · 2018-07-30T19:05:28Z

+            // Check only the mayor version, only support Windows Vista and later.
+            if (s_platformNotSupported)
+            {
+                throw new System.PlatformNotSupportedException(SR.Perflib_PlatformNotSupported);


remove System.

i removed check

Anipik · 2018-07-30T19:09:46Z

+            lock (this)
+            {
+                PerfProviderCollection.UnregisterCounterSet(m_counterSet);
+                if (m_instanceCreated)


cheange it to m_instanceCreated && m_provider != null

Anipik · 2018-07-30T19:16:21Z

+namespace System.Diagnostics.PerformanceData
+{
+    /// <summary>
+    /// CounterSetInstance class maps to "Instace" in native performance counter implementation.


typo instance

Anipik · 2018-07-30T19:16:54Z

+    /// </summary>
+    public sealed class CounterSetInstance : IDisposable
+    {
+        internal CounterSet m_counterSet;


Similar thing about removing the m_

Anipik · 2018-07-30T19:17:43Z

+            }
+            unsafe
+            {
+                if (m_nativeInst != null)


combine all the ifs with and operation

Anipik · 2018-07-30T19:18:03Z

+        /// Access CounterSetInstanceCounterDataSet property. Developers can then use defined indexer to access
+        /// specific CounterData object to query/update raw counter data.
+        /// </summary>
+        public CounterSetInstanceCounterDataSet Counters


use autoimplemented properties

MarcoRossignoli · 2018-08-28T07:43:37Z

@dotnet-bot test this please

MarcoRossignoli · 2018-08-29T11:19:50Z

dotnet test Linux x64 Release Build please
dotnet test NETFX x86 Release Build please
dotnet test UWP CoreCLR x64 Debug Build please

MarcoRossignoli · 2018-08-30T09:59:55Z

dotnet test Linux x64 Release Build please

MarcoRossignoli · 2018-08-30T16:06:48Z

@danmosemsft @brianrob do you think we need other re-runs?

danmoseley · 2018-08-30T19:10:59Z

If we want to prove we have eliminated flakiness we have to run several times
@dotnet-bot test this please

MarcoRossignoli · 2018-08-31T06:19:27Z

@dotnet-bot test this please
success.
unrelated fail
Packaging All Configurations x64 Debug Build
00:09:30 System\Runtime\InteropServices\Marshal\ChangeWrapperHandleStrengthTests.cs(61,37): error CS1061: 'TypeBuilder' does not contain a definition for 'CreateType' and no extension method 'CreateType' accepting a first argument of type 'TypeBuilder' could be found (are you missing a using directive or an assembly reference?) [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Runtime.InteropServices\tests\System.Runtime.InteropServices.Tests.csproj]

MarcoRossignoli · 2018-08-31T10:58:19Z

@dotnet-bot test this please
success.
unrelated fails
OSX x64 Debug Build
Packaging All Configurations x64 Debug Build

MarcoRossignoli · 2018-08-31T12:38:29Z

@dotnet-bot test this please
success
unrelated fail Packaging All Configurations x64 Debug Build

MarcoRossignoli · 2018-08-31T15:14:22Z

@dotnet-bot test this please
success
unrelated fail
Linux x64 Release Build
Packaging All Configurations x64 Debug Build

MarcoRossignoli · 2018-08-31T17:21:09Z

@dotnet-bot test this please
success
unrelated fail
Packaging All Configurations x64 Debug Build

MarcoRossignoli · 2018-08-31T18:30:26Z

@dotnet-bot test this please
success
unrelated fail
Packaging All Configurations x64 Debug Build

MarcoRossignoli · 2018-09-01T07:44:25Z

@dotnet-bot test this please
success
unrelated fail
Packaging All Configurations x64 Debug Build
UWP CoreCLR x64 Debug Build

MarcoRossignoli · 2018-09-02T13:55:34Z

@dotnet-bot test this please
success
unrelated fail
OSX x64 Debug Build

MarcoRossignoli · 2018-09-03T06:57:59Z

@dotnet-bot test this please

MarcoRossignoli · 2018-09-03T17:35:29Z

@danmosemsft run several times(10) and seems ok...one question, is there a way to check all PR CI run history?

danmoseley · 2018-09-03T20:38:32Z

@brianrob or @adiaaida could you please re review?

brianrob · 2018-09-04T18:38:26Z

@adiaaida, PTAL.

danmoseley · 2018-09-04T22:36:26Z

Thanks @MarcoRossignoli !

BTW here's another smaller port if you're interested: https://github.com/dotnet/corefx/issues/32103

danmoseley · 2018-09-04T22:36:36Z

Thanks @adiaaida also

MarcoRossignoli · 2018-09-05T07:22:13Z

thank's to all for help!

* move System.Diagnostics.PerformanceData from netfx * updates * use type aliases, fix style, cleanup * remove CAS attributes * nit: extralines * Add tests * update tests * update tests * update tests,use type aliases, cleanup * address PR feedback * address some of PR feedback * address some of PR feedback * address some of PR feedback * address PR feedback * added retry/dispose * update tests * try to run new test in isolated process * revert changes to other tests, add retry on new test * fix netfx test fail, add comments Commit migrated from dotnet/corefx@610e9c8

MarcoRossignoli added 9 commits July 25, 2018 17:51

move System.Diagnostics.PerformanceData from netfx

2e47d17

updates

7bb7399

use type aliases, fix style, cleanup

b86ad93

remove CAS attributes

5db9f6c

nit: extralines

8184ca9

Add tests

d47e720

update tests

d4a6028

update tests

e82c451

update tests,use type aliases, cleanup

9fb9e01

danmoseley reviewed Jul 30, 2018

View reviewed changes

safern reviewed Jul 30, 2018

View reviewed changes

address PR feedback

99fe8b0

danmoseley requested a review from brianrob July 30, 2018 18:48

Anipik reviewed Jul 30, 2018

View reviewed changes

karelz added area-System.Diagnostics.Tracing and removed area-System.Diagnostics labels Aug 23, 2018

karelz assigned brianrob, vancem and valenis Aug 23, 2018

michellemcdaniel approved these changes Sep 4, 2018

View reviewed changes

danmoseley merged commit 610e9c8 into dotnet:master Sep 4, 2018

MarcoRossignoli deleted the performancedata branch September 5, 2018 07:20

karelz added this to the 3.0 milestone Sep 6, 2018

Conversation

MarcoRossignoli commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Jul 30, 2018

Uh oh!

danmoseley commented Jul 30, 2018

Uh oh!

safern commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Jul 30, 2018

Uh oh!

brianrob commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern commented Jul 30, 2018 •

edited

Loading

Anipik Jul 30, 2018 •

edited

Loading

Anipik Jul 30, 2018 •

edited

Loading

MarcoRossignoli Jul 31, 2018 •

edited

Loading

MarcoRossignoli commented Aug 31, 2018 •

edited

Loading

MarcoRossignoli commented Aug 31, 2018 •

edited

Loading

MarcoRossignoli commented Aug 31, 2018 •

edited

Loading

MarcoRossignoli commented Aug 31, 2018 •

edited

Loading

MarcoRossignoli commented Aug 31, 2018 •

edited

Loading

MarcoRossignoli commented Aug 31, 2018 •

edited

Loading

MarcoRossignoli commented Sep 1, 2018 •

edited

Loading

MarcoRossignoli commented Sep 2, 2018 •

edited

Loading