9.59ms uninterruptible reflow at adjustHeight@chrome://global/content/bindings/autocomplete.xml:1239:32

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: geeknik, Assigned: Gijs)

Tracking

(Depends on 1 bug, Blocks 1 bug, {nightly-community, perf})

unspecified
mozilla63
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p2][fxsearch])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Here's the stack:

adjustHeight@chrome://global/content/bindings/autocomplete.xml:1239:32
_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml:1184:58
setTimeout handler*_invalidate@chrome://global/content/bindings/autocomplete.xml:1184:41
invalidate@chrome://global/content/bindings/autocomplete.xml:1162:11
showPopupWithResults@resource://gre/modules/AutoCompletePopup.jsm:179:7
receiveMessage@resource://gre/modules/AutoCompletePopup.jsm:245:9
Flags: qe-verify?
Priority: -- → P2
This must be due to one of the 4 getBoundingClientRect calls in this method: http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/toolkit/content/widgets/autocomplete.xml#1226
Component: Untriaged → Autocomplete
Product: Firefox → Toolkit
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
(Reporter)

Comment 2

2 years ago
Another stack: 

adjustHeight@chrome://global/content/bindings/autocomplete.xml:1239:32
_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml:1184:58
setTimeout handler*_invalidate@chrome://global/content/bindings/autocomplete.xml:1184:41
invalidate@chrome://global/content/bindings/autocomplete.xml:1162:11
notifyResults@resource://gre/components/UnifiedComplete.js:2114:5
_addMatch@resource://gre/components/UnifiedComplete.js:1736:5
_addSearchEngineMatch@resource://gre/components/UnifiedComplete.js:1502:5
_matchCurrentSearchEngine@resource://gre/components/UnifiedComplete.js:1467:5
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
TaskImpl_run@resource://gre/modules/Task.jsm:324:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
startSearch@resource://gre/components/UnifiedComplete.js:2237:5
onInput@chrome://browser/content/urlbarBindings.xml:1125:17
onxblinput@chrome://global/content/bindings/autocomplete.xml:649:9
Flags: qe-verify? → qe-verify-
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Priority: P3 → P4
Keywords: perf
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]
Priority: P4 → P3

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf]
Now that bug 1434376 is fixed, I think someone can probably take another crack at this. I'm queueing for the Firefox Performance team.
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p2]

Updated

11 months ago
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p2] → [ohnoreflow][qf:f64][qf:p1][fxperf:p2]

Updated

11 months ago
Whiteboard: [ohnoreflow][qf:f64][qf:p1][fxperf:p2] → [ohnoreflow][qf:p1:f64][fxperf:p2]
Hey Gijs, do you have a sense of whether or not it's worth spot-fixing this while the AwesomeBar dropdown refactor is still ~6 months out from MVP?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 5

9 months ago
(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #4)
> Hey Gijs, do you have a sense of whether or not it's worth spot-fixing this
> while the AwesomeBar dropdown refactor is still ~6 months out from MVP?

I don't remember off-hand. I'll look at this again tomorrow or Friday. IIRC the (horizontal row) overflow reflows are more common/frequent than this one so I dunno how significant the impact would be, but I guess it won't hurt to take another look to be sure.
yes, the overflow flushes are more common, but also harder to get rid of.
(Assignee)

Comment 7

9 months ago
After some discussion with Marco, I think I have something that works, which I'll attach, though I'll be curious if other people notice anything that behaves worse post-patch than pre-patch in terms of flickering. I don't see much difference either way, which I somewhat expected because I basically expect the cost of the reflow to now shift to the calls to onOverflow().
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 8

9 months ago
This goes back to relying on rows being the same height.
Places where we replace the popup will likely not use the richlistbox,
and we no longer have code that changes the height or exceeds the maximum
number of visible children with a scrollbar, so we should be OK.

To determine the padding on the richlistbox and the height of the initial
row, I've used a promiseDocumentFlushed callback. It's possible this causes
flicker the first time the popup opens. I can't see any, but it's quite
possible I'm missing something.
(Assignee)

Comment 10

9 months ago
This seems to fail a number of autocomplete (non-browser) chrome mochitests, because window.promiseDocumentFlushed() is not available. I'll look at this tomorrow - there are other test-failures, too, but off-hand they seem like they should be easier to address.
Priority: P3 → P1
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p2] → [ohnoreflow][qf:p1:f64][fxperf:p2][fxsearch]
(Assignee)

Updated

9 months ago
See Also: → 1356532
(Assignee)

Comment 11

9 months ago
Per discussion with mconley and mak on IRC, I'm adjusting only the URL bar here. I'll file a follow-up bug to consider if we can make improvements to the general in-content autocomplete even though formautofill is "special", and though window.documentPromiseFlushed() is not available on non-chrome documents (so the autocomplete mochitest-chrome tests don't like it...).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ba1b74793bd00cd046385fcd7c02a157a69bfc
(Assignee)

Updated

9 months ago
Depends on: 1477235
(Assignee)

Updated

9 months ago
No longer depends on: 1477235
See Also: → 1477235
(Assignee)

Comment 12

9 months ago
Removed animate support from toolkit per mak's comment. New trypush:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9997fbfdde9732345dcb950aeb3fc29bcc90e18
Comment on attachment 8993403 [details]
Bug 1358443 - remove reflows from adjustHeight itself, r?mak,florian

Marco Bonardo [::mak] has approved the revision.

https://phabricator.services.mozilla.com/D2242
Attachment #8993403 - Flags: review+
Comment on attachment 8993403 [details]
Bug 1358443 - remove reflows from adjustHeight itself, r?mak,florian

Florian Quèze [:florian] has approved the revision.

https://phabricator.services.mozilla.com/D2242
Attachment #8993403 - Flags: review+

Comment 15

9 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0ce819c0a081
remove reflows from adjustHeight itself, r=mak,florian

Comment 16

9 months ago
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c0ccf6a9bfc
Backed out changeset 0ce819c0a081 for browser chromes failures on browser_urlbar_search_no_speculative_connect_with_client_cert.
Backed out for browser chromes failures on browser_urlbar_search_no_speculative_connect_with_client_cert

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=0ce819c0a08153a533805a0c347f9cd298bf5b9a&tochange=6c0ccf6a9bfc3749de76ee24e3bcf4f4efa2f54f&filter-searchStr=browser%20chrome&selectedJob=190070311

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190070311&repo=autoland&lineNumber=3062

Backout link: https://hg.mozilla.org/integration/autoland/rev/6c0ccf6a9bfc3749de76ee24e3bcf4f4efa2f54f

09:36:44     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | The node is there. - true == true - 
09:36:44     INFO - Buffered messages finished
09:36:44     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | The second item is selected - Got 0, expected 1
09:36:44     INFO - Stack trace:
09:36:44     INFO - chrome://mochikit/content/browser-test.js:test_is:1305
09:36:44     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:popup_mousedown_no_client_cert_dialog_until_navigate_test:165
09:36:44     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1103
09:36:44     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1094
09:36:44     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:996
09:36:44     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
09:36:44     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | A valid string reason is expected - 
09:36:44     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | Reason cannot be empty - 
09:36:44     INFO - Console message: [JavaScript Error: "getScreenshot(https://localhost:50879/) failed: TypeError: NetworkError when attempting to fetch resource." {file: "resource://activity-stream/lib/Screenshots.jsm" line: 47}]
09:36:44     INFO - getScreenshotForURL@resource://activity-stream/lib/Screenshots.jsm:47:7
09:36:44     INFO - async*maybeCacheScreenshot@resource://activity-stream/lib/Screenshots.jsm:101:32
09:36:44     INFO - async*_fetchScreenshot@resource://activity-stream/lib/TopSitesFeed.jsm:231:11
09:36:44     INFO - async*_fetchIcon@resource://activity-stream/lib/TopSitesFeed.jsm:219:11
09:36:44     INFO - async*getLinksWithDefaults@resource://activity-stream/lib/TopSitesFeed.jsm:158:11
09:36:44     INFO - async*refresh@resource://activity-stream/lib/TopSitesFeed.jsm:180:25
09:36:44     INFO - async*onAction@resource://activity-stream/lib/TopSitesFeed.jsm:411:9
09:36:44     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
09:36:44     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
09:36:44     INFO - customDispatch/this.placesChangedTimer<@resource://activity-stream/lib/PlacesFeed.jsm:203:11
09:36:44     INFO - 
09:36:44     INFO - Console message: [JavaScript Error: "NetworkError when attempting to fetch resource."]
09:36:44     INFO - get@resource://services-settings/remote-settings.js:309:9
09:36:44     INFO - async*getSite@resource://activity-stream/lib/FaviconFeed.jsm:158:25
09:36:44     INFO - async*fetchIcon@resource://activity-stream/lib/FaviconFeed.jsm:132:24
09:36:44     INFO - async*onAction@resource://activity-stream/lib/FaviconFeed.jsm:182:9
09:36:44     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
09:36:44     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
09:36:44     INFO - _requestRichIcon@resource://activity-stream/lib/TopSitesFeed.jsm:252:5
09:36:44     INFO - _fetchIcon@resource://activity-stream/lib/TopSitesFeed.jsm:216:5
09:36:44     INFO - async*getLinksWithDefaults@resource://activity-stream/lib/TopSitesFeed.jsm:158:11
09:36:44     INFO - async*refresh@resource://activity-stream/lib/TopSitesFeed.jsm:180:25
09:36:44     INFO - async*onAction@resource://activity-stream/lib/TopSitesFeed.jsm:411:9
09:36:44     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
09:36:44     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
09:36:44     INFO - customDispatch/this.placesChangedTimer<@resource://activity-stream/lib/PlacesFeed.jsm:203:11
09:36:44     INFO - 
09:37:28     INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(gijskruitbosch+bugs)

Comment 19

9 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c334bde84123
remove reflows from adjustHeight itself, r=mak,florian

Comment 20

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c334bde84123
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

a month ago
Depends on: 1533433
You need to log in before you can comment on or make changes to this bug.