[WIP] Ported HighPerformanceCounter internal API from CoreRt #27175

VSadov · 2019-10-14T05:49:02Z

There is no access to a high precision timer in corlib, which is very limiting for time-sensitive features in managed code.
CoreRt HighPerformanceCounter is lean, yet seems sufficient for most needs.

jkotas · 2019-10-14T05:56:01Z

tests/src/CoreMangLib/system/highperformancecounter/highperformancecounter.cs

@@ -0,0 +1,56 @@
+// Licensed to the .NET Foundation under one or more agreements.


This is very weak test (e.g. compared to how we test stop watch). I do not think you need to bother with a test like this.

The API basically exposes QueryPerformanceCounter and QueryPerformanceFequency.
I am just testing that they are connected. - not crashing with missing methods, etc...

There is not a lot of functionality. I just could not do it without minimal testing to see that API works at all.

The longer term value of such testing is super low. You will get the same level of testing by any use of this.

I figured - if I didn't have tests, I'd be asked "why not a single test was added? how do we know it works?". I myself would feel uncomfortable.
Removing a test is easy though.

jkotas · 2019-10-14T05:58:30Z

src/System.Private.CoreLib/src/System/HighPerformanceCounter.cs

+    internal static class HighPerformanceCounter
+    {
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        internal static extern ulong QueryPerformanceFrequency();


It would be nice to wait for @AaronRobinsonMSFT #26458 to go through and the implement it using that.

It looks like #26458 is about PInvoking to external libraries. I need to call corlib since it shims these methods for multiple platforms (Linux, OSX).

I am wondering if #26458 will help this case.

It would use corefx shims, like CoreRT and Stopwatch does today.

Coreclr needs these APIs and will have to shim for any new platform added. Why not just use that?

The code written against CoreFX shims is shareable between CoreCLR, Mono and Corert. We are trying to maximize amount of such shared code,

Also #26458 seems to be inserting GCPoll in every call. Is that a good idea for a high performance counter?

Yes, I think we need to start working towards more predictable GC suspension schemes to support GC SLAs. Many FCalls are missing the GC poll today - it is a bug.

Just to be clear - I am not arguing against #26458 in general. I like it and would use it in many other places.
But for high performance counter having as little overhead as possible is important. FCall is the best option that we have for low overhead. I think it is more justified here than in many other places where FCall is used.

FCalls without a GC poll are also a reliable way to stall the GC. It is not hard to come up with FCall-heavy microbenchmarks that have GC suspension times 0.5 seconds and more.

The two instructions one pays for the GC poll is absolutely worth paying to mitigate this problem. I expect that we may enable the GC poll across the board for all FCalls once we start being serious about GC SLAs.

Note you have to pay the cost of the API wrapper with FCall. #26458 lets you call the OS API directly (on Windows at least), and so it is going to faster than what you got here, even with GC poll.

stephentoub · 2019-10-14T11:12:25Z

src/System.Private.CoreLib/src/System/HighPerformanceCounter.cs

+            {
+                return QueryPerformanceCounter();
+            }
+        }


What is the benefit of this as compared to Stopwatch.GetTickCount(), just that it's not in corelib?

This represents the bulk of the Stopwatch implementation. If we really need it in corelib, then shouldn't we just move Stopwatch down rather than duplicating it?

And if we must duplicate it, can't we use the code (modified as necessary) in the shared partition?

I also don't like us adding code like this that's not actually used.

Stopwatch is not in corlib, yes. It was recommended to look at CoreRt for how it deals with that. CoreRt has this. I am not sure why not Stopwatch. I assumed there are reasons for CoreRt having this API instead and also apply here. Like layering for additional features/diagnostics used there, or maybe just desire/rules to keep extension APIs out of corlib when possible.
Surely the same questions came up for CoreRt and were resolved once already.

I would be using this in an otherwise unrelated work. I did not want to bundle it all together. Also to manage risk of finding that the change is undesirable before investing more in this direction.

I kind of expected that this would be an uncontroversial simple change though, since I heard that not having HP timers in corlib was a problem in the past. And again - by looking at prior art in CoreRt.

was recommended to look at CoreRt for how it deals with that.

I have mentioned both CoreRT and Stopwatch as examples for how to deal with that. Both use regular PInvokes with signatures defined in shared partition. Neither of these two have duplicated FCalls.

I assumed there are reasons for CoreRt having this API.

We used to have a lot of duplication between CoreLib and CoreFX due to historical artificial layering. We do not have this artificial layering anymore and we prefer to avoid duplication.

jkotas · 2019-10-14T18:31:20Z

Moving the whole Stopwatch to CoreLib (without any modifications) is probably the best way to achieve what you are looking for.

VSadov · 2019-10-14T18:56:28Z

Moving the whole Stopwatch to CoreLib (without any modifications)

That is a bigger more disruptive change. Before embarking on that we'd better be in agreement that we want such change.

@stephentoub - do you see problems with that?
I.E: public API layering, compat, ref assemblies, reflection use, diagnostics, documenting, localization, build breaks when stuff is in both places etc.

Such move was probably done before. What are the typical traps to look for?

Do we want to move the tests too?

stephentoub · 2019-10-14T19:30:49Z

do you see problems with that?

I'm fine with that... actually called it out earlier: "This represents the bulk of the Stopwatch implementation. If we really need it in corelib, then shouldn't we just move Stopwatch down rather than duplicating it?" 😉

In the past to avoid issues we've staged it something like this:

PR in corefx to move Stopwatch to src\Common\src\Corelib.
Wait for mirror bot to open PR to bring changes to coreclr.
Update Corelib to include Stopwatch.
Wait for bot to open PR in corefx bringing updated build of corelib with Stopwatch
Add a commit to that PR which removes Stopwatch.*.cs from System.Runtime.Extensions build.

Ref and tests should remain in corefx.

VSadov · 2019-10-14T19:42:40Z

Thanks @stephentoub !!

Ref and tests should remain in corefx.

I was particularly unsure what happens to these. Sounds good.

VSadov · 2019-10-14T20:02:26Z

So I will close this PR then.

The Stopwatch move will need to be initiated from the CoreFx side.

Ported HighPerformanceCounter internal API from CoreRt

c1102cc

VSadov requested a review from jkotas October 14, 2019 05:49

jkotas reviewed Oct 14, 2019

View reviewed changes

Copy-paste in the test.

4a6cd1f

stephentoub reviewed Oct 14, 2019

View reviewed changes

VSadov changed the title ~~Ported HighPerformanceCounter internal API from CoreRt~~ [WIP] Ported HighPerformanceCounter internal API from CoreRt Oct 14, 2019

VSadov closed this Oct 15, 2019

VSadov mentioned this pull request Oct 23, 2019

Moving StopWatch to src\Common\src\Corelib dotnet/corefx#42072

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Ported HighPerformanceCounter internal API from CoreRt #27175

[WIP] Ported HighPerformanceCounter internal API from CoreRt #27175

VSadov commented Oct 14, 2019 •

edited

Loading

jkotas Oct 14, 2019 •

edited

Loading

VSadov Oct 14, 2019

VSadov Oct 14, 2019

jkotas Oct 14, 2019 •

edited

Loading

VSadov Oct 14, 2019

jkotas Oct 14, 2019

VSadov Oct 14, 2019

jkotas Oct 14, 2019

VSadov Oct 14, 2019

jkotas Oct 14, 2019

VSadov Oct 14, 2019

jkotas Oct 14, 2019

VSadov Oct 14, 2019 •

edited

Loading

jkotas Oct 14, 2019 •

edited

Loading

stephentoub Oct 14, 2019 •

edited

Loading

VSadov Oct 14, 2019

jkotas Oct 14, 2019

jkotas commented Oct 14, 2019 •

edited

Loading

VSadov commented Oct 14, 2019

stephentoub commented Oct 14, 2019

VSadov commented Oct 14, 2019 •

edited

Loading

VSadov commented Oct 14, 2019

		@@ -0,0 +1,56 @@
		// Licensed to the .NET Foundation under one or more agreements.

[WIP] Ported HighPerformanceCounter internal API from CoreRt #27175

[WIP] Ported HighPerformanceCounter internal API from CoreRt #27175

Conversation

VSadov commented Oct 14, 2019 • edited Loading

jkotas Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

jkotas Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

stephentoub Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Oct 14, 2019 • edited Loading

VSadov commented Oct 14, 2019

stephentoub commented Oct 14, 2019

VSadov commented Oct 14, 2019 • edited Loading

VSadov commented Oct 14, 2019

VSadov commented Oct 14, 2019 •

edited

Loading

jkotas Oct 14, 2019 •

edited

Loading

jkotas Oct 14, 2019 •

edited

Loading

VSadov Oct 14, 2019 •

edited

Loading

jkotas Oct 14, 2019 •

edited

Loading

stephentoub Oct 14, 2019 •

edited

Loading

jkotas commented Oct 14, 2019 •

edited

Loading

VSadov commented Oct 14, 2019 •

edited

Loading