User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170814072924 Steps to reproduce: I have several thouthsend of bookmarks. When I turn on the bookmarks toolbar, FF uses a lot of memory. When it is disabled, FF uses about 350 MB Actual results: Firefox use extra memory, more than in FF54

Can you produce 2 memory reports, one before and one after enabling the bookmarks toolbar? ( https://developer.mozilla.org/en-US/docs/Mozilla/Performance/about:memory#How_to_generate_memory_reports ) You can attach them via https://bugzilla.mozilla.org/attachment.cgi?bugid=1392081&action=enter .

A lot of bookmarks directly on bookmarks toolbar (without folders)

A lot (most?) of the extra memory is being consumed by favicons. I wonder if we can determine that they're off-screen and avoid setting the image source in that case (or display: none the items entirely or something). Marco? Maybe similar to bug 1380999?

I'd expect the images offloading to be done by something at the platform level when they are not visible, but we use style.visibility instead of collapsed, because we need the bounding rect to calculate overflow, and collapsed wouldn't allow a proper measurement afaict. So maybe that's where the problem lies. I wonder if the same issue exists for the menu views or not. IIRC once we load a bookmarks menu popup we just keep it populated, to allow opening the menus faster later. It's possible the menu frame discards the images by itself when it's not shown. If the problem is more general (affecting menus), we may need a more general solution. Bug 1380999 is partially due to querying and populating the toolbar synchronously. The favicons are loaded asynchronosuly and shouldn't block the toolbar loading, even if the IO could slowdown the whole process a bit. Actually, we build ALL the nodes on the toolbar, and then just hide the overflowed ones, that means the more elements exists, the more expensive the toolbar is. To calculate overflow we likely only need a subset of the nodes. I wonder if we could completely stop building nodes when we know it's impossible for those nodes to ever become visible even when enlarging the window at the maximum (surely the window can be larger than the screen, but at a certain point it becomes just unusable). Building 1000 buttons when we know in the worst case we may show a hundred, doesn't make a lot of sense. During the PT__insertNewItem call we could probably check the boundingClientRect, and when we are over double the screen size just give up and stop building nodes... May this work? It will just break cases where the window is larger than twice the screen size. I think it's acceptable. For the image attributes we could, as you suggested, not set the "image" attribute until a node is known to be visible. The magic happens here http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/browser/components/places/content/browserPlacesViews.js#1242 And the image attribute is set here: http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/browser/components/places/content/browserPlacesViews.js#1066 We could avoid setting the image attribute in the insert code, and later, when we set visibility to "visible", set it to child._placesNode.icon (if it's not null). I can surely make a patch for this and sounds like it would be a nice memory and perf win for people using the toolbar.

The test failures were due to previous tests collapsing and uncollapsing the toolbar. That way, instead of completely rebuilding it, the test was just adding nodes, we don't go through the new "saving" mode that way, since it's intended for the invalidate looping. Another problem exposed by that is the fact we don't destroy the view when the toolbar is collapsed, but we should, since there's no point in keeping it around taking up resources. So I added a fifth part that uninits the view when the toolbar is collapsed. I also removed the init() call in the code collapsing the toolbars, since the helper can listen to the toolbar change event by itself. I'm starting up a new try.

Comment on attachment 8902249 [details] Bug 1392081 - Use a document fragment to populate toolbar and menu Places views. https://reviewboard.mozilla.org/r/173778/#review180438 ::: browser/components/places/content/browserPlacesViews.js:1258 (Diff revision 1) > nodeInserted: > function PT_nodeInserted(aParentPlacesNode, aPlacesNode, aIndex) { > let parentElt = this._getDOMNodeForPlacesNode(aParentPlacesNode); > if (parentElt == this._rootElt) { > let children = this._rootElt.childNodes; > - this._insertNewItem(aPlacesNode, > + this._insertNewItem(aPlacesNode, this._rootElt, There might (not sure I understand the places code right here) be a further opportunity for optimization here for when users move/copy/paste/insert multiple nodes, which I think will notify for a batch start, then insert stuff, then batch end, for which we could potentially also use a fragment, but I don't know if that's worth worrying about - certainly not as part of this patchset.

Comment on attachment 8902250 [details] Bug 1392081 - Set the image attribute on bookmarks toolbar buttons only when they are visible. https://reviewboard.mozilla.org/r/173780/#review180442 This looks sane, but I have a question about the nodeInserted thing, so holding off on r+ for now, because it feels like I'm missing something here. :-) ::: browser/components/places/content/browserPlacesViews.js:1226 (Diff revision 1) > // Update the chevron on a timer. This will avoid repeated work when > // lot of changes happen in a small timeframe. > if (this._updateChevronTimer) > this._updateChevronTimer.cancel(); > > this._updateChevronTimer = this._setTimer(100); FWIW, it may be a good idea to use an idle-related timer here, Florian and/or the rest of the perf team will have more detailed/better suggestions than me (there may even be a bug on file already, not sure off-hand). ::: browser/components/places/content/browserPlacesViews.js:1266 (Diff revision 1) > let parentElt = this._getDOMNodeForPlacesNode(aParentPlacesNode); > if (parentElt == this._rootElt) { > let children = this._rootElt.childNodes; > - this._insertNewItem(aPlacesNode, this._rootElt, > + let button = this._insertNewItem(aPlacesNode, this._rootElt, > aIndex < children.length ? children[aIndex] : null); > + let icon = aPlacesNode.icon; Isn't it possible for this to insert a node in an overflowed position? What happens in that case? It looks like we update the visibility and remove the icon attribute again once the chevron update timer fires. It seems like we could do something like this: ```js let prevSiblingOverflowed = aIndex > 0 && children[aIndex - 1].style.visibility == "hidden"; if (prevSiblingOverflowed) { button.style.visibility = "hidden"; } else { // set icon code, then: this.updateChevron(); } ``` right? This would avoid triggering the chevron update code repeatedly for appending items at the end, where we already know they're overflowed, and also avoids requesting the icon if we know it won't be necessary. But maybe I'm misunderstanding how nodeInserted normally gets invoked here? If so, assuming I'm not being *really* dense, just back from PTO, maybe add a comment explaining why we *always* have to update the chevron here.

Comment on attachment 8902251 [details] Bug 1392081 - Only build a subset of the buttons that may become visible on the bookmarks toolbar. https://reviewboard.mozilla.org/r/173782/#review180446 ::: browser/components/places/content/browserPlacesViews.js:1058 (Diff revision 1) > + if (!this._resultNode || !this._rootElt) > + return; > + // We assume a button with just the icon will be more or less a square, > + // then compensate the measurement error by considering a larger screen > + // width. Moreover the window could be bigger than the screen. > + let size = button.clientHeight; I don't really know what the plan is for the bookmarks toolbar and the toolbar density options, but we should probably make sure that if/when we change something in bug 1388794 or one of the other deps of bug 1388676 that this is still more or less right. ::: browser/components/places/content/browserPlacesViews.js:1059 (Diff revision 1) > + return; > + // We assume a button with just the icon will be more or less a square, > + // then compensate the measurement error by considering a larger screen > + // width. Moreover the window could be bigger than the screen. > + let size = button.clientHeight; > + let limit = Math.min(cc, parseInt((window.screen.width * 1.5) / size)); Should we cache the screen width? Also, how sure are we that window.screen.width is the right screen in multiscreen setups? ::: browser/components/places/content/browserPlacesViews.js:1256 (Diff revision 1) > }, > > _updateChevronTimerCallback: function PT__updateChevronTimerCallback() { > let scrollRect = this._rootElt.getBoundingClientRect(); > let childOverflowed = false; > - for (let i = 0; i < this._rootElt.childNodes.length; i++) { > + for (let child of this._rootElt.childNodes) { My understanding is that for...of loops are normally slower than the C-style ones, but I also don't know if the childNodes[i] lookup is fast or not, which might counteract that. Do you have more info here, for my reference? :-)

Comment on attachment 8902252 [details] Bug 1392081 - Mochitest browser test for bookmarks toolbar overflow. https://reviewboard.mozilla.org/r/173784/#review180450 ::: browser/components/places/tests/browser/browser_toolbar_overflow.js:89 (Diff revision 1) > + Assert.ok(gToolbarContent.childNodes.length >= originalLen, > + "Number of built nodes should not reduce"); Should we also check the number of nodes hasn't increased here? ::: browser/components/places/tests/browser/browser_toolbar_overflow.js:123 (Diff revision 1) > + Assert.ok(gToolbarContent.childNodes[gToolbarContent.childNodes.length - 1], > + "A new node should be built"); This seems a bit of a strange check that will always be true unless there are no child nodes in the toolbar content (ie for array foo, you're checking foo[foo.length - 1] which basically devolves to "does this array have a non-0 length"). Can we make it more meaningful, maybe by checking that there was a different node here before?

Comment on attachment 8902439 [details] Bug 1392081 - Reset the Places toolbar view when the toolbar is collapsed. https://reviewboard.mozilla.org/r/174028/#review180452

Comment on attachment 8902250 [details] Bug 1392081 - Set the image attribute on bookmarks toolbar buttons only when they are visible. https://reviewboard.mozilla.org/r/173780/#review180442 > FWIW, it may be a good idea to use an idle-related timer here, Florian and/or the rest of the perf team will have more detailed/better suggestions than me (there may even be a bug on file already, not sure off-hand). There are 2 clashing factors here: 1. we want to avoid running too early, because we want to avoid repeated calls in a short time. So we can't just use idleDispatchToMainThread, since it could also run immediately, we don't know. 2. we don't want this to take too long, because we're reacting to a UI action from the user, a long delay would be perceived as worse performance. In any case I should keep the timer, as things stand, but I'll see if I can figure the best callback to query the scrollbox and set style.visibility, without causing reflows/restyles. > Isn't it possible for this to insert a node in an overflowed position? What happens in that case? It looks like we update the visibility and remove the icon attribute again once the chevron update timer fires. > > It seems like we could do something like this: > > ```js > let prevSiblingOverflowed = aIndex > 0 && children[aIndex - 1].style.visibility == "hidden"; > if (prevSiblingOverflowed) { > button.style.visibility = "hidden"; > } else { > // set icon code, then: > this.updateChevron(); > } > ``` > > right? This would avoid triggering the chevron update code repeatedly for appending items at the end, where we already know they're overflowed, and also avoids requesting the icon if we know it won't be necessary. But maybe I'm misunderstanding how nodeInserted normally gets invoked here? If so, assuming I'm not being *really* dense, just back from PTO, maybe add a comment explaining why we *always* have to update the chevron here. it's a good suggestion, I will add it!

Comment on attachment 8902251 [details] Bug 1392081 - Only build a subset of the buttons that may become visible on the bookmarks toolbar. https://reviewboard.mozilla.org/r/173782/#review180446 > I don't really know what the plan is for the bookmarks toolbar and the toolbar density options, but we should probably make sure that if/when we change something in bug 1388794 or one of the other deps of bug 1388676 that this is still more or less right. This code is mostly guessing a size. Without doing reflushes for every insert (or every N inserts), there's no way to calculate it properly. It's also very pessimistic since it's considering all buttons as not having a label and a screen size larger than the real one. I don't think those changes alone may create problems to this. In any case, this is a first step forward, I don't doubt with more refactoring we could stop every N inserts, wait a flush, calculate and move on, it's just a bit more risky/complex. > Should we cache the screen width? Also, how sure are we that window.screen.width is the right screen in multiscreen setups? I considered caching it, but we are accessing it only once per rebuild (it's out of the loop) in rAF, so it should be cheap. I can't tell much about the multiscreen setup, but I assume window.screen is the screen where the window exists at rebuild time. Surely the user could move the window to a different screen, but then we are again in a very edge case where the user has no labels on the toolbar, has a multiscreen setup, and the target screen is more than 1.5 times the original one. Not sure it's worth it. If we find it being an issue, in the future we can detect some specific event and force a rebuild. > My understanding is that for...of loops are normally slower than the C-style ones, but I also don't know if the childNodes[i] lookup is fast or not, which might counteract that. Do you have more info here, for my reference? :-) I think the reference ATM is https://bugzilla.mozilla.org/show_bug.cgi?id=1355874#c8 and yes, for...of is slower, though it's far more readable and in practice here each step time is completely obliterating any micro-optimization to the looping code. I think it really matters when the looping body is trivial and quick, for this case it's unlikely to make any measurable difference.

Comment on attachment 8902252 [details] Bug 1392081 - Mochitest browser test for bookmarks toolbar overflow. https://reviewboard.mozilla.org/r/173784/#review180450 > Should we also check the number of nodes hasn't increased here? Yes, I skipped that to keep the code simpler, but in the end it wasn't a big deal to handle it. > This seems a bit of a strange check that will always be true unless there are no child nodes in the toolbar content (ie for array foo, you're checking foo[foo.length - 1] which basically devolves to "does this array have a non-0 length"). Can we make it more meaningful, maybe by checking that there was a different node here before? The check should have used originalLen to check that the number of built nodes was kept consistent.

Comment on attachment 8902250 [details] Bug 1392081 - Set the image attribute on bookmarks toolbar buttons only when they are visible. https://reviewboard.mozilla.org/r/173780/#review181118 Looks good, assuming things still work with the comparison (= vs ==) below corrected, and some answer (prose or code) for the other question. :-) ::: browser/components/places/content/browserPlacesViews.js:1266 (Diff revision 2) > + let prevSiblingOverflowed = aIndex > 0 && aIndex < children.length && > + children[aIndex - 1].style.visibility == "hidden"; Why do we need to check for `< children.length` here? If the last current child is overflown, the newly added child will still overflow, right, even in the case where only some of the kids are being rendered? ::: browser/components/places/content/browserPlacesViews.js:1292 (Diff revision 2) > // Here we need the <menu>. > if (elt.localName == "menupopup") > elt = elt.parentNode; > > - if (parentElt == this._rootElt) { > + if (parentElt == this._rootElt) { // Node is on the toolbar. > + let overflowed = elt.style.visibility = "hidden"; This looks like it should be ```js let overflowed = elt.style.visibility == "hidden"; ``` (note double ==) right?

Comment on attachment 8902250 [details] Bug 1392081 - Set the image attribute on bookmarks toolbar buttons only when they are visible. https://reviewboard.mozilla.org/r/173780/#review181118 > Why do we need to check for `< children.length` here? If the last current child is overflown, the newly added child will still overflow, right, even in the case where only some of the kids are being rendered? Because the next children[aIndex - 1].style.visibility wouldn't work if children[aIndex - 1] is undefined. > This looks like it should be > > ```js > let overflowed = elt.style.visibility == "hidden"; > ``` > > (note double ==) > > right? Ah, I fixed this already, but I amended the change in the next part instead of here, my fault, it clearly works, it's just in the wrong patch.

(In reply to Marco Bonardo [::mak] from comment #25) > Comment on attachment 8902250 [details] > Bug 1392081 - Set the image attribute on bookmarks toolbar buttons only when > they are visible. > > https://reviewboard.mozilla.org/r/173780/#review181118 > > > Why do we need to check for `< children.length` here? If the last current child is overflown, the newly added child will still overflow, right, even in the case where only some of the kids are being rendered? > > Because the next children[aIndex - 1].style.visibility wouldn't work if > children[aIndex - 1] is undefined. OK, but then should it be `<= children.length` or `< children.length + 1` ? Because adding an item *at* `children.length` would work, because then we query `children[children.length - 1]` which is the last item, right?

yep, will change to a <= after the rebase, good catch.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/2b76c3b079d9 Use a document fragment to populate toolbar and menu Places views. r=Gijs https://hg.mozilla.org/integration/autoland/rev/ecd61b92f9b3 Set the image attribute on bookmarks toolbar buttons only when they are visible. r=Gijs https://hg.mozilla.org/integration/autoland/rev/8998e0bed6ff Only build a subset of the buttons that may become visible on the bookmarks toolbar. r=Gijs https://hg.mozilla.org/integration/autoland/rev/882a40327e5a Mochitest browser test for bookmarks toolbar overflow. r=Gijs https://hg.mozilla.org/integration/autoland/rev/cf5b68506bef Reset the Places toolbar view when the toolbar is collapsed. r=Gijs

Backed out for failing browser/components/places/tests/browser/browser_toolbar_overflow.js: https://hg.mozilla.org/integration/autoland/rev/aae17c1a570cdc89c1d97e748311d88181d9d69e https://hg.mozilla.org/integration/autoland/rev/294c5bf9ffea5c0110c88656bad9f55db89da1f1 https://hg.mozilla.org/integration/autoland/rev/db23967116ba47a873ea676192d40039a964b1aa https://hg.mozilla.org/integration/autoland/rev/82ef4728fdf54703454e4a16200d20576e905017 https://hg.mozilla.org/integration/autoland/rev/aabf377fc622676904eee0b6f62afdb3bb3d65c9 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cf5b68506bef58600c4e8ec1654102fd36bf0c52&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128646938&repo=autoland [task 2017-09-05T20:33:32.558739Z] 20:33:32 INFO - Leaving test bound setup [task 2017-09-05T20:33:32.559942Z] 20:33:32 INFO - Entering test bound [task 2017-09-05T20:33:32.563147Z] 20:33:32 INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The overflow chevron should be visible - true == true - [task 2017-09-05T20:33:32.569622Z] 20:33:32 INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Not all the nodes should be built by default - true == true - [task 2017-09-05T20:33:32.572049Z] 20:33:32 INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The number of visible nodes should be smaller than the number of built nodes - true == true - [task 2017-09-05T20:33:32.574387Z] 20:33:32 INFO - Node at the last visible index [task 2017-09-05T20:33:32.577349Z] 20:33:32 INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Sanity check the bookmark index - 43 == 43 - [task 2017-09-05T20:33:32.582079Z] 20:33:32 INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the added bookmark at the expected position - "yYvTd7Ox7kuV" == "yYvTd7Ox7kuV" - [task 2017-09-05T20:33:32.584296Z] 20:33:32 INFO - Buffered messages finished [task 2017-09-05T20:33:32.587507Z] 20:33:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_toolbar_overflow.js | Uncaught exception - The bookmark node should be visible - timed out after 50 tries.

Uh, it must be an intermittent, I retriggered the test multiple times on Try and it was always green.

Please also take a look at this failure: https://treeherder.mozilla.org/logviewer.html#?job_id=128675876&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=128668413&repo=autoland

the failures in comment 45 look unlikely to be caused by my patch, It is my understanding that browser_startup.js runs without opening any places view (the toolbar is hidden by default) and as such I can't really see how my patch could cause those. At a maximum I'm delaying things... The failure actually happened on the push for Bug 1353013, so it may be interesting to see if it returns after the backout. Florian, any idea how this bug or Bug 1353013 may have influenced the time by which we load Places? I wonder if delaying more stuff we ended up making the star init happen sooner.

also, may the about:NewTab page load Places sooner? Unfortunately I cannot check if Bug 1353013 is involved because it also has been backed out shortly after, thus I can't tell if the intermittent continued after the backout of this bug. But I really don't see a relation with my changes.

Unfortunately, there's still enough non-determinism in startup that sometimes landing an improvement causes something else to run sooner. This is especially true for anything that involves promises resolving when some I/O finishes, or for messages received from content processes. We recently had bug 1391495 that became perma-fail, likely after some unrelated patches landed and made something else faster during startup. If you are confident that your patch is an improvement, I don't want you to be blocked on these failures (if they are confirmed to happen more when landing your patch), so I'm ok with rs'ing a patch moving the relevant lines from the blacklist of the "before handling user events" phase to the "before first paint" blacklist. Preferably with a new bug filed to investigate, and the bug number mentioned in comments inside the browser_startup.js file.

In the end I think the failures in comment 45 are existing intermittents that bug 1353013 made more prominent. See https://bugzilla.mozilla.org/show_bug.cgi?id=1353013#c24

After losing far too many hours trying to understand why PromiseLayoutFlush doesn't call back, I just gave up and left a todo comment in code. This will be a nice win regardless, and it's not regressing reflows. It would have been nice to further improve, but it's not worth the cost at this point, I'll file a follow-up to investigate the problem, at that point the code is in the tree and people more expert with reflow internals may look into that. The try without PLF looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29b274f1ab028e26e6290761cce800ddc09fe4cd

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/8cabc32862b8 Use a document fragment to populate toolbar and menu Places views. r=Gijs https://hg.mozilla.org/integration/autoland/rev/3f2d6fc1693d Set the image attribute on bookmarks toolbar buttons only when they are visible. r=Gijs https://hg.mozilla.org/integration/autoland/rev/b3a7b5c0618b Only build a subset of the buttons that may become visible on the bookmarks toolbar. r=Gijs https://hg.mozilla.org/integration/autoland/rev/fad9ce51e129 Mochitest browser test for bookmarks toolbar overflow. r=Gijs https://hg.mozilla.org/integration/autoland/rev/a0be5817a721 Reset the Places toolbar view when the toolbar is collapsed. r=Gijs

https://hg.mozilla.org/mozilla-central/rev/8cabc32862b8 https://hg.mozilla.org/mozilla-central/rev/3f2d6fc1693d https://hg.mozilla.org/mozilla-central/rev/b3a7b5c0618b https://hg.mozilla.org/mozilla-central/rev/fad9ce51e129 https://hg.mozilla.org/mozilla-central/rev/a0be5817a721

I tryed nighty release. I open FF with toolbar - everything is OK, but than i press on ">>" to display another bookmarks... freeze, high load, and extra memory usage again :( may be it help you to find solve If i use bookmarks panel (ctrl+b) - all OK, i can open any folder with bookmarks and scroll without freezes

Yes, the fix is only about the toolbar view, the chevron (>>) uses the menu view, that still has performance problems to be investigated. For now the important thing was to ensure the toolbar view doesn't slow down loading of the window, and doesn't use more memory than expected.

Unfortunately limiting the number of items in the bookmarks toolbar breaks the use case of a multi-row scrollable bookmarks toolbar (enabled via userChrome.css), in this case one would like to have all the bookmarks visible. See https://github.com/Aris-t2/CustomCSSforFx/issues/99 Any chance that the behavior could be made configurable?

Related to #60, maybe the chevron (>>) can be shown even in the multi-row case when the number of total items in the toolbar exceeds the limit. Any idea about how to do that?