Skip to content

Conversation

@bnham
Copy link
Contributor

@bnham bnham commented May 13, 2022

f822d46

Enforce foreground WebContent memory limit on macOS
https://bugs.webkit.org/show_bug.cgi?id=240397

Reviewed by Chris Dumez.

We removed the foreground memory limit for WebContent on macOS in r272046. But based on some
bug reports that we've seen, it seems like we need to restore some limit to prevent bad user
outcomes when a misbehaving process has runaway memory usage.

This patch adds a foreground memory limit of 8GB or 16GB depending on RAM size. This matches
the limits set by other browsers for their content process.

* wtf/MemoryPressureHandler.cpp:
(WTF::thresholdForMemoryKillOfActiveProcess):
(WTF::MemoryPressureHandler::thresholdForMemoryKill):

Canonical link: https://commits.webkit.org/250548@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294181 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@bnham bnham self-assigned this May 13, 2022
@bnham bnham added Safari 15 WebKit Process Model Bugs related to WebKit's multi-process architecture labels May 13, 2022
@bnham bnham requested a review from cdumez May 13, 2022 20:57
@bnham bnham force-pushed the eng/fg-webcontent-mem-limit branch from 223aafb to fde6208 Compare May 13, 2022 20:58
@bnham bnham requested a review from geoffreygaren May 13, 2022 20:59
Copy link
Contributor

@cdumez cdumez May 13, 2022

Choose a reason for hiding this comment

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

Change log says:

This patch adds a foreground memory limit of 8GB or 16GB depending on RAM size

but seems like the code does something a bit different? Even if you assume there is a single tab in the process, seems like 16GB of RAM would give you a limit of 8GB, no? Since you are using > and not >= in your check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I misread. Your change log only says it's based on RAM size but no actual values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the > rather than >= here is intentional to match the magic values that other browsers chose based on RAM size.

Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

r=me

@bnham bnham added the merge-queue Applied to send a pull request to merge-queue label May 13, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit f822d46 into WebKit:main May 13, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r294181 (250548@main): https://commits.webkit.org/250548@main

Reviewed commits have been landed. Closing PR #618 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKit Process Model Bugs related to WebKit's multi-process architecture

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants