1617092 - nsXULPrototypeCache::HasData claims urls are cached when they aren't

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Dave Townsend [:mossop]]

Bug 1550108 introduced a logic error into the check for whether we need to cache a parsed XUL prototype meaning that effectively the cache has been disabled ever since. It looks like the other parts of the bug introduced enough performance improvements that this regression was overshadowed but talos results are suggesting that fixing this gives us around a 2%. ts_paint improvement on Windows and potentially almost as much on OSX: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1635081f3be7ac2050c2008897b0d94368df0bd9&newProject=try&newRevision=e9dcf9e9c21aa4b2b717cacafe1179618440f377&framework=1. Interestingly Linux seems much less affected.

Comment 1

[Dave Townsend [:mossop]]

Attachment: Bug 1617092: nsXULPrototypeCache::HasData claims urls are cached when they aren't. r=dthayer.

A logic error is causing nsXULPrototypeCache::HasData to almost always return
true meaning we aren't actually writing anything to the XUL prototype cache.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/458b5c5e0183
nsXULPrototypeCache::HasData claims urls are cached when they aren't. r=dthayer.

Comment 3

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/458b5c5e0183

Comment 4

[Pascal Chevrel:pascalc (PTO until April 26)]

Dave, is that something we could uplift to beta or do you consider that there is risk for regressions and we let it ride the 75 train?

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 9128038 [details]
Bug 1617092: nsXULPrototypeCache::HasData claims urls are cached when they aren't. r=dthayer.

Beta/Release Uplift Approval Request

• User impact if declined: The browser will start up slower than otherwise and bug 1603956 will be more of an issue.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): A simple logic error fix, should be no risk of regression really.
• String changes made/needed: None

Comment 6

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9128038 [details]
Bug 1617092: nsXULPrototypeCache::HasData claims urls are cached when they aren't. r=dthayer.

Low risk perf startup gain, uplift approved for 74.0b8, thanks.

Comment 7

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/8de39ece626e

Comment 8

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #25117 (as of Wed, 26 Feb 2020 08:19:07 GMT) ==

Improvements:

3% ts_paint_webext windows10-64-shippable opt e10s stylo 335.96 -> 326.83
2% ts_paint_webext windows10-64-shippable-qr opt e10s stylo 340.58 -> 332.33
2% ts_paint windows7-32-shippable opt e10s stylo 341.25 -> 333.67
1% ts_paint_webext windows10-64-shippable-qr opt e10s stylo 337.42 -> 333.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25117