Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Port to Release/3.1 - Fix available memory extraction on Linux #26938

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Port to Release/3.1 - Fix available memory extraction on Linux #26938

merged 1 commit into from
Oct 14, 2019

Conversation

agoretsky
Copy link

Port #26764 to 3.1 Release

Close #26924

@agoretsky
Copy link
Author

Got it working on OSX, but there are some build errors on Linux, @janvorli could you help me?

@janvorli
Copy link
Member

@agoretsky the issue is that you have accidentally added the GetLogicalProcessorCacheSizeFromOS function to gcenv.os.cpp. That was not part of my change.

@janvorli @agoretsky
Fix available memory extraction on Linux (#26764)
03f6e04 
* Fix available memory extraction on Linux

The GlobalMemoryStatusEx in PAL is returning number of free physical pages in
the ullAvailPhys member. But there are additional pages that are allocated
as buffers and caches that get released when there is a memory pressure and
thus they are effectively available too.

This change extracts the available memory on Linux from the /proc/meminfo
MemAvailable row, which is reported by the kernel as the most precise
amount of available memory.
@agoretsky agoretsky marked this pull request as ready for review September 30, 2019 13:55
@agoretsky
Copy link
Author

@MeiChin-Tsai

@janvorli
Copy link
Member

Btw, if @MeiChin-Tsai supports porting this fix, I'd still like to give it a bit more time to bake in before merging this port.

@wtgodbe
Copy link
Member

wtgodbe commented Oct 1, 2019

@agoretsky @janvorli does this need to be included with release/3.1-preview1? If so, it needs to be merged by EOD today (as we start the build process tomorrow morning). Otherwise we can take it for preview2.

@janvorli
Copy link
Member

janvorli commented Oct 1, 2019

@wtgodbe thank you for the heads up, I don't want to rush this. We need to do some more GC perf measurements before porting it.

@wtgodbe wtgodbe added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 1, 2019
janvorli
janvorli approved these changes Oct 7, 2019
View reviewed changes
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@janvorli janvorli added this to the 3.1 milestone Oct 7, 2019
@MeiChin-Tsai
Copy link

@agoretsky Can you articulate the scenario that drive the need? Or the risk of fix?

@janvorli
Copy link
Member

janvorli commented Oct 7, 2019

@MeiChin-Tsai here is the original issue where the details were discussed:
https://github.com/dotnet/coreclr/issues/26568
Maybe the best thing to show the benefit is this p99 response time graph the Criteo guys have produced before and after the change:
https://twitter.com/KooKiz/status/1173514597061599232

@MeiChin-Tsai
Copy link

Approved

@richlander
Copy link
Member

I also endorse this change.

@wtgodbe wtgodbe added the Servicing-approved Approved for servicing release label Oct 7, 2019
@wtgodbe
Copy link
Member

wtgodbe commented Oct 10, 2019

This can be merged tomorrow, that's when branches open for preview2

@wtgodbe wtgodbe merged commit ff243be into dotnet:release/3.1 Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@janvorli janvorli janvorli approved these changes

Assignees
No one assigned
Labels
* NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) Servicing-approved Approved for servicing release
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
5 participants
@agoretsky @janvorli @wtgodbe @MeiChin-Tsai @richlander