restrict memory limit by taking VM into consideration#17177
Conversation
| { | ||
| if (hinstKernel32 != 0) | ||
| { | ||
| // We can also free the lib here - if we are limited by VM we will not be calling |
There was a problem hiding this comment.
Do we really need to load K32GetProcessMemoryInfo dynamically? According to MSDN, it is always available in kernel32.dll starting with Windows 7. The dynamic loading looks like leftover from times when we have supported earlier OSes.
And CoreCLR.dll always depends on kernel32.dll, so we are not getting anything by loading it dynamically.
There was a problem hiding this comment.
I just looked; this code was changed a lot by various people and they gradually got rid of the dynamic checks...but somehow the last checkin left this one...which I didn't CR 'cause I was out (but you did :-))...
There was a problem hiding this comment.
Similar cleanups tend to be done gradually. People do an iterations and then move on, even though there is more that can be cleaned up.
I have just pointed this out since you are adding more code here that does not seem necessary.
There was a problem hiding this comment.
it's only unnecessary if the dynamic check isn't there. do you remember if there was a reason why this particular dynamic check was left there? it would seem it's easier to just remove them all.
There was a problem hiding this comment.
#8624 was about removing API sets dependencies.
The dynamic check for K32GetProcessMemoryInfo was there even on the non-API set (ie !FEATURE_CORECLR) codepath before the change, and I guess that's why it was kept even with API set dependencies removed.
There was a problem hiding this comment.
ok...I'm going to open an issue to track this - I also see that there're unnecessary things like "GCIsProcessInJob = &(::IsProcessInJob);", why not just call IsProcessJob if we are sure it's there? also the standalone gc's implementation still has all the checks which seem unnecessary.
|
Also, this change should be replicated in https://github.com/dotnet/coreclr/blob/master/src/gc/windows/gcenv.windows.cpp for standalone GC. |
|
ah yeah, I'll add that. thanks! |
|
@Maoni0 the GetProcessMemoryLoad does clipping of ullAvailPhys by ullAvailVirtual. I understand that this change adds clipping by the job limits, so it seems it would be nice to change the title/ description of the PR to make it clear. |
@janvorli not sure what you meant? this PR adds clipping even when you are not using a job...it's just to restrict the memory by another means, ie, via VM. |
|
@Maoni0 we were already clipping the physical memory via VM before, even for PAL. See the GetProcessMemoryLoad implementation here: https://github.com/dotnet/coreclr/blob/master/src/vm/corhost.cpp#L2957-L2962 |
|
@janvorli wow that's quite a dangerous thing to do - so we are returning the available VM as available physical mem, yet we do not modify the memory load to reflect this... |
|
@Maoni0 it has been like that for a long time if not forever - the code has not changed since the initial publishing of opensource coreclr. |
|
@janvorli I'm going to get rid of it and use the logic in my PR. do you see a problem with that for non Windows platform? |
|
FWIW, this code was added on 2003/05/10 by weiwenli. There are no further details on why it was added. I agree that it looks wrong. I do not see a problem with getting rid of it. |
|
#17243 |
|
LGTY? 😸 |
5 participants
for 32-bit processes when we look at the memory limit for the process we also should take VM into consideration. right now the VM limit is not implemented on PAL so this is only for non PAL.