Fix DSE prewarm metrics for RenderProcess reuse [chromium/src : main]
Keita Suzuki (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Jiacheng Guo
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 4 comments![]( "Open in Gerrit")
Since the logic change is non-trivial, would I ask for a browser test to verify the UMA values are recorded as expected?
content::SiteInstanceProcessAssignment::REUSED_EXISTING_PROCESS));`REUSED_EXISTING_PROCESS` will be the process assignment source in multiple cases not only the RPH reuse for the prerender page. If DSEKeepAlive or multiple SRP in the same renderer feature is enabled, the process assignment will be the same value. A typical case will be:
Prewarm page -> Reuse as SRP A -> Host another SRP B -> Keep alive after closing A and B -> Reuse for C.
Do we want to record prewarm reuse for the page B and C as well?
auto* prewarm_data =IIUC, the code here is trying to query if there was once a prewarm page committed in the renderer process. Maybe it is better to do so in `GWSPageLoadMetricsObserver::OnCommit` rather than `RenderFrameHostImpl` since this class is for RFH rather than RPH. Please also add comments about what the code is trying to accomplish since it is a bit hard to understand.
GetProcess(), prewarm_data->prewarm_committed());Another question is that we are using the navigation request and the process when getting and setting the user data. Can we fetch the previous `prewarm_committed` value stored in the user data of the `RenderProcessHost` with a `NavigationRequest`? I assume them to be different maps.
Related details
- Keita Suzuki
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
Sorry for the delay.
Here are high-level feedback from the viewpoint of //content/public regulations.
static PrerenderPrewarmNavigationData* Get(We can still inherit content/public/browser/navigation_handle_user_data.h?
// Copyright 2025 The Chromium AuthorsHigh-level comment:
https://chromium.googlesource.com/chromium/src/+/main/content/public/README.md
Regarding the //content/public, we have some strict rules, and here are some rules that this CL is related, or might be related in the future.
//content/public should contain only interfaces, enums, structs and (rarely) static functions.
Interfaces that content implements usually should be pure abstract, because usually there's only one implementation. These should not be implemented outside of content. (i.e., content will freely assume that it can cast to its implementation(s)).
It‘s acceptable to put implementation files that hold constructors/destructors of interfaces/structs which might have member variables. For structs, this covers initializing member variables. For interfaces (i.e. RenderFrameObserver) this might cover things like automatic registration/unregistration. Normally we would put this small code in headers, but because of the clang checks against putting code in headers, we’re forced to put it in .cc files (we don't want to make a clang exception for the content/public directory since that would lead to confusion).
Only expose methods in the public API that embedders need. If a method is only used by other code in content, it belongs in foo_impl.h and not foo.h.
So, the implementation file contains some logic other than ctor/dtor, we need to split the implementation to _impl.cc so that we can hide actual implementation from outside //content, and to make the PrerenderPrewarmNavigationData pure virtual (== interface).
Related details
- Keita Suzuki
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. |
Keita Suzuki (Gerrit)
Keita Suzuki voted and added 5 comments![]( "Open in Gerrit")
Votes added by Keita Suzuki
| Commit-Queue | +1 |
5 comments
content::SiteInstanceProcessAssignment::REUSED_EXISTING_PROCESS));`REUSED_EXISTING_PROCESS` will be the process assignment source in multiple cases not only the RPH reuse for the prerender page. If DSEKeepAlive or multiple SRP in the same renderer feature is enabled, the process assignment will be the same value. A typical case will be:
Prewarm page -> Reuse as SRP A -> Host another SRP B -> Keep alive after closing A and B -> Reuse for C.
Do we want to record prewarm reuse for the page B and C as well?
To clarify, what we want to know is the conversion rate of the prewarm.
Personally, I think it is OK to have both
DSE Prewarm -> DSE A
and
DSE Prewarm -> [Multiple Sites on same RenderProcessHost] -> DSE A
as any subsequent navigations would also benefit from the Prewarm feature.
Does this make sense?
auto* prewarm_data =IIUC, the code here is trying to query if there was once a prewarm page committed in the renderer process. Maybe it is better to do so in `GWSPageLoadMetricsObserver::OnCommit` rather than `RenderFrameHostImpl` since this class is for RFH rather than RPH. Please also add comments about what the code is trying to accomplish since it is a bit hard to understand.
This is trying to query whether a committed `NavigationHandle` has the `PrerenderPrewarmNavigationData` associated with them.
I think it is OK to do so in PLMO, but I personally believe this should be set here for clarity.
GetProcess(), prewarm_data->prewarm_committed());Another question is that we are using the navigation request and the process when getting and setting the user data. Can we fetch the previous `prewarm_committed` value stored in the user data of the `RenderProcessHost` with a `NavigationRequest`? I assume them to be different maps.
Hmm, my understanding is that `NavigationRequest` might not exist when `RenderProcessHost` is querying for the previous commit, so we need to copy them to `RenderProcessHost`.
static PrerenderPrewarmNavigationData* Get(We can still inherit content/public/browser/navigation_handle_user_data.h?
I'm a bit torn on two opinions.
On one hand, I want to keep inheriting the `NavigationHandleUserData`, since that provides us with all the utils for NavigationHandle.
On the other hand, we are adding the same UserData to `RenderProcessHost` as well, which cannot use the NavigationHandle utils.
Because of this, I prepared a seperate utils for this UserData, and figured if we are using them, it might not be useful to inherit the `NavigationHandleUserData`.
WDYT?
// Copyright 2025 The Chromium AuthorsHigh-level comment:
https://chromium.googlesource.com/chromium/src/+/main/content/public/README.md
Regarding the //content/public, we have some strict rules, and here are some rules that this CL is related, or might be related in the future.
//content/public should contain only interfaces, enums, structs and (rarely) static functions.
Interfaces that content implements usually should be pure abstract, because usually there's only one implementation. These should not be implemented outside of content. (i.e., content will freely assume that it can cast to its implementation(s)).
It‘s acceptable to put implementation files that hold constructors/destructors of interfaces/structs which might have member variables. For structs, this covers initializing member variables. For interfaces (i.e. RenderFrameObserver) this might cover things like automatic registration/unregistration. Normally we would put this small code in headers, but because of the clang checks against putting code in headers, we’re forced to put it in .cc files (we don't want to make a clang exception for the content/public directory since that would lead to confusion).
Only expose methods in the public API that embedders need. If a method is only used by other code in content, it belongs in foo_impl.h and not foo.h.
So, the implementation file contains some logic other than ctor/dtor, we need to split the implementation to _impl.cc so that we can hide actual implementation from outside //content, and to make the PrerenderPrewarmNavigationData pure virtual (== interface).
Thanks for the pointers!
So just to clarify (and looking at the existing use case of the //content/public), the static methods seems to still be OK to exist in //cotent/public, but the `GetNavigationStatus` seems to be out of place?
In that case, I'll split it to a seperate file (or maybe move it under gws_plmo since it is not used in //content).
Related details
- Jiacheng Guo
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
// Copyright 2025 The Chromium AuthorsKeita SuzukiHigh-level comment:
https://chromium.googlesource.com/chromium/src/+/main/content/public/README.md
Regarding the //content/public, we have some strict rules, and here are some rules that this CL is related, or might be related in the future.
//content/public should contain only interfaces, enums, structs and (rarely) static functions.
Interfaces that content implements usually should be pure abstract, because usually there's only one implementation. These should not be implemented outside of content. (i.e., content will freely assume that it can cast to its implementation(s)).
It‘s acceptable to put implementation files that hold constructors/destructors of interfaces/structs which might have member variables. For structs, this covers initializing member variables. For interfaces (i.e. RenderFrameObserver) this might cover things like automatic registration/unregistration. Normally we would put this small code in headers, but because of the clang checks against putting code in headers, we’re forced to put it in .cc files (we don't want to make a clang exception for the content/public directory since that would lead to confusion).
Only expose methods in the public API that embedders need. If a method is only used by other code in content, it belongs in foo_impl.h and not foo.h.
So, the implementation file contains some logic other than ctor/dtor, we need to split the implementation to _impl.cc so that we can hide actual implementation from outside //content, and to make the PrerenderPrewarmNavigationData pure virtual (== interface).
Thanks for the pointers!
So just to clarify (and looking at the existing use case of the //content/public), the static methods seems to still be OK to exist in //cotent/public, but the `GetNavigationStatus` seems to be out of place?
In that case, I'll split it to a seperate file (or maybe move it under gws_plmo since it is not used in //content).
Right. The proposed approach sgtm. that seems a reasonable idea!
Related details
- Jiacheng Guo
- Keita Suzuki
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
static PrerenderPrewarmNavigationData* Get(Keita SuzukiWe can still inherit content/public/browser/navigation_handle_user_data.h?
I'm a bit torn on two opinions.
On one hand, I want to keep inheriting the `NavigationHandleUserData`, since that provides us with all the utils for NavigationHandle.
On the other hand, we are adding the same UserData to `RenderProcessHost` as well, which cannot use the NavigationHandle utils.Because of this, I prepared a seperate utils for this UserData, and figured if we are using them, it might not be useful to inherit the `NavigationHandleUserData`.
WDYT?
Ah, I see. OK, your proposal SGTM.
Jiacheng Guo (Gerrit)
Jiacheng Guo added 3 comments![]( "Open in Gerrit")
content::SiteInstanceProcessAssignment::REUSED_EXISTING_PROCESS));Keita Suzuki`REUSED_EXISTING_PROCESS` will be the process assignment source in multiple cases not only the RPH reuse for the prerender page. If DSEKeepAlive or multiple SRP in the same renderer feature is enabled, the process assignment will be the same value. A typical case will be:
Prewarm page -> Reuse as SRP A -> Host another SRP B -> Keep alive after closing A and B -> Reuse for C.
Do we want to record prewarm reuse for the page B and C as well?
To clarify, what we want to know is the conversion rate of the prewarm.
Personally, I think it is OK to have both
DSE Prewarm -> DSE A
and
DSE Prewarm -> [Multiple Sites on same RenderProcessHost] -> DSE Aas any subsequent navigations would also benefit from the Prewarm feature.
Does this make sense?
Thanks, that makes sense to me.
GetProcess(), prewarm_data->prewarm_committed());Keita SuzukiAnother question is that we are using the navigation request and the process when getting and setting the user data. Can we fetch the previous `prewarm_committed` value stored in the user data of the `RenderProcessHost` with a `NavigationRequest`? I assume them to be different maps.
Hmm, my understanding is that `NavigationRequest` might not exist when `RenderProcessHost` is querying for the previous commit, so we need to copy them to `RenderProcessHost`.
Ah I missed you're copying to the RPH. Thanks for clarifying and the comments.
// True when webcontent had dse prewarm.Since the user data is now bound to the navigation request and the RPH, the comments need update.
Would you explain that the `prewarm_committed_` will be true for the RPH once a prewarm page is committed in the process and will stay true in further reuses?
Related details
- Keita Suzuki
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. |
Keita Suzuki (Gerrit)
Keita Suzuki added 1 comment![]( "Open in Gerrit")
static PrerenderPrewarmNavigationData* Get(Keita SuzukiWe can still inherit content/public/browser/navigation_handle_user_data.h?
Takashi ToyoshimaI'm a bit torn on two opinions.
On one hand, I want to keep inheriting the `NavigationHandleUserData`, since that provides us with all the utils for NavigationHandle.
On the other hand, we are adding the same UserData to `RenderProcessHost` as well, which cannot use the NavigationHandle utils.Because of this, I prepared a seperate utils for this UserData, and figured if we are using them, it might not be useful to inherit the `NavigationHandleUserData`.
WDYT?
Ah, I see. OK, your proposal SGTM.
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 6 comments![]( "Open in Gerrit")
auto* prewarm_data =Keita SuzukiIIUC, the code here is trying to query if there was once a prewarm page committed in the renderer process. Maybe it is better to do so in `GWSPageLoadMetricsObserver::OnCommit` rather than `RenderFrameHostImpl` since this class is for RFH rather than RPH. Please also add comments about what the code is trying to accomplish since it is a bit hard to understand.
This is trying to query whether a committed `NavigationHandle` has the `PrerenderPrewarmNavigationData` associated with them.
I think it is OK to do so in PLMO, but I personally believe this should be set here for clarity.
Sorry for overlooking this in the initial high-level review, but +1 for the Jiacheng's suggestion now.
Another factor to decide the direction here is that the prewarm is a concept outside //content. So, if we want to handle this here, we need to abstract it as a embedder-triggered prerendering common operation. There would be no technical difficulty for that. It's just a small editorial changes, but it also remove the benefit of the code clarity here, i.e. we could only have ambiguous comment rather precise readable comment here.
On the other hand, if we can have this operation in the PLMO, we can keep having everything outside the //content and say "prewarm". Also code change could be smaller as we don't need to replace the struct.
// True when the corresponding UserData had dse prewarm.`DSE` (large capital to be consistent)
~PrerenderPrewarmNavigationData() override = default;also, disallow copy, and so on?
// The key to store and retrieve this data from RenderProcessHost.`NavigationHandle and`?
Also can this be in the private section?
I think external users don't need to know this key as they always access the instance via the Get{OrCreate} class method below.
class CONTENT_EXPORT PrerenderPrewarmNavigationDataAs discussed above, this class name is also problematic as the prewarm is a concept outside the //content.
// Copyright 2025 The Chromium AuthorsKeita SuzukiHigh-level comment:
https://chromium.googlesource.com/chromium/src/+/main/content/public/README.md
Regarding the //content/public, we have some strict rules, and here are some rules that this CL is related, or might be related in the future.
//content/public should contain only interfaces, enums, structs and (rarely) static functions.
Interfaces that content implements usually should be pure abstract, because usually there's only one implementation. These should not be implemented outside of content. (i.e., content will freely assume that it can cast to its implementation(s)).
It‘s acceptable to put implementation files that hold constructors/destructors of interfaces/structs which might have member variables. For structs, this covers initializing member variables. For interfaces (i.e. RenderFrameObserver) this might cover things like automatic registration/unregistration. Normally we would put this small code in headers, but because of the clang checks against putting code in headers, we’re forced to put it in .cc files (we don't want to make a clang exception for the content/public directory since that would lead to confusion).
Only expose methods in the public API that embedders need. If a method is only used by other code in content, it belongs in foo_impl.h and not foo.h.
So, the implementation file contains some logic other than ctor/dtor, we need to split the implementation to _impl.cc so that we can hide actual implementation from outside //content, and to make the PrerenderPrewarmNavigationData pure virtual (== interface).
Takashi ToyoshimaThanks for the pointers!
So just to clarify (and looking at the existing use case of the //content/public), the static methods seems to still be OK to exist in //cotent/public, but the `GetNavigationStatus` seems to be out of place?
In that case, I'll split it to a seperate file (or maybe move it under gws_plmo since it is not used in //content).
Right. The proposed approach sgtm. that seems a reasonable idea!
- Keita Suzuki
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. |
Keita Suzuki (Gerrit)
Keita Suzuki added 6 comments![]( "Open in Gerrit")
THanks for the reviews. I am still working on the test (might make it a seperate CL, since I am trying to add them to PLMO's browsertest, which requires adding setups for the prewarm .
auto* prewarm_data =Keita SuzukiIIUC, the code here is trying to query if there was once a prewarm page committed in the renderer process. Maybe it is better to do so in `GWSPageLoadMetricsObserver::OnCommit` rather than `RenderFrameHostImpl` since this class is for RFH rather than RPH. Please also add comments about what the code is trying to accomplish since it is a bit hard to understand.
Takashi ToyoshimaThis is trying to query whether a committed `NavigationHandle` has the `PrerenderPrewarmNavigationData` associated with them.
I think it is OK to do so in PLMO, but I personally believe this should be set here for clarity.
Sorry for overlooking this in the initial high-level review, but +1 for the Jiacheng's suggestion now.
Another factor to decide the direction here is that the prewarm is a concept outside //content. So, if we want to handle this here, we need to abstract it as a embedder-triggered prerendering common operation. There would be no technical difficulty for that. It's just a small editorial changes, but it also remove the benefit of the code clarity here, i.e. we could only have ambiguous comment rather precise readable comment here.
On the other hand, if we can have this operation in the PLMO, we can keep having everything outside the //content and say "prewarm". Also code change could be smaller as we don't need to replace the struct.
OK, sounds good. Moved it to GWS PLMO.
// True when the corresponding UserData had dse prewarm.`DSE` (large capital to be consistent)
Done
~PrerenderPrewarmNavigationData() override = default;also, disallow copy, and so on?
Done
// The key to store and retrieve this data from RenderProcessHost.`NavigationHandle and`?
Also can this be in the private section?
I think external users don't need to know this key as they always access the instance via the Get{OrCreate} class method below.
Done
class CONTENT_EXPORT PrerenderPrewarmNavigationDataAs discussed above, this class name is also problematic as the prewarm is a concept outside the //content.
- Jiacheng Guo
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 2 comments![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
2 comments
HISTOGRAM_PREFIX "Prerender.PrewarmNavigationStatus";Should this have a suffix 2? (IIUC, you changed the condition to record this only for the renderer-initiated cases.)
auto* prewarm_data =Keita SuzukiIIUC, the code here is trying to query if there was once a prewarm page committed in the renderer process. Maybe it is better to do so in `GWSPageLoadMetricsObserver::OnCommit` rather than `RenderFrameHostImpl` since this class is for RFH rather than RPH. Please also add comments about what the code is trying to accomplish since it is a bit hard to understand.
Takashi ToyoshimaThis is trying to query whether a committed `NavigationHandle` has the `PrerenderPrewarmNavigationData` associated with them.
I think it is OK to do so in PLMO, but I personally believe this should be set here for clarity.
Keita SuzukiSorry for overlooking this in the initial high-level review, but +1 for the Jiacheng's suggestion now.
Another factor to decide the direction here is that the prewarm is a concept outside //content. So, if we want to handle this here, we need to abstract it as a embedder-triggered prerendering common operation. There would be no technical difficulty for that. It's just a small editorial changes, but it also remove the benefit of the code clarity here, i.e. we could only have ambiguous comment rather precise readable comment here.
On the other hand, if we can have this operation in the PLMO, we can keep having everything outside the //content and say "prewarm". Also code change could be smaller as we don't need to replace the struct.
OK, sounds good. Moved it to GWS PLMO.
- Jiacheng Guo
- Keita Suzuki
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. |
Keita Suzuki (Gerrit)
Keita Suzuki added 4 comments![]( "Open in Gerrit")
Since the logic change is non-trivial, would I ask for a browser test to verify the UMA values are recorded as expected?
Done
HISTOGRAM_PREFIX "Prerender.PrewarmNavigationStatus";Should this have a suffix 2? (IIUC, you changed the condition to record this only for the renderer-initiated cases.)
True. Updated.
HISTOGRAM_PREFIX "Prerender.PrewarmNavigationStatus";Should this have a suffix 2? (IIUC, you changed the condition to record this only for the renderer-initiated cases.)
I don't think we need to rename this, since we are not getting any data on the original histogram due to the implicaton errors.
Let's keep the original name
Since the user data is now bound to the navigation request and the RPH, the comments need update.
Would you explain that the `prewarm_committed_` will be true for the RPH once a prewarm page is committed in the process and will stay true in further reuses?
Done
Related details
- Jiacheng Guo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keita Suzuki (Gerrit)
Keita Suzuki voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix DSE prewarm metrics for RenderProcess reuse
The current DSE prewarm metrics can be inaccurate in scenarios involving
RenderProcessHost reuse. The prewarm status was tracked on the
NavigationHandle, which failed to correctly attribute prewarming when a
prewarmed process was reused for a subsequent, different navigation.
This resulted in undercounting prewarmed navigations.
This change moves the responsibility of storing the prewarm status to
the RenderProcessHost itself. When a prewarmed prerender navigation
commits, the status is attached directly to the process. The page load
metrics observer can then reliably check the process to determine if the
navigation occurred in a prewarmed renderer, regardless of process
reuse.
To facilitate this, PrerenderPrewarmNavigationData is moved from
//components/page_load_metrics to the content layer in
//content/public/browser.
Details (internal doc):
https://docs.google.com/document/d/1N9u6oE5EjJPyuDINnMKTWxNpNiSF6u71B5aTGmEgVAo/edit?tab=t.0
NO_IFTTT=Moving the target file to a new directory
- M chrome/browser/preloading/prerender/prerender_manager.cc
- M chrome/browser/preloading/prerender/prerender_manager.h
- M components/page_load_metrics/google/browser/gws_page_load_metrics_observer.cc
- M components/page_load_metrics/google/browser/gws_page_load_metrics_observer.h
- M components/page_load_metrics/google/browser/prerender_prewarm_navigation_data.cc
- M components/page_load_metrics/google/browser/prerender_prewarm_navigation_data.h
Code-Review: +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |