dotnet-api-docs icon indicating copy to clipboard operation
dotnet-api-docs copied to clipboard

Fixed documentation for MemoryLoadBytes

Open mrsharm opened this issue 1 year ago • 9 comments

Summary

Fixed documentation for the MemoryLoadBytes section as it was misleading before as it's not a percentage but a quantity with units in bytes.

Fixes an issue in the runtime repo: https://github.com/dotnet/runtime/issues/106712

CC: @Maoni0 @cshung @mangod9 @markples

mrsharm avatar Aug 23 '24 18:08 mrsharm

Learn Build status updates of commit b5b09cc:

:white_check_mark: Validation status: passed

File Status Preview URL Details
xml/System/GCMemoryInfo.xml :white_check_mark:Succeeded View

For more details, please refer to the build report.

For any questions, please:

Learn Build status updates of commit 81edcb3:

:white_check_mark: Validation status: passed

File Status Preview URL Details
xml/System/GCMemoryInfo.xml :white_check_mark:Succeeded View

For more details, please refer to the build report.

For any questions, please:

this still reads a bit confusing to me. for example, not sure how the reader would interpret what memory load is a field in the MEMORYSTATUS structure in bytes means and this was the original confusion indicated in https://github.com/dotnet/runtime/issues/94565. I'd suggest we enumerate all cases to make it very clear, something like this -

when a process is not running in a container or running in a container without a memory limit -

  • on windows this is (the memory load field of the MEMORYSTATUS structure (which is a percentage) * total physical memory available on the machine).
  • on Linux this is the MemAvailable field in /proc/meminfo.
  • on FreeBSD...
  • ...

when a process is running in a container with a memory limit -

  • on Windows...
  • ...

also CC-ing @janvorli for review on info about non windows side of things.

Maoni0 avatar Aug 23 '24 19:08 Maoni0

Learn Build status updates of commit d47ead9:

:white_check_mark: Validation status: passed

File Status Preview URL Details
xml/System/GCMemoryInfo.xml :white_check_mark:Succeeded View

For more details, please refer to the build report.

For any questions, please:

Learn Build status updates of commit 1a2f0b3:

:x: Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
:x:Error Details

  • [Error: CannotMergeCommit] Cannot merge commit 1a2f0b3d33c9b94b80a99bf4b37f696cec6ab164 in branch memory_fix of repository https://github.com/mrsharm/dotnet-api-docs into branch main (commit 3b118564873b158594f05f81458f7f587ee9b1c5). Please follow this documentation: https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/ to use git.exe to resolve you content conflicts locally and then push to remote.

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

Learn Build status updates of commit 1579d4c:

:white_check_mark: Validation status: passed

File Status Preview URL Details
xml/System/GCMemoryInfo.xml :white_check_mark:Succeeded View

For more details, please refer to the build report.

For any questions, please:

@janvorli, any feedback here? Explicitly highlighted the source of the memory load calculation, which prior to this change was ambiguously stated. Based on the code of GCToOSInterface::GetMemoryStatus, there were two cases: one with a memory limit set and one without whether in a container or not -- these were basically added to the documentation.

mrsharm avatar Sep 12 '24 21:09 mrsharm

Learn Build status updates of commit 832b8f5:

:white_check_mark: Validation status: passed

File Status Preview URL Details
xml/System/GCMemoryInfo.xml :white_check_mark:Succeeded View

For more details, please refer to the build report.

For any questions, please:

@gewarren, can we please merge this PR? All comments have been resolved and all the checks have passed.

mrsharm avatar Jul 25 '25 15:07 mrsharm