32 bit memory getters on Process should not wrap

[danmoseley]

These property getters on the Process type were obsoleted (somewhere between .NET Framework 2.0 and .NET Framework 4.0 inclusive) because their return type is int, which is frequently too small.

NonpagedSystemMemorySize
PagedMemorySize
PagedSystemMemorySize
PeakPagedMemorySize
PeakVirtualMemorySize
PeakWorkingSet
PrivateMemorySize
VirtualMemorySize
WorkingSet

They are implemented to do an unchecked cast of the actual long value to int. That is frequently meaningless, eg., this from a dotnet process a Ubuntu machine our tests run on:

###	PeakVirtualMemorySize: -866021376
###	PeakVirtualMemorySize64: 3428945920
###	PeakWorkingSet: 51585024
###	PeakWorkingSet64: 51585024
###	PrivateMemorySize: 106975232
###	PrivateMemorySize64: 106975232
###	VirtualMemorySize: -866070528
###	VirtualMemorySize64: 3428896768
###	WorkingSet: 51585024
###	WorkingSet64: 51585024

The negative values have no conceivable use and at first I assumed we have a bug in how we read /proc/PID/stat.

These properties exist so that code written a long time ago can either log the value or make a decision. I wonder if it might be better to instead return Int32.MaxValue if it exceeds the range of an int. That is suggestive that the range has been exceeded, and any code making a decision based on the value may be less likely to misbehave.

[ghost]

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

Issue Details

---

These property getters on the Process type were obsoleted (somewhere between .NET Framework 2.0 and .NET Framework 4.0 inclusive) because their return type is int, which is frequently too small.

NonpagedSystemMemorySize
PagedMemorySize
PagedSystemMemorySize
PeakPagedMemorySize
PeakVirtualMemorySize
PeakWorkingSet
PrivateMemorySize
VirtualMemorySize
WorkingSet

They are implemented to do an unchecked cast of the actual long value to int. That is frequently meaningless, eg., this from a dotnet process a Ubuntu machine our tests run on:

###	PeakVirtualMemorySize: -866021376
###	PeakVirtualMemorySize64: 3428945920
###	PeakWorkingSet: 51585024
###	PeakWorkingSet64: 51585024
###	PrivateMemorySize: 106975232
###	PrivateMemorySize64: 106975232
###	VirtualMemorySize: -866070528
###	VirtualMemorySize64: 3428896768
###	WorkingSet: 51585024
###	WorkingSet64: 51585024

The negative values have no conceivable use and at first I assumed we have a bug in how we read /proc/PID/stat.

These properties exist so that code written a long time ago can either log the value or make a decision. I wonder if it might be better to instead return Int32.MaxValue if it exceeds the range of an int. That is suggestive that the range has been exceeded, and any code making a decision based on the value may be less likely to misbehave.

Author:	danmosemsft
Assignees:	-
Labels:	area-System.Diagnostics.Process, untriaged
Milestone:	-

[danmoseley]

cc @marklio in case he has an opinion. My guess is there is zero existing code that will handle a negative number here, as if they were aware of that possibility, they would have changed to use the newer properties.

Using my imagination, I could imagine this scenario: someone is using very old code for logging, and it logs the negative number. Nobody wants to change that code. The actual value for some reason is never larger than 2^16, so they reverse engineer the actual value from the negative number. That seems a bit absurd though.

[eiriktsarpalis]

I think that would be a reasonable improvement cc @adamsitnik

[marklio]

I notice that the .NET 5 docs don't list the obsoletion (while 4.8 docs do). We should make sure that gets fixed, or we'll accrue more risk of problems here over time. It looks like there are only a handful of callers of the non-64 versions of these properties in NuGet. Interestingly, there are a few hits in core-targeting assemblies. However, the numbers are vanishingly small compared to most compat investigations, particularly for the properties likely to exceed 32-bits.

My opinion is that with the presence of the 64-bit replacements, it isn't worth changing. If the bulk of risk comes from existing .NET Framework libraries, then these are libraries already exposed to this problem. Is code more likely to be exposed to such a condition on Core due to accounting differences in Linux or some other external scenario?

If we're worried about new code being written, we should ensure that obsoletion is working properly in the dev experience, or hide them from intellisense, or even remove them from the ref assemblies.

[danmoseley]

@marklio in my mind the reason to change it is that it no longer appears to be broken (as even I, who work on it, at first assumed). With respect to usage, it is of course fine to use them if you're measuring a process that never exceeds 2GB in whatever measure.

My opinion is that with the presence of the 64-bit replacements, it isn't worth changing

In .NET Core, I think the default should always be to fix/improve things, however minor, unless there are good compat reasons not to. (As opposed to in .NET Framework, where compat is the overriding consideration.) It that a reasonable way of thinking about it do you think?

[adamsitnik]

were obsoleted (somewhere between .NET Framework 2.0 and .NET Framework 4.0 inclusive)

If they were obsoleted for more than a few years I believe that we should not be doing any investment in them. Users had enough time to stop using them.

Using my imagination, I could imagine this scenario: someone is using very old code for logging,

Such users would most likely not update to .NET Core, not to speak about updating to .NET 6.0 when it gets released.

[danmoseley]

Do we warn about them at build time? I know we improved our obsoletion story recently, but the test library (S.R.IS.RI) that calls them builds without warnings.

[adamsitnik]

Do we warn about them at build time?

They are all annotated with [Obsolete] attribute:

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

Lines 319 to 320 in ef2a187

	[ObsoleteAttribute("This property has been deprecated. Please use System.Diagnostics.Process.NonpagedSystemMemorySize64 instead. https://go.microsoft.com/fwlink/?linkid=14202")]
	public int NonpagedSystemMemorySize

the test library (S.R.IS.RI) that calls them builds without warnings

I've checked that and the obsoletion warnings are disabled on purpose in the tests source code:

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

Lines 1768 to 1770 in 5808716

	#pragma warning disable 0618
	AssertNonZeroWindowsZeroUnix(_process.NonpagedSystemMemorySize);
	#pragma warning restore 0618

[danmoseley]

That's my bad for commenting on my phone without looking at the code. I meant the S.R.I.RI test that we use to log the Helix configurations. It seems @stephentoub added these, I'm not sure why:
https://github.com/dotnet/corefx/pull/35645/files#diff-eaa334ef3a9273c71a311add7a7c55f42c7f480925201b102bef79acfff49a0fR65

I will take those 32 bit ones out of there and remove the pragmas.

With respect to what to do about the properties themselves, it's your call -- if you want to make them MaxValue, I'm happy to make the PR, but I don't have a strong opinion either way.

[adamsitnik]

That's my bad for commenting on my phone without looking at the code

It's always good to ask questions and this one led to an interesting discussion!

With respect to what to do about the properties themselves, it's your call -- if you want to make them MaxValue, I'm happy to make the PR, but I don't have a strong opinion either way.

I hardly ever have a strong opinion besides the performance aspects, but here I would really prefer to avoid making any improvements in code that have been marked as [Obsolete]. It could give our users a false hope that it's still fine to use these properties. It's not recommended to use them and they should not be used.

[danmoseley]

Sounds good, feel free to close. I will fix up that test later.

[danmoseley]

I do wonder whether instead of
[ObsoleteAttribute("This property has been deprecated. Please use System.Diagnostics.Process.WorkingSet64 instead. ...
which sounds like a "eh, but ideally you wouldn't use me" it should be something more blunt like
[ObsoleteAttribute("This property may return incorrect results. Please use System.Diagnostics.Process.WorkingSet64 instead...

[adamsitnik]

This property may return incorrect results

This is definitely a good idea!