Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.38] Taking some IOS GC branches for WPE as well #1422

Open
wants to merge 3 commits into
base: wpe-2.38
Choose a base branch
from

Conversation

tomasz-karczewski-red
Copy link

The aim is to make the GC more reliable on low-memory platforms. IOS branches applied:

  • FullGCActivityCallback::doCollection - do not bail out of GC early
  • MemoryPressureHandler - apply IOS defaults for conservative/strict mode thresholds
  • MemoryRelease / releaseCriticalMemory - request immediate GC
  • WorkerGlobalScope::deleteJSCodeAndGC - collect even if called with Synchronous::No

Copy link

@magomez magomez left a comment

Choose a reason for hiding this comment

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

Please, we have here 4 changes that should go in 4 separate commits. Well, 3, as the change in the MemoryPressureHandler fractions should not be there. Please, remove the fractions change and split the PR in 3 different commits.

@@ -40,7 +40,7 @@ namespace WTF {

WTF_EXPORT_PRIVATE bool MemoryPressureHandler::ReliefLogger::s_loggingEnabled = false;

#if PLATFORM(IOS_FAMILY)
#if PLATFORM(IOS_FAMILY) || PLATFORM(WPE)
Copy link

Choose a reason for hiding this comment

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

We cannot change the default values of the fractions, cause this affects all of the devices. As I told you in previous PRs, there's API to change these values for your platform only, please use it.

The aim is to make the GC more reliable on low-memory platforms.
In FullGCActivityCallback, the change is
to do not bail out of GC early.
The aim is to make the GC more reliable on low-memory platforms.
In MemoryRelease, the change is to request immediate GC in
releaseCriticalMemory.
The aim is to make the GC more reliable on low-memory platforms.
In WorkerGlobalScope::deleteJSCodeAndGC the change is to
collect even if called with 'Synchronous::No'
@tomasz-karczewski-red tomasz-karczewski-red force-pushed the RDK-588_GC_finetuning_use_IOS_branches branch from 7cd0bde to 8c1c5e5 Compare October 22, 2024 09:13
@tomasz-karczewski-red
Copy link
Author

sure - I've just updated the PR

@magomez
Copy link

magomez commented Oct 22, 2024

@modeveci , this is technically fine for merge, but it's adding changes to how the memory release is done and may have an impact in performance, so it should be verified on your side first.

@woutermeek
Copy link

Thanks @magomez, we'll investigate if there are any performance issues.

@woutermeek woutermeek requested a review from modeveci October 30, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants