977594 - display:flex with unique styles and non-block children uses more memory than display:inline because of nsStyleDisplay faulted out of rule tree sharing

Status: NEW

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Ben Kelly [:bkelly, not reviewing]]

While profiling and experimenting with roc's virtual-list-demo in bug 975831 I noticed that display:flex seems to use more pres-shell memory than display:inline.

I realized this when I accidentally created a DOM with 10,000 absolute positioned divs, each set as display:flex.  Within each of these divs were three spans.  One with a background-color meant to simulate an image, and two with text.  With this DOM I was seeing ~21MB of pres-shell memory usage in about:memory.

If I changed the 10,000 divs to use display:inline and the three contained spans to use display:block, then I saw only ~10MB of pres-shell memory being used.

This was on a 1.4 buri device.

:dholbert asked about the memory difference with empty divs.  Removing the three contained spans from each div resulted in display:inline using about 0.35MB more memory than the display:flex solution.

I was waiting to file this till I had a minimal test, but I haven't had time to put that together yet.  In the meantime you can fairly easily get these situations by hacking up roc's demo here:

  http://people.mozilla.org/~roc/virtual-list-demo.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

An nsFlexContainerFrame should actually be smaller than an nsInlineFrame.

I'd really appreciate the actual code you were dealing with; it doesn't have to be minimal, just needs to exist so we can see what the frame tree ends up looking like.....

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #1)
> I'd really appreciate the actual code you were dealing with; it doesn't have
> to be minimal, just needs to exist so we can see what the frame tree ends up
> looking like.....

Its 99% the virtual-list-demo.html linked in comment 0.

To get the massive DOM change line 80 from:

  function getScrollPortHeight() { return window.innerHeight; }

To:

  function getScrollPortHeight() { return window.clientHeight; }

This will cause all the elements to be rendered up-front instead of lazily.

To get the non-flex version to compare against I changed the CSS like this:

.item {
  display:inline;
  /* leave other attributes */
}
.name {
  display:block;
  /* leave other attributes */
}
.number {
  display:block;
  /* leave other attributes */
}
.image {
  display:block;
  float: left;
  /* leave other attributes */
}

Sorry I don't have time at the moment to package it up into a nice clickable page, but maybe that will be enough to start with.

Comment 3

[Boris Zbarsky [:bzbarsky]]

No need to package anything up; comment 2 is great.  Though I had to use 300000 for the scrollport height, since window.clientHeight is undefined.

So here's what I see with a current nightly on that test in about:memory:

│  │  │  │  ├──121.52 MB (40.84%) -- layout
│  │  │  │  │  ├───60.98 MB (20.49%) ── pres-shell
│  │  │  │  │  ├───22.75 MB (07.64%) -- frames
│  │  │  │  │  │   ├───8.26 MB (02.78%) ── nsBlockFrame
│  │  │  │  │  │   ├───6.66 MB (02.24%) ── nsHTMLScrollFrame
│  │  │  │  │  │   ├───4.06 MB (01.36%) ── nsTextFrame
│  │  │  │  │  │   └───3.77 MB (01.27%) ++ (3 tiny)
│  │  │  │  │  ├───22.31 MB (07.50%) ── style-contexts

If I add this style to the page:

  .name, .number, .image { display: block }

then I get numbers like so:

│  │  │  ├───94.73 MB (36.13%) -- layout
│  │  │  │   ├──34.19 MB (13.04%) ── pres-shell
│  │  │  │   ├──22.75 MB (08.68%) -- frames
│  │  │  │   │  ├───8.26 MB (03.15%) ── nsBlockFrame
│  │  │  │   │  ├───6.66 MB (02.54%) ── nsHTMLScrollFrame
│  │  │  │   │  ├───4.06 MB (01.55%) ── nsTextFrame
│  │  │  │   │  └───3.77 MB (01.44%) ++ (3 tiny)
│  │  │  │   ├──22.31 MB (08.51%) ── style-contexts

The difference is that now we're sharing a single nsStyleDisplay across each of those kids, instead of allocating a unique one for each one.  With the scroll height I set, I see document.querySelectorAll(".name, .number, .image").length == 56964, so we're saving about 57000 nsStyleDisplay allocations.  I'm on a 64-bit system, so an nsStyleDisplay is a bit north of 300 bytes, if I count right, which puts us in the right ballpark to be using 20+MB of memory for nsStyleDisplays here.  :(

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I thought I had tested adding display:block to .name/.number..image with display:flex and still saw elevated memory usage.  (At least dholbert asked me to test that!)  Maybe I screwed up my test.  Obviously I don't remember exactly my test conditions.

So is just a case where the content could be better styled and its not really a bug in the platform?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I guess there's the question of why style context sharing doesn't save us.  That's because each div has different rules (due to all the inline styles setting .top) and hence different style contexts...  If I take that rule out, the pres-shell number drops to 6MB and the style-contexts number drops to 3MB.  If I also take out the inline style for the background on .image, those numbers drop to 0.84MB and 0.01MB, even with the display:block rule removed, since now we're sharing style contexts for everything.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, per spec having a flex item parent should automagically make you a block.  So the explicit block styles should not be needed, ideally.

The problem is that the automagic state depends on the parent, and hence needs to be stored in each child separately, unless they manage to share data.  The explicit style, on the other hand, could be stored in the ruletree.

So one thing we could try doing is this.  When doing the flex-item display fixup, check whether our display struct is stored in the ruletree (which it almost always is).  If so, check another cache attached to the rulenode for an already-munged display struct with the block display.  If it's there, just use it.  Otherwise, create one, stash it in the ruletree, and use it.  That would let us share the allocations.

One problem is that this would add a word to all rulenodes, which would be pretty rarely used.  :(

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably this is
https://hg.mozilla.org/mozilla-central/file/58eca03214a6/layout/style/nsStyleContext.cpp#l357

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems a bit odd to me to go to so much effort to work around this one case of faulting out of the rule tree.  I think em units are probably a substantially more common case, though I'd like to try to solve that one.  Maybe what I was thinking in bug 804975 could be applied here too?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, it absolutely could.  Comment 6 is a cut-down version of that proposal, clearly.  ;)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #7)
> Presumably this is
> https://hg.mozilla.org/mozilla-central/file/58eca03214a6/layout/style/nsStyleContext.cpp#l357

And in particular, the GetUniqueStyleData() call there, which has a warning about this exact problem:
> 225 // This is an evil evil function, since it forces you to alloc your own separate copy of
> 226 // style data!  Do not use this function unless you absolutely have to!  You should avoid
> 227 // this at all costs! -dwh
> 228 void* 
> 229 nsStyleContext::GetUniqueStyleData(const nsStyleStructID& aSID)
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp?rev=3038a5a34a75&mark=225-229#225