Add if origin is recently accessed in cache rejection status metrics [chromium/src : main]
Mingyu Lei (Gerrit)
Mingyu Lei added 1 comment![]( "Open in Gerrit")
}Mingyu LeiThis is concerning to me. Navigation is already performance sensitive and this adds a traversal of *all* session history entries for the current profile on every navigation that hits the network. Can we at least instrument with measurements how long does this take?
For example, I have currently 800+ tabs in my work profile to manage my state. Each tab has non-trivial session history. It is likely this change will add some milliseconds to each navigation I perform.
Nasko OskovThanks, yeah we are aware of the potential performance impact and that's why the feature flag was added. When it's turned off, IsMainFrameOriginAccessibleByHistoryNavigation will just return false (see the help method in content/browser/url_loader_factory_params_helper.cc.
The purpose of setting this flag is for metrics collecting, and we will only enable it to a very small amount of users (1% should be enough) in stable. After we have the metrics we want, the logic will be cleaned up.
Mingyu LeiI do not believe we should be experimenting on 1% stable with this code. Happy to discuss alternatives in higher bandwidth modes, but we should not be landing this CL.
I tried the local build with around 100 tabs, each with some entries, and the time taken for the tab traversal is around 0.1~0.2ms, yes I expect it to grow to more than 1ms when the number of tabs grow larger.
Also I found that this metric may not be as useful as we hope it to be, for example, when restoring a tab, it might be navigating to a resource whose main frame origin is not accessible (since it's closed before the restore). After some discussion with @rak...@chromium.org, I reimplement this and now we are differentiating the navigation by whether "it's loading a resource whose main frame origin is recently accessed". This actually sounds like another layer of LRU cache but it's for origin only.
Related details
- Adam Rice
- Adam Rice
- Hidehiko Abe
- Nasko Oskov
- Nidhi Jaju
- Rakina Zata Amni
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
Mingyu LeiThis is concerning to me. Navigation is already performance sensitive and this adds a traversal of *all* session history entries for the current profile on every navigation that hits the network. Can we at least instrument with measurements how long does this take?
For example, I have currently 800+ tabs in my work profile to manage my state. Each tab has non-trivial session history. It is likely this change will add some milliseconds to each navigation I perform.
Nasko OskovThanks, yeah we are aware of the potential performance impact and that's why the feature flag was added. When it's turned off, IsMainFrameOriginAccessibleByHistoryNavigation will just return false (see the help method in content/browser/url_loader_factory_params_helper.cc.
The purpose of setting this flag is for metrics collecting, and we will only enable it to a very small amount of users (1% should be enough) in stable. After we have the metrics we want, the logic will be cleaned up.
Mingyu LeiI do not believe we should be experimenting on 1% stable with this code. Happy to discuss alternatives in higher bandwidth modes, but we should not be landing this CL.
I tried the local build with around 100 tabs, each with some entries, and the time taken for the tab traversal is around 0.1~0.2ms, yes I expect it to grow to more than 1ms when the number of tabs grow larger.
Also I found that this metric may not be as useful as we hope it to be, for example, when restoring a tab, it might be navigating to a resource whose main frame origin is not accessible (since it's closed before the restore). After some discussion with @rak...@chromium.org, I reimplement this and now we are differentiating the navigation by whether "it's loading a resource whose main frame origin is recently accessed". This actually sounds like another layer of LRU cache but it's for origin only.
Thank you for re-evaluating and coming up with a different solution. It looks like a better to have a LRU cache to store those. I have a comment on that solution, but I think this thread is at this point complete and I will resolve it.
GetRecentlyAccessedOriginSet().Put(std::move(origin));If we continuously put origins in the set without pruning it periodically, wouldn't it grow unbounded over time? There is no recency defined for this set.
Related details
- Adam Rice
- Adam Rice
- Hidehiko Abe
- Mingyu Lei
Mingyu Lei (Gerrit)
Mingyu Lei voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mingyu Lei
| Commit-Queue | +1 |
1 comment
GetRecentlyAccessedOriginSet().Put(std::move(origin));If we continuously put origins in the set without pruning it periodically, wouldn't it grow unbounded over time? There is no recency defined for this set.
The size is a concern, so the set returned by `GetRecentlyAccessedOriginSet()` *is* an LRUCacheSet and I put the limit in feature flag param (`kRecentlyAccessedOriginCacheSize`, default to 64). The eviction behavior is also covered in the unit test.
Related details
- Adam Rice
- Adam Rice
- Hidehiko Abe
- Nasko Oskov
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
// `IS_MAIN_FRAME_ORIGIN_RECENTLY_ACCESSED` load flag.It would be good for the comment to mention that this only affects metrics.
Related details
- Hidehiko Abe
- Mingyu Lei
Adam Rice (Gerrit)
Adam Rice voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hidehiko Abe
- Mingyu Lei
- Nasko Oskov
- Nidhi Jaju
- Rakina Zata Amni
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
2 comments
LGTM
GetRecentlyAccessedOriginSet().Put(std::move(origin));Mingyu LeiIf we continuously put origins in the set without pruning it periodically, wouldn't it grow unbounded over time? There is no recency defined for this set.
The size is a concern, so the set returned by `GetRecentlyAccessedOriginSet()` *is* an LRUCacheSet and I put the limit in feature flag param (`kRecentlyAccessedOriginCacheSize`, default to 64). The eviction behavior is also covered in the unit test.
- Hidehiko Abe
- Mingyu Lei
- Nidhi Jaju
- Rakina Zata Amni
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tsuyoshi Horo
| Code-Review | +1 |
1 comment
Related details
- Hidehiko Abe
- Mingyu Lei
- Nidhi Jaju
- Rakina Zata Amni
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hidehiko Abe
- Mingyu Lei
- Nidhi Jaju
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Records the outcome of considering whether to reject a cache entry based on
in-memory hints. This is used to avoid accessing disk for entries that are
likely to be unusable. Recorded every time we try to open a cache entry, but
there is no active one.This is the same as the description in "HttpCache.EntryRejectionStatus". Can we just make this a variant on the previous histogram, and update the summary in the variants accordingly to mention when it's recorded?
Related details
- Hidehiko Abe
- Mingyu Lei
Mingyu Lei (Gerrit)
Mingyu Lei voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mingyu Lei
| Commit-Queue | +1 |
3 comments
for more background and discussion (googlers only)Nasko OskovI do not have access to this document, even as a googler, so I cannot sign off on this CL without understanding why this change is necessary.
Mingyu LeiThanks for sharing the document. I am not convinced that walking all session history on each navigation synchronously is right approach here. Can this be accomplished in a different way that is not on the blocking path?
Done
It would be good for the comment to mention that this only affects metrics.
Done
Records the outcome of considering whether to reject a cache entry based on
in-memory hints. This is used to avoid accessing disk for entries that are
likely to be unusable. Recorded every time we try to open a cache entry, but
there is no active one.This is the same as the description in "HttpCache.EntryRejectionStatus". Can we just make this a variant on the previous histogram, and update the summary in the variants accordingly to mention when it's recorded?
- Hidehiko Abe
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei added 1 comment![]( "Open in Gerrit")
Done
Related details
- Hidehiko Abe
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
Nidhi Jaju (Gerrit)
Nidhi Jaju voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nidhi Jaju
| Code-Review | +1 |
2 comments
This metric logs the outcome of considering whether to reject a cache entry
based on in-memory hints, which is used to avoid accessing disk for entries
that are likely to be unusable.
The general metric (with empty variant name) is recorded every time when we
try to open a cache entry but no active entry is found. A variant is
recorded specifically when the `UpdateIsMainFrameOriginRecentlyAccessed`
flag is set, indicating the request contains recent-access information for
the resource's main frame origin.
</summary>
<token key="Type">
<variant name="" summary="recorded for every request"/>
<variant name=".MainFrameOriginNotRecentlyAccessed"
summary="requests to resources whose main frame's origin is not
recently accessed"/>
<variant name=".MainFrameOriginRecentlyAccessed"
summary="requests to resources whose main frame's origin is recently
accessed"/>Sorry if it was unclear, we can actually use the variants' summary in the histogram description, otherwise we won't see the differences on the dashboard.
```suggestion
Records the outcome of considering whether to reject a cache entry based on
in-memory hints. This is used to avoid accessing disk for entries that are
likely to be unusable. Recorded every time we try to open a cache entry, but
there is no active one{Type}
</summary>
<token key="Type">
<variant name="" summary="."/>
<variant name=".MainFrameOriginNotRecentlyAccessed"
summary=", only if `UpdateIsMainFrameOriginRecentlyAccessed` is
enabled and the request is for a resource whose main frame's origin is not
recently accessed."/>
<variant name=".MainFrameOriginRecentlyAccessed"
summary=", only if `UpdateIsMainFrameOriginRecentlyAccessed` is
enabled and the request is for a resource whose main frame's origin is recently
accessed."/>
```Related details
- Hidehiko Abe
- Mingyu Lei
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mingyu Lei
| Commit-Queue | +1 |
1 comment
This metric logs the outcome of considering whether to reject a cache entry
based on in-memory hints, which is used to avoid accessing disk for entries
that are likely to be unusable.
The general metric (with empty variant name) is recorded every time when we
try to open a cache entry but no active entry is found. A variant is
recorded specifically when the `UpdateIsMainFrameOriginRecentlyAccessed`
flag is set, indicating the request contains recent-access information for
the resource's main frame origin.
</summary>
<token key="Type">
<variant name="" summary="recorded for every request"/>
<variant name=".MainFrameOriginNotRecentlyAccessed"
summary="requests to resources whose main frame's origin is not
recently accessed"/>
<variant name=".MainFrameOriginRecentlyAccessed"
summary="requests to resources whose main frame's origin is recently
accessed"/>Sorry if it was unclear, we can actually use the variants' summary in the histogram description, otherwise we won't see the differences on the dashboard.
```suggestion
Records the outcome of considering whether to reject a cache entry based on
in-memory hints. This is used to avoid accessing disk for entries that are
likely to be unusable. Recorded every time we try to open a cache entry, but
there is no active one{Type}
</summary>
<token key="Type">
<variant name="" summary="."/>
<variant name=".MainFrameOriginNotRecentlyAccessed"
summary=", only if `UpdateIsMainFrameOriginRecentlyAccessed` is
enabled and the request is for a resource whose main frame's origin is not
recently accessed."/>
<variant name=".MainFrameOriginRecentlyAccessed"
summary=", only if `UpdateIsMainFrameOriginRecentlyAccessed` is
enabled and the request is for a resource whose main frame's origin is recently
accessed."/>
```
Oh I thought we can only replace with the variant name in the template. Updated.
Thanks!
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
25 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/metadata/net/histograms.xml
Insertions: 11, Deletions: 14.
The diff is too large to show. Please review the diff.
```
Change information
Add if origin is recently accessed in cache rejection status metrics
This CL marks navigation requests for resources whose main frame origin
has been seen before. Instead of checking the entire navigation history,
this CL introduces a new mechanism to track recently accessed main frame
origins.
A new feature flag, kUpdateIsMainFrameOriginRecentlyAccessed, is
introduced to control this functionality, and it is disabled by default.
When enabled, a new load flag, IS_MAIN_FRAME_ORIGIN_RECENTLY_ACCESSED,
and a new URLLoaderFactoryParams, is_main_frame_origin_recently_accessed
are used to plumb the information from //content/browser and
//services/network to //net/http.
When the feature is enabled, URLLoaderFactoryParamsHelper will maintain
an LRU cache of recently accessed main frame origins. On each main frame
navigation, the origin will be added to the cache. For each navigation
request, the browser will check if the main frame origin is in the cache
and set the new load flag accordingly.
The UMA metric HttpCache.EntryRejectionStatus is updated to be recorded
with suffixes MainFrameOriginRecentlyAccessed and
MainFrameOriginNotRecentlyAccessed to analyze the behavior differences
between requests based on whether their main frame origin has been
recently accessed.
See
https://docs.google.com/document/d/1rfWTtUjo5gfA8jFFw1SQzANK5hXxQf6QC4cFiGDyjns
for more background and discussion (googlers only)
- M chrome/browser/chrome_content_browser_client.cc
- M chrome/browser/chrome_content_browser_client.h
- M chrome/browser/chrome_content_browser_client_unittest.cc
- M content/browser/loader/navigation_url_loader_impl.cc
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/url_loader_factory_params_helper.cc
- M content/browser/url_loader_factory_params_helper.h
- A content/browser/url_loader_factory_params_helper_unittest.cc
- M content/public/browser/content_browser_client.cc
- M content/public/browser/content_browser_client.h
- M content/test/BUILD.gn
- M net/base/features.cc
- M net/base/features.h
- M net/base/load_flags_list.h
- M net/http/http_cache_transaction.cc
- M services/network/cors/cors_url_loader_factory.cc
- M services/network/cors/cors_url_loader_factory.h
- M services/network/public/mojom/network_context.mojom
- M tools/metrics/histograms/metadata/net/histograms.xml
Code-Review: +1 by Tsuyoshi Horo, +1 by Nidhi Jaju, +1 by Rakina Zata Amni, +1 by Nasko Oskov, +1 by Adam Rice
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |