[2.38][MemoryPressureHandler] Subtract GPU memory from RSS when deciding me…

[asurdej-comcast]

…mory policy

On Mali GPU drivers GPU memory is also included in process RSS mem (RSSFile) that cause pressure handler to kick in too early comparing to other devices. GPU memory doesn't increase container's total mem usage in cgroups so this change doesn't affect total memory allowance but delays pressure handler only.

Hidden under env variable, disabled by default because whole GPU memory (footprintVideo) is custom donwstream solution and is controlled with envs currently.

[magomez]

@asurdej-comcast I'm not sure whether to review this or not, cause it's not a draft, but haven't requested review yet. But on a first glance I can already see that this should be separated into two commits. The one about subtracting the GPU memory value from the RSS, and another one adding the new StrictSynchronous threshold.

Then, about the StrictSynchronous change, why is this needed? Why not do sync releases with the strict policy instead of adding a new policy?

[asurdej-comcast]

@magomez I can't request for review on my own. I don't have proper permission

I created separate StrictSynchronous to keep all thresholds calculation in single place and remove the if condition with manual footprint check if to decide if to call critical realease synchronuosly or not. We could move everything to Strict and set strict threshold percent to "1.0" but maybe it is beneficial to keep another level of memory pressure handling and do it not sync if possible

[magomez]

@magomez I can't request for review on my own. I don't have proper permission

I created separate StrictSynchronous to keep all thresholds calculation in single place and remove the if condition with manual footprint check if to decide if to call critical realease synchronuosly or not. We could move everything to Strict and set strict threshold percent to "1.0" but maybe it is beneficial to keep another level of memory pressure handling and do it not sync if possible

Ok, fair enough, let's go with StrictSynchronous then. But still you need to split this into two commits, as they are two independent changes.

[asurdej-comcast]

@magomez thanks, Splitted into two commits as requested

[magomez]

The change seems ok now. But before merging, I'd like to run this by @modeveci , as this change is going to kill the performance of WPE when it's over the memory limit. In this situation WPE is going to spend more time destroying assets to free memory and then recreating them again than actually rendering or doing anything else. If this is not the expected behavior, the StrictSynchronous change should not be added.

[asurdej-comcast]

in fact, I haven't changed anything here, functional wise, Strict synchronous was there before as a special case under Strict policy:

    case MemoryUsagePolicy::Strict:
        if (footprint > m_configuration.baseThreshold || footprintVideo > m_configuration.baseThresholdVideo) {
            WTFLogAlways("MemoryPressure: Critical memory usage (PID=%d) [MB]: %zu (of %zu), video: %zu (of %zu)\n",
                          getpid(), footprint / MB, m_configuration.baseThreshold / MB,
                          footprintVideo / MB, m_configuration.baseThresholdVideo / MB);
            releaseMemory(Critical::Yes, Synchronous::Yes);
            break;
        }

this is exactly the same behavior, just moved to different place. The same approach is part of 2.28 also, implemented by MemoryUsagePoller in Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp

There real change here is footprint reduction by video memory that is disabled by default anyway

[magomez]

in fact, I haven't changed anything here, functional wise, Strict synchronous was there before as a special case under Strict policy:

Ah, you're right! I missed that!