796005 - Add telemetry for XUL cache being disabled

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Andrew McCreight [:mccr8]]

Some addons (like extension developer addons) set this pref ( nglayout.debug.disable_xul_cache ) without the user being aware of it, and it can cause severed performance degradation. It might be good to track how widespread of a problem this is.

I'm not sure what exactly we can do about this, aside from maybe alerting the user or something.

Comment 1

[Bindar Robert]

Hi, would this telemetry be still useful?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I think so. It would be good to know how often users suffer from significantly slower UI because
the xul cache is disabled-

Comment 3

[Bindar Robert]

So, the telemetry should only measure the xul_cache switches to disabled?

Comment 4

[Bindar Robert]

Attachment: Patch v1

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 734717 [details] [diff] [review]
Patch v1

Did you mean for Olli to review?

Comment 6

[Bindar Robert]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 734717 [details] [diff] [review]
> Patch v1
> 
> Did you mean for Olli to review?

Sorry, my bad, fixing now.

Comment 7

[Andrew McCreight [:mccr8]]

Its okay, I already switched it over to asking him for review instead. :)

Comment 8

[Bindar Robert]

Thanks a lot.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 734717 [details] [diff] [review]
Patch v1

+    if ( gDisableXULCache )
+        Telemetry::Accumulate(Telemetry::XUL_CACHE_DISABLED, true);
should be
    if (gDisableXULCache) {
        Telemetry::Accumulate(Telemetry::XUL_CACHE_DISABLED, true);
    }


But the kind for XUL_CACHE_DISABLED should be "flag", not "boolean" I believe.

Comment 10

[Bindar Robert]

Attachment: patch v2

Comment 11

[Bindar Robert]

Attachment: patch v2.1

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 735323 [details] [diff] [review]
patch v2


>     // Flush the cache, regardless
>     nsXULPrototypeCache* cache = nsXULPrototypeCache::GetInstance();
>     if (cache)
>         cache->Flush();
>+    if (gDisableXULCache)
>+        Telemetry::Accumulate(Telemetry::XUL_CACHE_DISABLED, true);
please add a new line before if
But looks like we update gDisableXULCache variable in two places, so you need to call Telemetry in both places, or better would be to add a helper method
which updates gDisableXULCache and calls telemetry


>+  "XUL_CACHE_DISABLED": {
>+    "kind": "flag",
>+    "description": "Xul cache was disabled"
It is XUL, not Xul

Comment 13

[Bindar Robert]

Olli Pettay:
  Thanks for review, I will implement it soon. It's ok if I make the method member of the nsXULPrototypeCache class?

Comment 14

[Bindar Robert]

Attachment: Patch v3

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 735942 [details] [diff] [review]
Patch v3

Make UpdategDisableXULCache return void, so you don't have to have return 0.

Comment 16

[Bindar Robert]

Attachment: Patch v3.1

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 736786 [details] [diff] [review]
Patch v3.1

I'll land this soon.

Comment 18

[Bindar Robert]

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 736786 [details] [diff] [review]
> Patch v3.1
> 
> I'll land this soon.

Thanks a lot.

Comment 19

[Andrew McCreight [:mccr8]]

Thanks for fixing this!

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76c3843d0c31

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76c3843d0c31

Comment 22

[Andrew McCreight [:mccr8]]

Huzzah.  This should be in tomorrow's Nightly.  In about a week, Olli or I should look to see if anything has shown up in telemetry.

Comment 23

[Andrew McCreight [:mccr8]]

It is still a bit early for data, but looking at metrics.mozilla.com it looks like 0.5% have the cache disabled.  I assume that means it was disabled at least once during whatever session the telemetry is for.