Propagate NONE level in UserLevelMemoryPressureSignalGenerator [chromium/src : main]
0 views
Skip to first unread message
Patrick Monette (Gerrit)
Dec 1, 2025, 11:20:14 AM (6 days ago) Dec 1
to Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Takashi Sakamoto
Patrick Monette added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Takashi Sakamoto
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Dec 2, 2025, 11:42:18 AM (5 days ago) Dec 2
to Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Takashi Sakamoto
Patrick Monette added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Patrick Monette . resolved
@Takashi friendly ping
Related details
Attention is currently required from:
- Takashi Sakamoto
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Dec 2, 2025, 7:38:38 PM (5 days ago) Dec 2
to Patrick Monette, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Patrick Monette
Takashi Sakamoto voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Patrick Monette
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Dec 3, 2025, 10:33:06 AM (4 days ago) Dec 3
to Philip Jägenstedt, Arthur Sonzogni, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Arthur Sonzogni and Philip Jägenstedt
Patrick Monette added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 9 (Latest):
Patrick Monette . resolved
+Philip for third_party/blink
+Arthur for content/
PTAL
Related details
Attention is currently required from:
- Arthur Sonzogni
- Philip Jägenstedt
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Dec 4, 2025, 4:41:15 AM (4 days ago) Dec 4
to Patrick Monette, Philip Jägenstedt, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Patrick Monette and Philip Jägenstedt
Arthur Sonzogni added 6 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 10 (Latest):
Arthur Sonzogni . resolved
Thanks Patrick!
content/ LGTM, but I think I found a bug in blink ;-)
File content/browser/memory_pressure/user_level_memory_pressure_signal_generator.cc
Line 250, Patchset 10 (Latest):
// Don't notify duplicate NONE pressure level.Arthur Sonzogni . unresolved
The code handled CRITICAL differently from NONE and MODERATE.
Are you sure the comment is correct? It feels incorrect. Or maybe MODERATE isn't really used at the moment?
Line 252, Patchset 10 (Latest):
level != base::MEMORY_PRESSURE_LEVEL_CRITICAL) {Arthur Sonzogni . unresolved
Consider adding a brief comment explaining why CRITICAL is exempted from the check (i.e., supporting legacy listeners that rely on polling/repeated signals).
File third_party/blink/renderer/controller/user_level_memory_pressure_signal_generator.cc
Line 104, Patchset 10 (Latest):
last_critical_generated_ = std::nullopt;Arthur Sonzogni . unresolved
See my comment in Generate. We are going to immediately set it to "now" in L108.
Line 106, Patchset 10 (Latest):
// Only generate a signal if the last generated signal was not NONE.Arthur Sonzogni . unresolved
This repeats the code, so it adds no value, only noise.
Line 194, Patchset 10 (Latest):
last_critical_generated_ = now;Arthur Sonzogni . unresolved
There is a bug here. You should probably check `level == CRITICAL` before assigning this.
Could you please add a regression test checking the transition CRITICAL => NONE => CRITICAL.
Even if the transition NONE => CRITICAL is quick, we should check the the critical signal isn't delayed.
Related details
Attention is currently required from:
- Patrick Monette
- Philip Jägenstedt
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Dec 4, 2025, 4:38:01 PM (3 days ago) Dec 4
to Philip Jägenstedt, Arthur Sonzogni, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Arthur Sonzogni and Philip Jägenstedt
Patrick Monette added 6 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 12 (Latest):
Patrick Monette . resolved
Thanks for the thorough review Arthur! PTAnL
File content/browser/memory_pressure/user_level_memory_pressure_signal_generator.cc
The code handled CRITICAL differently from NONE and MODERATE.
Are you sure the comment is correct? It feels incorrect. Or maybe MODERATE isn't really used at the moment?
Patrick Monette
Yeah moderate is not used yet, but I agree the code and the comment looked contradictory. Fixed.
Consider adding a brief comment explaining why CRITICAL is exempted from the check (i.e., supporting legacy listeners that rely on polling/repeated signals).
Patrick Monette
Done
File third_party/blink/renderer/controller/user_level_memory_pressure_signal_generator.cc
See my comment in Generate. We are going to immediately set it to "now" in L108.
Patrick Monette
Good catch! I fixed Generate() now.
Line 106, Patchset 10:
// Only generate a signal if the last generated signal was not NONE.Arthur Sonzogni . resolved
This repeats the code, so it adds no value, only noise.
Patrick Monette
Done
There is a bug here. You should probably check `level == CRITICAL` before assigning this.
Could you please add a regression test checking the transition CRITICAL => NONE => CRITICAL.
Even if the transition NONE => CRITICAL is quick, we should check the the critical signal isn't delayed.
Attention is currently required from:
- Arthur Sonzogni
- Philip Jägenstedt
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Dec 5, 2025, 4:49:58 AM (3 days ago) Dec 5
to Patrick Monette, Arthur Sonzogni, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Arthur Sonzogni and Patrick Monette
Philip Jägenstedt voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Arthur Sonzogni
- Patrick Monette
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Dec 5, 2025, 5:23:09 AM (3 days ago) Dec 5
to Patrick Monette, Philip Jägenstedt, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Patrick Monette
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Patrick Monette
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Dec 5, 2025, 9:52:07 AM (2 days ago) Dec 5
to Arthur Sonzogni, Philip Jägenstedt, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Patrick Monette voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Dec 5, 2025, 9:56:19 AM (2 days ago) Dec 5
to Arthur Sonzogni, Philip Jägenstedt, Takashi Sakamoto, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Patrick Monette voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Dec 5, 2025, 9:59:15 AM (2 days ago) Dec 5
to Patrick Monette, Arthur Sonzogni, Philip Jägenstedt, Takashi Sakamoto, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
Propagate NONE level in UserLevelMemoryPressureSignalGenerator
In preparation for migrating all base::MemoryPressureListener
implementations to base::MemoryConsumer, the memory pressure signal
must become stateful.
In its current state, the MEMORY_PRESSURE_LEVEL_NONE is never sent to
MemoryPressureListeners. The API works by repeatedly sending either
MEMORY_PRESSURE_LEVEL_CRITICAL or MEMORY_PRESSURE_LEVEL_MODERATE until
the memory pressure state resolves.
The new base::MemoryConsumer API will work by only notifying state
transitions. There will no longer be repeat identical notifications,
and implementers can assume the memory pressure holds as long as they
don't receive another notification.
This change will enable the migration from the old behavior to the new
as it is a mix of both.
Bug: 462288601
Change-Id: I1fd766747c1de60972211427225567cd9d186c0a
Reviewed-by: Takashi Sakamoto <ta...@google.com>
Reviewed-by: Philip Jägenstedt <foo...@chromium.org>
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Commit-Queue: Patrick Monette <pmon...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1554646}
Files:
- M content/browser/memory_pressure/user_level_memory_pressure_signal_generator.cc
- M content/browser/memory_pressure/user_level_memory_pressure_signal_generator.h
- M content/renderer/render_thread_impl.cc
- M third_party/blink/public/web/DEPS
- M third_party/blink/public/web/web_user_level_memory_pressure_signal_generator.h
- M third_party/blink/renderer/controller/user_level_memory_pressure_signal_generator.cc
- M third_party/blink/renderer/controller/user_level_memory_pressure_signal_generator.h
- M third_party/blink/renderer/controller/user_level_memory_pressure_signal_generator_test.cc
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Change size: M
Delta: 9 files changed, 154 insertions(+), 45 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Takashi Sakamoto, +1 by Philip Jägenstedt, +1 by Arthur Sonzogni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chrome Crash (Prod) (Gerrit)
Dec 6, 2025, 1:58:29 PM (yesterday) Dec 6
to Chromium LUCI CQ, Patrick Monette, Arthur Sonzogni, Philip Jägenstedt, Takashi Sakamoto, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Chrome Crash (Prod) has created a revert of this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages