Re: how to handle logging reasons for evicting from bfcache?

248 views
Skip to first unread message

Fergal Daly

unread,
Jun 23, 2021, 3:50:48 AM6/23/21
to Alex Moshchuk, embedd...@chromium.org, Kinuko Yasuda, Matt Falkenhagen, Hajime Hoshi, content-owners, bfcache-dev
[+embedder-dev]

Edge use UKM/UMA (I assume others do too).

TL;DR We have an API where the embedder cause a page to be evicted from BFCache. It currently doesn't take a reason. We want to supply a reason and while all of the chromium reasons can live in 1 enum, embedders will need to be able to supply their own reasons. There are 2 options on the table

1 The API takes a struct of approximately `{Source source, int reason}` where `source` is one of chromium/embedder and `reason` maps to an enum and what gets logged is `source << 16 + reason`
2 The API takes an just `Reason reason`, where `Reason` is an enum and chromium will never use any values above 1<<16, embedders can patch the enum with extra values

Any preferences?

F

On Wed, 9 Jun 2021 at 11:53, Fergal Daly <fer...@google.com> wrote:
On Wed, 9 Jun 2021 at 02:32, Alex Moshchuk <ale...@chromium.org> wrote:
On Tue, Jun 8, 2021 at 7:47 AM Kinuko Yasuda <kin...@chromium.org> wrote:
I took a(nother) look at the CL but wasn't able to clearly see what we wanted to distinguish either.  I might be missing some details but I feel starting with a single enum can work at least for the goal we want to achieve.

If we want to make it explicit that other implementers can add their enums, we can probably have something like kExternalReasonStartId = 1 << 16 within DisallowActivationReasonType, with a comment that the enum values beyond that can be used by different implementations?  (And we won't use or record the values in the chromium repository)

On Tue, Jun 8, 2021 at 8:14 PM Matt Falkenhagen <fal...@chromium.org> wrote:
I think it'd make sense to use one kPostMessage.

There doesn't seem to be any other opinions here yet. If there are still no opinions by tomorrow, my vote would be to just put a single enum in //components/bfcache_metrics (or //components/mparch_metrics) and use it for everything in the repo until we discover there is a real need to do something differently.

This sounds simplest to me, so +1 to starting there, and +1 to mparch_metrics for making it more reusable (e.g. for prerender cancel reasons).  I'm not sure we need to care that much about non-chromium code for UMA, and if different implementations did want to extend the enum, they could use the approach suggested by Kinuko.

I've reached out to someone on Edge. I would be very surprised if they didn't use UMA/UKM since maintaining an alternative sounds difficult to maintain. I will let you know what I hear back.

The Source enum essentially formalizes the << 16 idea and avoids  It seems like we shouldn't force embedder to patch our code if we can easily avoid it,

F
 

FWIW, we have some precedent for the namespacing approach as well, e.g. with BrowserDataRemover data types in content/public (where content defines its last value, which is then used by each embedder (currently chrome and weblayer) to start off their own enum values), though those don't need to be tied to UMA.  But I agree that having components involved kind of complicates this.


2021年6月8日(火) 17:04 Hajime Hoshi <hajim...@chromium.org>:
I understand that //component functions can be invoked from //content and vice versa, but I'm not sure we really want to distinguish the 'root' sources of disallow-activation calls, or not. For example, in my current CL, kPostMessage is defined in components/content_public (which might be renamed to e.g, bfcache_metrics later). As kPostMessage is a general thing, would it make sense to distinguish whether the root source is //content or the outside here? If so, do we have to have two kPostMessage? Sorry if I am missing something...

On Mon, Jun 7, 2021 at 6:50 PM Matt Falkenhagen <fal...@chromium.org> wrote:
Thanks Fergal for sending this. This is somewhat related to the recent DEPS discussion and a result of having some //components depend on //content and some //components depended on by //content.

Some thoughts: I wonder how much we need to care about non-chromium repository code with respect to UMA. I wonder if a possible solution could be just to have a //component/bfcache_metrics, which has the enum BackForwardCacheEvictionReason, etc, and all code in the chromium repository can depend on that, and we don't distinguish between //content and embedder code. If an external embedder wants to use UMA and add on to the enum, they would need to augment the enum in their own way, e.g., by patching the //component/bfcache_metrics file and start counting at a high number. But I'm not sure if that's acceptable.

Another solution could be like #3 above, except all chromium repository code uses //component/bfcache_metrics and a non-chromium embedder uses a namespace like kExternal.
 

2021年6月7日(月) 11:12 'Fergal Daly' via content-owners <content...@chromium.org>:
https://crrev.com/c/2900198 is attempting to add a reason parameter to `RenderFrameHost::IsInactiveAndDisallowActivation`. Calling this causes the page to be evicted from BFCache and if the user navigates back to it we will log this reason.

Where this gets tricky is that this is a public API, so we cannot have a single enum with all possible reasons. Edge etc need to be able to pass their own reasons and should be able to do so without patching content.

The solution to this is namespacing. The API does not involve an enum for reasons, there is just an int for the reason and an enum for the source of the reason. The caller knows the meaning of the int. The question is, how many sources should we define?

Inside our repo, this API is called from content and from components. I thought of components as being part of Chrome and so the initial plan was to distinguish Content vs Embedder with everything outside of content attaching source=kEmbedder but actually content can call into a component which could then call this API.

So I think we have a few choices for the source enum:
1 kContent, kComponent, kEmbedder (which would include non-component chrome callers)
2 kContent, kComponent, kChrome, kEmbedder (3rd parties)
3 kContentOrComponent, kEmbedder

#3 clearly breaks the content/non-content boundary so maybe it's objectionable
#2 also does so by making Chrome a special embedder

#1 makes life a bit more awkward but I think it's philosophically correct. 

We already hit this problem with https://crrev.com/c/2695013 but I think we solved it incorrectly since components are get kEmbedder, the failure mode here is to imagine a component that is called by content, so it will use kEmbedder + an int. Now imagine that happening inside Edge which has its own meanings for these ints. Not likely to happen in practice but not impossible.

Thanks,

F

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAAozHLngkWSNn6YJgfW-tZ4ADc2z6jmO2tZaO1-%2ByS-fGP_6zw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "bfcache-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bfcache-dev...@chromium.org.
To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/bfcache-dev/CAJ_xCimGJtxAyJzsWwP-ruGcCy3zX0UoUC5HArXdErR2FaTiyw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "bfcache-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bfcache-dev...@chromium.org.
To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/bfcache-dev/CAJ_xCimEOfdkfOKPc_HcEaU_z6sFfMhY4ex2q5YmR_qV7b5QMg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAMWgRNa9G5swEdW4yYJA2LSgnFvUR%3D3EcTMyyyXNf%3DG-mpFbog%40mail.gmail.com.

Fergal Daly

unread,
Jun 27, 2021, 8:32:04 PM6/27/21
to Alex Moshchuk, embedd...@chromium.org, Kinuko Yasuda, Matt Falkenhagen, Hajime Hoshi, content-owners, bfcache-dev, Arvind Murching

Fergal Daly

unread,
Jun 29, 2021, 3:55:16 AM6/29/21
to Alex Moshchuk, embedd...@chromium.org, Kinuko Yasuda, Matt Falkenhagen, Hajime Hoshi, content-owners, bfcache-dev, Arvind Murching
I'd like to get this change landed, so does anyone object to

IsInactiveAndDisallowActivation(int64_t reason)

and < 2^16 is reserved for internal use?

Pros
- simple
- embedders don't have to patch

Cons
- not strongly typed but
 - all we are doing is passing it through to UKM/UMA
 - we would always call it through a wrapper which uses the enum type

F


On Tue, 29 Jun 2021 at 16:52, Fergal Daly <fer...@google.com> wrote:
I'd like to get this change landed, so does anyone object to

IsInactiveAndDisallowActivation(int64_t reason)

and < 2^16 is reserved for internal use?

Pros
- simple
- embedders don't have to patch

Cons
- not strongly typed but
 - all we are doing is passing it through to UKM/UMA
 - we would always call it through a wrapper which uses the enum type

F

Erik Anderson

unread,
Jun 29, 2021, 12:03:16 PM6/29/21
to fer...@chromium.org, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, fal...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching

Hi Fergal,

 

At a high level, either of your proposed approaches makes sense. I really appreciate you considering potential impacts to other embedders!

 

I’ve been informed that this general pattern can be a problem if it’s logged as a plan enum histogram since the implementation assumes the values are contiguous. Since the embedder would be logging 2^16 values, the histogram code can potentially allocate for all of the intermediate values in the gap. Switching to a sparse histogram is what folks have done when dealing with having embedder-only values.

 

This info is second hand from other Edge folks that ran into it, so if any of that sounds off please push back.

 

Assuming that’s true, it might be worth considering exploring if we can ensure any such interactions with embedder-defined values will be caught in Chromium (these are not mutually exclusive):

  1. Have a test assertion that the histogram is sparse and include a comment about why.
  2. Have a Chromium value at the beginning of the range that often gets used to ensure that if a pathological case gets exercise that it could be caught.
  3. Investigate if there should be a special histogram type that can be annotated as “this histogram has two disjoint beginning values; starting at 0 and 2^16”.

 

If those optionhs are unappealing, you could also consider having embedder-reserved ranges toward the beginning of the enum that Chromium will never use. If an embedder runs out of values, they could open a CL in Chromium to add a new reserved range at which point the Chromium-defined+embedder-defined values may become interleaved.

 

Thanks,

Erik

--
You received this message because you are subscribed to the Google Groups "Chromium Embedders" group.
To unsubscribe from this group and stop receiving emails from it, send an email to embedder-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/embedder-dev/CAAozHLm2keSPoZroXkM7So_DkvgkpAWDH_d8A4fWJq_2QyKRZA%40mail.gmail.com.

Fergal Daly

unread,
Jul 8, 2021, 9:23:52 PM7/8/21
to Erik Anderson, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, fal...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching
Thanks Erik.

On Wed, 30 Jun 2021 at 01:03, Erik Anderson <Erik.A...@microsoft.com> wrote:

Hi Fergal,

 

At a high level, either of your proposed approaches makes sense. I really appreciate you considering potential impacts to other embedders!

 

I’ve been informed that this general pattern can be a problem if it’s logged as a plan enum histogram since the implementation assumes the values are contiguous. Since the embedder would be logging 2^16 values, the histogram code can potentially allocate for all of the intermediate values in the gap. Switching to a sparse histogram is what folks have done when dealing with having embedder-only values.

This is already a sparse histogram in the CL.
 

This info is second hand from other Edge folks that ran into it, so if any of that sounds off please push back.

 

Assuming that’s true, it might be worth considering exploring if we can ensure any such interactions with embedder-defined values will be caught in Chromium (these are not mutually exclusive):

  1. Have a test assertion that the histogram is sparse and include a comment about why.
  2. Have a Chromium value at the beginning of the range that often gets used to ensure that if a pathological case gets exercise that it could be caught.
  3. Investigate if there should be a special histogram type that can be annotated as “this histogram has two disjoint beginning values; starting at 0 and 2^16”.

Perhaps we can get a type into the metrics code like embedder-extendable-enum-int and add an overload so that using that with sparse histogram is easy but would require a cast to use it with a non-sparse histogram. That would make it hard to unconsciously change it to a non-sparse and it would leave an explicit example for future APIs.

F

Fergal Daly

unread,
Jul 8, 2021, 9:27:22 PM7/8/21
to Erik Anderson, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, fal...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching
Thanks Erik.

On Wed, 30 Jun 2021 at 01:03, Erik Anderson <Erik.A...@microsoft.com> wrote:

Hi Fergal,

 

At a high level, either of your proposed approaches makes sense. I really appreciate you considering potential impacts to other embedders!

 

I’ve been informed that this general pattern can be a problem if it’s logged as a plan enum histogram since the implementation assumes the values are contiguous. Since the embedder would be logging 2^16 values, the histogram code can potentially allocate for all of the intermediate values in the gap. Switching to a sparse histogram is what folks have done when dealing with having embedder-only values.

This is already a sparse histogram in the CL.
 

 

This info is second hand from other Edge folks that ran into it, so if any of that sounds off please push back.

 

Assuming that’s true, it might be worth considering exploring if we can ensure any such interactions with embedder-defined values will be caught in Chromium (these are not mutually exclusive):

  1. Have a test assertion that the histogram is sparse and include a comment about why.
  2. Have a Chromium value at the beginning of the range that often gets used to ensure that if a pathological case gets exercise that it could be caught.
  3. Investigate if there should be a special histogram type that can be annotated as “this histogram has two disjoint beginning values; starting at 0 and 2^16”.

Perhaps we can get a type into the metrics code like embedder-extendable-enum-int and add an overload so that using that with sparse histogram is easy but would require a cast to use it with a non-sparse histogram. That would make it hard to unconsciously change it to a non-sparse and it would leave an explicit example for future APIs.

F
 

 

If those optionhs are unappealing, you could also consider having embedder-reserved ranges toward the beginning of the enum that Chromium will never use. If an embedder runs out of values, they could open a CL in Chromium to add a new reserved range at which point the Chromium-defined+embedder-defined values may become interleaved.

Fergal Daly

unread,
Jul 16, 2021, 2:45:00 AM7/16/21
to Erik Anderson, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, fal...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching
https://crrev.com/c/2900198 has been updated now and is ready for review again. I think it makes it pretty hard for embedders to accidentally use values in the chrome range without hurting usability for chrome or embedders,

F


Matt Falkenhagen

unread,
Jul 19, 2021, 12:07:55 AM7/19/21
to Fergal Daly, Erik Anderson, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching
Note for content-owners: https://crrev.com/c/2900198 is adding a dependency from //content to //components. I think this approach makes sense as discussed in this thread.

2021年7月16日(金) 15:45 Fergal Daly <fer...@google.com>:

Fergal Daly

unread,
Jul 19, 2021, 12:16:10 AM7/19/21
to Matt Falkenhagen, Erik Anderson, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching
On Mon, 19 Jul 2021 at 13:07, Matt Falkenhagen <fal...@chromium.org> wrote:
Note for content-owners: https://crrev.com/c/2900198 is adding a dependency from //content to //components. I think this approach makes sense as discussed in this thread.

It also adds a small stand-alone build target under content/public/browser (to define the struct that is used to carry reasons in the API). That's the part I expect to be more contentious although I couldn't find a way to avoid this due to the circular deps between content/public/browser and content/browser. The only alternative that worked is to put this in a stand-alone component which seems even worse.

I'm curious if there's a way to break this circular dep. It seems to be there because the public API includes some impls that call into non-public code but there should never be a need for content/browser to use these functions. Could we break the circular dep by having a headers-only target in content/public/browser that content/browser could depend on?

F

Fergal Daly

unread,
Aug 17, 2021, 5:58:51 AM8/17/21
to Matt Falkenhagen, John Abd-El-Malek, Erik Anderson, ale...@chromium.org, embedd...@chromium.org, kin...@chromium.org, hajim...@chromium.org, content...@chromium.org, bfcac...@chromium.org, Arvind Murching
Another update on this. jam@ started reviewing my CL and then we had a GVC about it.

jam@'s preference is for the enum to live in content/public and we comment it as not to be used by embedders. This avoids adding a component

I'm OK with this but it seems a bit odd to make it public when it's intended to not be used, so I said I would run it past content-owners first.

Also, I figured out that I was misunderstanding `allow_circular_includes_from` and it's possible to do the component version of this without adding a new build target under content/public/browser,

F
Reply all
Reply to author
Forward
0 new messages