Record font fallback metrics by script in GWS PLMO
[DSEPrewarm] Record font fallback metrics by script in GWS PLMO
This CL extends the font loading performance metrics to report fallback
count and duration grouped by Unicode script.
Specifically:
- We track fallback count and duration per UScriptCode in FontPerformance.
- We plumb this script-specific data through WebPerformanceMetricsForReporting
and the Page Load Metrics Mojo pipeline (FontLoadingMetrics).
- In GWSPageLoadMetricsObserver, we log the new metrics for FCP, AFTEnd, and
Complete milestones:
- PageLoad.Clients.GoogleSearch.FontLoading.FallbackCountByScript.<Script>.<Milestone>
- PageLoad.Clients.GoogleSearch.FontLoading.FallbackDurationByScript.<Script>.<Milestone>
Supported scripts are Latin, Han, Hangul, Hiragana, Katakana, Arabic,
Bengali, Devanagari, Cyrillic, and others are grouped under "Other".
Bug: 517003594
TAG=agy
CONV=75c1922d-ce5a-4662-bba8-8ef1f1050292
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
struct ScriptFallbackInfo {For each struct member please add comments.
std::string_view GetScriptSuffix(int32_t script_code) {instead of adding suffix, can we simply record a enumerated histogram for all the scripts?
struct ScriptFontFallbackDetailsForReporting {also add comments here to each member.
static void Reset();why was this change required?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
also add comments here to each member.
Done
#include <unicode/uscript.h>You don't need to include `<unicode/uscript.h>` here since you only use `int32_t` for the script code in this header. Removing it can help improve compilation times.
why was this change required?
Done
base::TimeDelta time) {The commit message mentions tracking "fallback count and duration grouped by Unicode script", and logging `PageLoad.Clients.GoogleSearch.FontLoading.FallbackDurationByScript.<Script>.<Milestone>`. However, `time` is not used in the script-specific map here and the duration histogram isn't added.
Should the duration also be tracked as initially intended, or should the commit message be updated to reflect that only counts are tracked?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
You don't need to include `<unicode/uscript.h>` here since you only use `int32_t` for the script code in this header. Removing it can help improve compilation times.
Done
The commit message mentions tracking "fallback count and duration grouped by Unicode script", and logging `PageLoad.Clients.GoogleSearch.FontLoading.FallbackDurationByScript.<Script>.<Milestone>`. However, `time` is not used in the script-specific map here and the duration histogram isn't added.
Should the duration also be tracked as initially intended, or should the commit message be updated to reflect that only counts are tracked?
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
For each struct member please add comments.
Done
instead of adding suffix, can we simply record a enumerated histogram for all the scripts?
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
}If `type` doesn't match any case (e.g., an invalid enum value passed via IPC from a compromised renderer), control will fall off the end of this function without returning a value, resulting in Undefined Behavior.
Please add `NOTREACHED();` or return a default string (e.g., `"Other"`) at the end of the function.
for (const auto& details : perf.GetScriptFontFallbackDetails()) {Multiple `details` entries can map to the same `mojom::ScriptType`. For example, multiple unlisted scripts will map to `kOther`, and different entries with `is_emoji == true` will map to `kEmoji`.
Because `RecordFontMetrics` in the browser logs exactly one sample per item in this array, pushing them separately will cause the browser to record multiple independent histogram samples for the same script in a single page load.
You should aggregate `details.fallback_count` by `mojom::ScriptType` before populating `font_metrics->script_fallback_metrics`. For example:
```cpp
std::map<mojom::ScriptType, uint32_t> aggregated;
for (const auto& details : perf.GetScriptFontFallbackDetails()) {
aggregated[MapToMojoScriptType(details.script_code, details.is_emoji)] +=
details.fallback_count;
}
for (const auto& [type, count] : aggregated) {
auto info = mojom::ScriptFallbackInfo::New();
info->script_type = type;
info->fallback_count = count;
font_metrics->script_fallback_metrics.push_back(std::move(info));
}
```
FontPerformance::AddSystemFallbackFontTime(script_code, is_emoji,`timer.Elapsed()` evaluates as an argument at the end of this block, which means the time spent querying ICU (`uscript_getScript`) will be included in the measurement.
You should capture the elapsed time immediately after `PlatformFallbackFontForCharacter()` to avoid artificially inflating the fallback duration metric.
```cpp
base::TimeDelta elapsed = timer.Elapsed();
int32_t script_code = USCRIPT_INVALID_CODE;
// ...
FontPerformance::AddSystemFallbackFontTime(script_code, is_emoji, elapsed);
```
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
If `type` doesn't match any case (e.g., an invalid enum value passed via IPC from a compromised renderer), control will fall off the end of this function without returning a value, resulting in Undefined Behavior.
Please add `NOTREACHED();` or return a default string (e.g., `"Other"`) at the end of the function.
Done
for (const auto& details : perf.GetScriptFontFallbackDetails()) {Multiple `details` entries can map to the same `mojom::ScriptType`. For example, multiple unlisted scripts will map to `kOther`, and different entries with `is_emoji == true` will map to `kEmoji`.
Because `RecordFontMetrics` in the browser logs exactly one sample per item in this array, pushing them separately will cause the browser to record multiple independent histogram samples for the same script in a single page load.
You should aggregate `details.fallback_count` by `mojom::ScriptType` before populating `font_metrics->script_fallback_metrics`. For example:
```cpp
std::map<mojom::ScriptType, uint32_t> aggregated;
for (const auto& details : perf.GetScriptFontFallbackDetails()) {
aggregated[MapToMojoScriptType(details.script_code, details.is_emoji)] +=
details.fallback_count;
}
for (const auto& [type, count] : aggregated) {
auto info = mojom::ScriptFallbackInfo::New();
info->script_type = type;
info->fallback_count = count;
font_metrics->script_fallback_metrics.push_back(std::move(info));
}
```
Done
FontPerformance::AddSystemFallbackFontTime(script_code, is_emoji,`timer.Elapsed()` evaluates as an argument at the end of this block, which means the time spent querying ICU (`uscript_getScript`) will be included in the measurement.
You should capture the elapsed time immediately after `PlatformFallbackFontForCharacter()` to avoid artificially inflating the fallback duration metric.
```cpp
base::TimeDelta elapsed = timer.Elapsed();
int32_t script_code = USCRIPT_INVALID_CODE;
// ...
FontPerformance::AddSystemFallbackFontTime(script_code, is_emoji, elapsed);
```
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
#include <unicode/uscript.h>I think this is `#include "third_party/icu/source/common/unicode/uscript.h"` though this still can pass compiling.
But you may not need this in this file?
font_loading_metrics->fallback_duration = base::Milliseconds(150);
font_loading_metrics->fallback_count = 14;
font_loading_metrics->fallback_initial_duration = base::Milliseconds(42);
font_loading_metrics->shape_cache_hit_count = 80;
font_loading_metrics->shape_cache_miss_count = 20;We initialize several fields with expected values here, and check them below after line 746. So, can we use const variables for them to set here and use them as expected values below so that we should not make a mistake?
// This is mapped from Blink's internal UScriptCode or fallback priority.Can we use `LINT.IfChange` here? The other size is in the histograms.xml
mojom::ScriptType MapToMojoScriptType(int32_t script_code, bool is_emoji) {Keep using UScriptCode here and there.
struct ScriptFontFallbackDetailsForReporting {Can we keep UScriptCode and size_t?
See other relevant comments below.
int32_t script_code = USCRIPT_INVALID_CODE;
bool is_emoji = IsNonTextFallbackPriority(fallback_priority);
UErrorCode err = U_ZERO_ERROR;
UScriptCode script = uscript_getScript(lookup_char, &err);
if (U_SUCCESS(err)) {
script_code = static_cast<int32_t>(script);
}Let's keep the original type as we can as possible while detecting.
```
bool is_emoji = IsNonTextFallbackPriority(fallback_priority);
UErrorCode err = U_ZERO_ERROR;
UScriptCode script = uscript_getScript(lookup_char, &err);
if (U_FAILURE(err)) {
script = USCRIPT_INVALID_CODE;
}
```
static const std::map<ScriptKey, uint32_t>& GetScriptFallbackCounts();Using `size_t` can clarify the meaning as this is a counter.
static void AddSystemFallbackFontTime(int32_t script_code,Can we still use UScriptCode?
bool operator<(const ScriptKey& other) const {
return std::tie(script, is_emoji) <
std::tie(other.script, other.is_emoji);
}You can skip this manual code by using the following code in C++20.
```
friend auto operator<=>(const ScriptKey&, const ScriptKey&) = default;
```
```#include <compare>``` may be needed.
if (!IsMainThread()) [[unlikely]] {Just in case, is this the expected case?
If so, you can add some comments, and why it's ok to skip non-main thread use cases.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// This is mapped from Blink's internal UScriptCode or fallback priority.Also, can we say this is based on ICU's UScriptType, and how you modified it with what kind of intentions.
static void Reset() {maybe CHECK(IsMainThread()); and also other methods need the check?
#include <unicode/uscript.h>#include "third_party/icu/source/common/unicode/uscript.h"
Sorry, I might have missed this.
struct ScriptFontFallbackDetailsForReporting {Can we keep UScriptCode and size_t?
See other relevant comments below.
maybe resolved?
int32_t script_code = USCRIPT_INVALID_CODE;
bool is_emoji = IsNonTextFallbackPriority(fallback_priority);
UErrorCode err = U_ZERO_ERROR;
UScriptCode script = uscript_getScript(lookup_char, &err);
if (U_SUCCESS(err)) {
script_code = static_cast<int32_t>(script);
}Let's keep the original type as we can as possible while detecting.
```
bool is_emoji = IsNonTextFallbackPriority(fallback_priority);
UErrorCode err = U_ZERO_ERROR;
UScriptCode script = uscript_getScript(lookup_char, &err);
if (U_FAILURE(err)) {
script = USCRIPT_INVALID_CODE;
}
```
resolved?
#include <unicode/uscript.h>#include "third_party/icu/source/common/unicode/uscript.h"
static const std::map<ScriptKey, uint32_t>& GetScriptFallbackCounts();Using `size_t` can clarify the meaning as this is a counter.
resolved?
static void AddSystemFallbackFontTime(int32_t script_code,Can we still use UScriptCode?
resolved?
bool operator<(const ScriptKey& other) const {
return std::tie(script, is_emoji) <
std::tie(other.script, other.is_emoji);
}You can skip this manual code by using the following code in C++20.
```
friend auto operator<=>(const ScriptKey&, const ScriptKey&) = default;
``````#include <compare>``` may be needed.
resolved?
int32_t script;keep UScriptCode
resolved?
Just in case, is this the expected case?
If so, you can add some comments, and why it's ok to skip non-main thread use cases.
I will drop this comment as this is just consistent with other methods.
LINT.IfChange here
resolved?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
Sorry! I was discussing a couple of things with AI agent and some how the comments slipped off. Updated!
I think this is `#include "third_party/icu/source/common/unicode/uscript.h"` though this still can pass compiling.
But you may not need this in this file?
Done. Since we refactored the test to use the Mojo ScriptType enum instead of ICU's UScriptCode constants, this include is indeed no longer needed. I have removed it entirely.
font_loading_metrics->fallback_duration = base::Milliseconds(150);
font_loading_metrics->fallback_count = 14;
font_loading_metrics->fallback_initial_duration = base::Milliseconds(42);
font_loading_metrics->shape_cache_hit_count = 80;
font_loading_metrics->shape_cache_miss_count = 20;We initialize several fields with expected values here, and check them below after line 746. So, can we use const variables for them to set here and use them as expected values below so that we should not make a mistake?
Done. Defined constexpr constants for all simulated/expected values (e.g., kFallbackDuration, kFallbackCount, kLatinFallbackCount, etc.) at the beginning of the test case and used them for both setup and verification.
// This is mapped from Blink's internal UScriptCode or fallback priority.Can we use `LINT.IfChange` here? The other size is in the histograms.xml
Done. Added LINT.IfChange here and a corresponding LINT.ThenChange in histograms.xml around the GWSFontFallbackScript variants.
// This is mapped from Blink's internal UScriptCode or fallback priority.Also, can we say this is based on ICU's UScriptType, and how you modified it with what kind of intentions.
Done. Expanded the comment to clarify that this is a curated subset of ICU's UScriptCode (plus kEmoji) designed specifically for tracking high-impact font fallback categories in UMA.
}Keita Suzukiinsert an empty line
Done. Inserted an empty line.
#include <unicode/uscript.h>Keita Suzukiditto
Done. Updated the include to the full path: #include "third_party/icu/source/common/unicode/uscript.h".
mojom::ScriptType MapToMojoScriptType(int32_t script_code, bool is_emoji) {Keep using UScriptCode here and there.
Done. Updated MapToMojoScriptType to take UScriptCode directly, avoiding the early cast to int32_t.
#include <unicode/uscript.h>#include "third_party/icu/source/common/unicode/uscript.h"
Sorry, I might have missed this.
Unfortunately, it seems like including "third_party/icu/source/common/unicode/uscript.h" violates Blink's checkdeps rules, which forbid direct dependencies on that directory from Blink. We must use the short <unicode/uscript.h> here.
```
You added one or more #includes that violate checkdeps rules.
third_party/blink/public/web/web_performance_metrics_for_reporting.h
Illegal include: "third_party/icu/source/common/unicode/uscript.h"
Because of "-third_party/icu/source/common/unicode" from third_party's include_rules.
```
struct ScriptFontFallbackDetailsForReporting {Can we keep UScriptCode and size_t?
See other relevant comments below.
Done. Included <unicode/uscript.h> and updated ScriptFontFallbackDetailsForReporting to use UScriptCode for script_code and size_t for fallback_count.
int32_t script_code = USCRIPT_INVALID_CODE;
bool is_emoji = IsNonTextFallbackPriority(fallback_priority);
UErrorCode err = U_ZERO_ERROR;
UScriptCode script = uscript_getScript(lookup_char, &err);
if (U_SUCCESS(err)) {
script_code = static_cast<int32_t>(script);
}Let's keep the original type as we can as possible while detecting.
```
bool is_emoji = IsNonTextFallbackPriority(fallback_priority);
UErrorCode err = U_ZERO_ERROR;
UScriptCode script = uscript_getScript(lookup_char, &err);
if (U_FAILURE(err)) {
script = USCRIPT_INVALID_CODE;
}
```
Done. Adopted your suggested simplification: we now obtain UScriptCode script directly and pass it to FontPerformance after checking for failure, avoiding the separate script_code variable and early casting.
#include <unicode/uscript.h>#include "third_party/icu/source/common/unicode/uscript.h"
ditto.
static const std::map<ScriptKey, uint32_t>& GetScriptFallbackCounts();Using `size_t` can clarify the meaning as this is a counter.
Done. Updated GetScriptFallbackCounts and the underlying map value type to size_t. Also updated system_fallback_count_ and its getter SystemFallbackFontCount() to use size_t as well.
static void AddSystemFallbackFontTime(int32_t script_code,Can we still use UScriptCode?
Done. Updated AddSystemFallbackFontTime to take UScriptCode.
static void Reset() {maybe CHECK(IsMainThread()); and also other methods need the check?
seems like font fallback can legally happen on worker threads (e.g., via OffscreenCanvas in Workers), so a CHECK might cause crashes.
bool operator<(const ScriptKey& other) const {
return std::tie(script, is_emoji) <
std::tie(other.script, other.is_emoji);
}You can skip this manual code by using the following code in C++20.
```
friend auto operator<=>(const ScriptKey&, const ScriptKey&) = default;
``````#include <compare>``` may be needed.
This is extremely good to know! Simplified the struct by using C++20's default three-way comparison operator and including <compare>.
int32_t script;Keita Suzukikeep UScriptCode
Done. Changed ScriptKey::script type to UScriptCode.
if (!IsMainThread()) [[unlikely]] {Just in case, is this the expected case?
If so, you can add some comments, and why it's ok to skip non-main thread use cases.
Done. Added a comment explaining that FontPerformance collects metrics for Page Load Metrics (main-thread only). Since FontCache is thread-specific (via FontGlobalContext), font fallback can be legally triggered on worker threads (e.g. via OffscreenCanvas in Workers), but we safely ignore those to avoid thread-safety/locking overhead on these global static metrics.
Keita SuzukiLINT.IfChange here
Done. Added LINT.ThenChange pointing back to the Mojo ScriptType enum.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
#include <unicode/uscript.h>Keita Suzuki#include "third_party/icu/source/common/unicode/uscript.h"
Sorry, I might have missed this.
Unfortunately, it seems like including "third_party/icu/source/common/unicode/uscript.h" violates Blink's checkdeps rules, which forbid direct dependencies on that directory from Blink. We must use the short <unicode/uscript.h> here.
```
You added one or more #includes that violate checkdeps rules.
third_party/blink/public/web/web_performance_metrics_for_reporting.h
Illegal include: "third_party/icu/source/common/unicode/uscript.h"
Because of "-third_party/icu/source/common/unicode" from third_party's include_rules.
```
Oh, you are right.
It seems the ICU headers are handled as system headers in Blink.
#include <unicode/uscript.h>Keita Suzuki#include "third_party/icu/source/common/unicode/uscript.h"
ditto.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
static void Reset() {Keita Suzukimaybe CHECK(IsMainThread()); and also other methods need the check?
seems like font fallback can legally happen on worker threads (e.g., via OffscreenCanvas in Workers), so a CHECK might cause crashes.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
@har...@chromium.org, could I get your review on third_party/blink/public/web/ web_performance_metrics_for_reporting.h?
This adds struct member and a Getter definition for renderer performance reporting.
Thanks!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
[DSEPrewarm] Record font fallback metrics by script in GWS PLMO
This CL extends the font loading performance metrics to report fallback
count grouped by Unicode script (and Emoji) using separate histograms.
Specifically:
- We track fallback count per script/emoji in FontPerformance.
- We plumb this script-specific data through WebPerformanceMetricsForReporting
and the Page Load Metrics Mojo pipeline (FontLoadingMetrics).
- In GWSPageLoadMetricsObserver, we log:
- PageLoad.Clients.GoogleSearch.FontLoading.FallbackCount.<Script>.<Milestone>
(where <Script> is Latn, Hani, Hang, Hira, Kana, Arab, Beng, Deva, Cyrl,
Zyyy, Emoji, or Other)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |