--
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.
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.
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.
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.
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.
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):
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.
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):
- Have a test assertion that the histogram is sparse and include a comment about why.
- 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.
- 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”.
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):
- Have a test assertion that the histogram is sparse and include a comment about why.
- 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.
- 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.
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):
- Have a test assertion that the histogram is sparse and include a comment about why.
- 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.
- 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.
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.