Extra memory usage and startup hit with turned on bookmarks toolbar

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Bookmarks & History
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: che13.ne, Assigned: mak)

Tracking

(Blocks: 1 bug, {footprint, perf})

55 Branch
Firefox 57
footprint, perf
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch] [reserve-photon-performance])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Reporter)

Description

3 months ago
Created attachment 8899247 [details]
Extra memory usage with turned on bookmarks toolbar

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
(Reporter)

Updated

3 months ago
Component: Untriaged → Toolbars and Customization
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 .
Component: Toolbars and Customization → Untriaged
Flags: needinfo?(che13.ne)

Updated

3 months ago
Component: Untriaged → Toolbars and Customization
Keywords: footprint
(Reporter)

Comment 2

3 months ago
Created attachment 8899536 [details]
bookmarks toolbar turned on

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

Comment 3

3 months ago
Created attachment 8899537 [details]
bookmarks toolbar turned off
Flags: needinfo?(che13.ne)
(Reporter)

Updated

3 months ago
Attachment #8899536 - Attachment description: bookmarks toolbox turned on → bookmarks toolbar turned on
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?
Component: Toolbars and Customization → Bookmarks & History
Flags: needinfo?(mak77)
See Also: → bug 1380999
(Assignee)

Comment 5

3 months ago
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.
Assignee: nobody → mak77
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mak77)
Priority: -- → P1
Whiteboard: [fxsearch]
(Assignee)

Updated

3 months ago
Blocks: 1380999
See Also: bug 1380999
(Assignee)

Updated

3 months ago
Summary: Extra memory usage with turned on bookmarks toolbar → Extra memory usage and startup hit with turned on bookmarks toolbar
Whiteboard: [fxsearch] → [fxsearch][photon-performance]

Updated

3 months ago
Flags: qe-verify?
Whiteboard: [fxsearch][photon-performance] → [fxsearch] [reserve-photon-performance]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

3 months ago
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.
(Assignee)

Updated

3 months ago
Keywords: perf

Comment 12

3 months ago
mozreview-review
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.
Attachment #8902249 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 13

3 months ago
mozreview-review
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.
Attachment #8902250 - Flags: review?(gijskruitbosch+bugs)

Comment 14

3 months ago
mozreview-review
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? :-)
Attachment #8902251 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 15

3 months ago
mozreview-review
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?
Attachment #8902252 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 16

3 months ago
mozreview-review
Comment on attachment 8902439 [details]
Bug 1392081 - Reset the Places toolbar view when the toolbar is collapsed.

https://reviewboard.mozilla.org/r/174028/#review180452
Attachment #8902439 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 17

3 months ago
mozreview-review-reply
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!
(Assignee)

Comment 18

3 months ago
mozreview-review-reply
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.
(Assignee)

Comment 19

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 months ago
mozreview-review
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?
Attachment #8902250 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 25

2 months ago
mozreview-review-reply
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?
(Assignee)

Comment 27

2 months ago
yep, will change to a <= after the rebase, good catch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

2 months ago
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.
Flags: needinfo?(mak77)
(Assignee)

Comment 44

2 months ago
Uh, it must be an intermittent, I retriggered the test multiple times on Try and it was always green.
Flags: needinfo?(mak77)
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
(Assignee)

Comment 46

2 months ago
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.
Flags: needinfo?(florian)
(Assignee)

Comment 47

2 months ago
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.
Flags: needinfo?(florian)
(Assignee)

Comment 49

2 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 53

2 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 56

2 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 months ago
Iteration: --- → 57.3 - Sep 19
(Assignee)

Updated

2 months ago
Depends on: 1398019
(Reporter)

Comment 58

2 months ago
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
(Assignee)

Comment 59

2 months ago
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.
Depends on: 1398621

Updated

2 months ago
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.