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.

Hi, would this telemetry be still useful?

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

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

Comment on attachment 734717 [details] [diff] [review] Patch v1 Did you mean for Olli to review?

(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.

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

Thanks a lot.

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 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

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

Comment on attachment 735942 [details] [diff] [review] Patch v3 Make UpdategDisableXULCache return void, so you don't have to have return 0.

Comment on attachment 736786 [details] [diff] [review] Patch v3.1 I'll land this soon.

(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.

Thanks for fixing this!

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.

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.