Fix #91958: use `mach_timebase_info` to determine process time coefficient on macOS by ForNeVeR · Pull Request #92185 · dotnet/runtime

ForNeVeR · 2023-09-16T20:25:45Z

Closes #91958.

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

ghost · 2023-09-16T20:25:55Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	ForNeVeR
Assignees:	-
Labels:	`area-System.Diagnostics.Process`
Milestone:	-

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

adamsitnik

Overall it looks good, but I found few places that could be polished a bit.

Thank you for your contribution @ForNeVeR !

adamsitnik · 2023-09-22T12:02:31Z

src/libraries/Common/src/Interop/OSX/Interop.libSystem.cs

@jkoritzinsky @AaronRobinsonMSFT In terms of marshaller best practices, do we need to explicitly specify sequential layout for such structs?

Suggested change

public struct mach_timebase_info_data_t

[StructLayout(LayoutKind.Sequential)]

public struct mach_timebase_info_data_t

Technically, no. Value types in .NET default to sequential layout. However, and this is annoying, there are some Roslyn warnings that are suppressed if one does explicitly mark the type with sequential layout. The reasoning here is historical, but the gist is if Roslyn complains about unreferenced fields, which can happen for types used in interop, then placing StructLayout(LayoutKind.Sequential) on the type will automatically suppress the warning.

The interop team's general guidance here has been to accept the defaults except where there is annoying friction with C# or where the tooling requires explicit details. This falls into the C# friction bucket, but only if a warning is emitted.

@AaronRobinsonMSFT thank you for a very detailed answer!

I can't see any related warnings in the compilation (and also we seem to have warnings as errors enabled in this part?).

Does this mean this attribute is unnecessary? I am totally okay with adding that if required. Though, yeah, we all know that sequential is the default struct layout 😅

Does this mean this attribute is unnecessary?

Yes.

adamsitnik · 2023-09-22T12:24:57Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs

It would be better to initialize this field lazily, when we need it for the first time. Otherwise this sys-call:

may be called even if we don't need it (during type initializaiton)

in theory it may fail and be very hard to handle properly

Since we use the following logic in 4 places:

return new TimeSpan(Convert.ToInt64($ulong * timeBase.numer / timeBase.denom / NanosecondsTo100NanosecondsFactor));

we could introduce a helper method that could take care of everything, something like this:

Suggested change

private static readonly Interop.libSystem.mach_timebase_info_data_t timeBase = GetTimeBase();

private static volatile uint s_timeBase_numer, s_timeBase_denom;

private static TimeSpan Map(ulong sysTime)

{

uint denom = s_timeBase_denom;

if (denom == default)

{

Interop.libSystem.mach_timebase_info_data_t timeBase = GetTimeBase();

s_timeBase_denom = denom = timeBase.denom;

s_timeBase_numer = timeBase.numer;

}

uint numer = s_timeBase_numer;

return new TimeSpan(Convert.ToInt64(sysTime * numer / denom / NanosecondsTo100NanosecondsFactor));

}

Yeah, agree with your reasoning and applied a bit modified version of this snippet (the only change is in naming, I like MapTime a little bit better).

adamsitnik · 2023-09-22T12:31:55Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs

thank you for finding and updating all use cases! 👍

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

ForNeVeR · 2023-09-24T18:41:53Z

Build on CI is now failing due to something being wrong with SR on Windows. Have I broken that? I need help with resources, I don't think my changes to StringResourcesPath were right.

am11 · 2023-09-24T23:11:13Z

src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj

We have two options:

Include the two keys in test resources:

<data name="CantGetAllPids" xml:space="preserve"> <value>Could not get all running Process IDs.</value> </data> <data name="RUsageFailure" xml:space="preserve"> <value>Failed to set or retrieve rusage information. See the error code for OS-specific error information.</value> </data>

in:

runtime/src/libraries/System.Diagnostics.Process/tests/Resources/Strings.resx

Line 1 in b4a3c9e

<?xml version="1.0" encoding="utf-8"?>

and delete this hard-coded line.

Include test resources alongside source ones -- separated by semicolon ;:

<StringResourcesPath>$(MSBuildProjectDirectory)\Resources\Strings.resx;$(MSBuildProjectDirectory)\..\src\Resources\Strings.resx</StringResourcesPath>

multiple resources seem to be supported.

Option 1 is probably much cleaner that avoids mixing stuff, but I'll defer to others. cc @ViktorHofer

Agreed. Option 1 is cleaner while it adds some duplication.

danmoseley · 2023-09-25T04:15:33Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs

Should it do the last division first, like here?

https://chromium.googlesource.com/chromium/src/+/refs/tags/58.0.3029.141/base/time/time_mac.cc#43

(I don't know what values it typically returns)

Oh, this says it gets the 42 from 125/3. Does 125 * ticks go over ulong max?

https://stackoverflow.com/questions/69318594/how-to-get-the-true-value-of-the-system-clock-in-raw-ticks-i-e-mach-absolute-t

Also I suppose you can store 1000 * denom

I'm sure someone will point out that won't necessarily give the exact same result @tannergooding 🙂

That's a good point, but I was concerned about losing some precision due to doing division first. Though I can't wrap my head around it right now. Will try to do more thorough analysis later today.

Ok, here are my thoughts.

Typical tick values are pretty large actually. For comparison, let's consider a program that was working on a single-core CPU for a year.

> [TimeSpan]::FromDays(365).Ticks 315360000000000 > [long]::MaxValue 9223372036854775807

(here I consider long since TimeSpan takes long in its ctor, not ulong)

This means we will overflow on a numer of 29248 (or if our tick value will get significantly bigger, i.e. we take 29000 years into account, or a CPU with 29k cores).

This is very far from being realistic, but it is also much closer than I expected. So, to be on the safe side, let's do the same as Chromium does: divide by our factor first, to get a hundred times more space, by losing some precision.

We may, of course, also do value / (denom * 100) * numer which is relatively the same, yet will lose a bit more precision as well.

I'm also OK with asserting that numer is reasonably small ( say < 1000) as an alternative, since we suspect that will always be true. Or assert that the math comes out almost the same in 128 bits.

If not we should try to quantify the rounding error and how much it matters.

My current suggestion is to leave this as sysTime / NanosecondsTo100NanosecondsFactor * numer / denom.

I am not sure how useful an assertion would be in this code. Perhaps it'd be better to add checked, since we are concerned by overflow? We do not expect this code to be too performance sensitive, so checked might be good.

What do you think?

Not my area, but I"m not sure an exception would be an improvement. I'd be interested in thoughts of a numeric expert like @tannergooding . I'll step aside for area owners to sign off overall...

I would say that an explicit exception might be an improvement (especially since it would be thrown from a member property such as TotalProcessorTime and not on class init).

But of course, I am ready to listen to other suggestions.

…efficient on macOS

…_timebase_info

ForNeVeR · 2023-09-26T20:31:49Z

I believe that all the existing feedback is addressed, so I'm waiting for further feedback. Thank you so much for your time, folks.

marek-safar · 2023-09-29T20:09:06Z

@dotnet/area-system-diagnostics-process please review

EgorBo · 2023-10-10T17:39:20Z

Ping @dotnet/area-system-diagnostics-process

adamsitnik

LGTM, thank you for your fix @ForNeVeR !

And apologies for the delay (I was on a parental leave).

adamsitnik · 2023-10-16T07:29:54Z

src/libraries/System.Diagnostics.Process/tests/Resources/Strings.resx

  <data name="Argv_IncludeDoubleQuote" xml:space="preserve">
    <value>The argv[0] argument cannot include a double quote.</value>
  </data>
+  <data name="CantGetAllPids" xml:space="preserve">


The fix I've suggested in #92185 (comment) should just work, but I can take care of that in a separate PR to get the fix merged right now.

ForNeVeR · 2023-10-16T18:28:11Z

Thank you so much for help! ❤️

jeffhandley · 2024-03-22T04:40:47Z

/backport to release/8.0-staging

github-actions · 2024-03-22T04:40:58Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8385573189

jeffhandley · 2024-03-22T04:41:52Z

Backporting this fix to 8.0 since this was reported again in #98121.

github-actions · 2024-03-22T04:42:28Z

@jeffhandley backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix #91958: use mach_timebase_info to determine process time coefficient on macOS
Using index info to reconstruct a base tree...
M	src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs
CONFLICT (content): Merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix #91958: use mach_timebase_info to determine process time coefficient on macOS
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-03-22T04:42:28Z

@jeffhandley an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

ghost added area-System.Diagnostics.Process community-contribution Indicates that the PR has been added by a community member labels Sep 16, 2023

ForNeVeR commented Sep 16, 2023

View reviewed changes

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs Outdated Show resolved Hide resolved

ForNeVeR commented Sep 16, 2023

View reviewed changes

src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Outdated Show resolved Hide resolved

ForNeVeR force-pushed the bugfix/91958.macos-cpu-time branch from 7b79541 to 7571e06 Compare September 16, 2023 20:30

This was referenced Sep 16, 2023

Test failure WindowsAlternateDataStreamOverwrite #83659

Open

Tracking issue for "WORKLOAD TIMED OUT" #90309

Closed

adamsitnik requested review from am11 and tmds September 18, 2023 10:38

adamsitnik added the os-mac-os-x macOS aka OSX label Sep 18, 2023

adamsitnik reviewed Sep 22, 2023

View reviewed changes

adamsitnik added this to the 9.0.0 milestone Sep 22, 2023

ForNeVeR requested review from AaronRobinsonMSFT and adamsitnik September 23, 2023 15:43

am11 reviewed Sep 24, 2023

View reviewed changes

danmoseley reviewed Sep 25, 2023

View reviewed changes

ForNeVeR force-pushed the bugfix/91958.macos-cpu-time branch from 861ef04 to d81a801 Compare September 25, 2023 20:21

ForNeVeR added 5 commits September 25, 2023 22:21

Fix dotnet#91958: use mach_timebase_info to determine process time co…

999d6ae

…efficient on macOS

(dotnet#91958) Process.OSX: fallback for non-zero exit code from mach…

4ba25eb

…_timebase_info

(dotnet#91958) Process: move OSX-specific tests to a separate file

8932990

(dotnet#91958) Process.OSX: calculate times lazily

354d181

(dotnet#91958) Process: include string resources properly in tests

d31f96c

ForNeVeR force-pushed the bugfix/91958.macos-cpu-time branch 2 times, most recently from 080428d to 1a57499 Compare September 25, 2023 20:29

ForNeVeR added 2 commits September 25, 2023 22:30

(dotnet#91958) Process: more overflow-safe logic

473d7fc

(dotnet#91958) Minimize diff

ce50c65

ForNeVeR force-pushed the bugfix/91958.macos-cpu-time branch from 1a57499 to ce50c65 Compare September 25, 2023 20:31

build-analysis bot mentioned this pull request Sep 25, 2023

Build crashes in System.Runtime.Serialization.SerializationGuard #92007

Closed

adamsitnik approved these changes Oct 16, 2023

View reviewed changes

adamsitnik merged commit fb74d89 into dotnet:main Oct 16, 2023

adamsitnik mentioned this pull request Oct 16, 2023

reuse existing resource file #93539

Merged

ForNeVeR deleted the bugfix/91958.macos-cpu-time branch October 16, 2023 18:34

ghost locked as resolved and limited conversation to collaborators Nov 15, 2023

github-actions bot unlocked this conversation Mar 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024

	public struct mach_timebase_info_data_t
	[StructLayout(LayoutKind.Sequential)]
	public struct mach_timebase_info_data_t

-        private static readonly Interop.libSystem.mach_timebase_info_data_t timeBase = GetTimeBase();
+        private static volatile uint s_timeBase_numer, s_timeBase_denom;
+        private static TimeSpan Map(ulong sysTime)
+        {
+            uint denom = s_timeBase_denom;
+            if (denom == default)
+            {
+                Interop.libSystem.mach_timebase_info_data_t timeBase = GetTimeBase();
+                s_timeBase_denom = denom = timeBase.denom;
+                s_timeBase_numer = timeBase.numer;
+            }
+            uint numer = s_timeBase_numer;
+            return new TimeSpan(Convert.ToInt64(sysTime * numer / denom / NanosecondsTo100NanosecondsFactor));
+        }

Conversation

ForNeVeR commented Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 16, 2023

Uh oh!

Uh oh!

Uh oh!

adamsitnik left a comment

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!

Uh oh!

ForNeVeR commented Sep 24, 2023

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!

ForNeVeR Sep 25, 2023 • 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!

danmoseley Sep 25, 2023 • 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!

ForNeVeR commented Sep 26, 2023

Uh oh!

marek-safar commented Sep 29, 2023

Uh oh!

EgorBo commented Oct 10, 2023

Uh oh!

adamsitnik left a comment

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!

ForNeVeR commented Oct 16, 2023

Uh oh!

jeffhandley commented Mar 22, 2024

Uh oh!

github-actions bot commented Mar 22, 2024

Uh oh!

jeffhandley commented Mar 22, 2024

Uh oh!

github-actions bot commented Mar 22, 2024

Uh oh!

github-actions bot commented Mar 22, 2024

ForNeVeR commented Sep 16, 2023 •

edited

Loading

ForNeVeR Sep 25, 2023 •

edited

Loading

danmoseley Sep 25, 2023 •

edited

Loading