Closed Bug 1369902 Opened 7 years ago Closed 7 years ago

stylo: Not much ComputedValues sharing going on on github diff page

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

53 Branch
enhancement

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

()

Details

On https://github.com/bholley/gecko/commit/96ecf505adde95240799d285de90a007310f7bda even with the various fixes we've made for bug 1367862 (which help a lot for that page), I'm not getting very good style sharing.  Even in sequential mode.

Specifically, I am seeing about 109k ComputedValues structs allocated for this page, of which 43k are for text and 39k are for other pseudos.  The remaining 27k are for elements.  For comparison, Gecko allocates a total of about 2200 style contexts here.
> which help a lot for that page

I meant help a lot for the page in bug 1367862.

Just for context, there are 63k actual elements on this page.

To put numbers to the memory usage here, by the way, about:memory claims 244.69 MB used by this page's process, with 135.95 MB unclassified.  With Gecko those numbers are 144.44 MB and 30.34 MB respectively.
So each line of code has markup that looks like this:

  <tr>
    <td class="blob-num blob-num-addition empty-cell"></td>
    <td id="diff-8b00cd0478f568c67a7c0949894dfa78R32" data-line-number="32" class="blob-num blob-num-addition js-linkable-line-number"></td>
    <td class="blob-code blob-code-addition">
      <button class="btn-link add-line-comment js-add-line-comment js-add-single-line-comment" data-path="third_party/rust/coco/src/epoch/atomic.rs" data-anchor="diff-8b00cd0478f568c67a7c0949894dfa78" data-position="32" data-line="32" type="button" aria-label="Add line comment">
         <svg aria-hidden="true" class="octicon octicon-plus" height="16" version="1.1" viewBox="0 0 12 16" width="12"><path fill-rule="evenodd" d="M12 9H7v5H5V9H0V7h5V2h2v5h5z"></path></svg>
      </button>
      <span class="blob-code-inner">+<span class="pl-k">pub</span> <span class="pl-k">struct</span> <span class="pl-en">Atomic</span>&lt;T&gt; {</span>
    </td>
  </tr>

and the page styles have:

  .blob-code-inner::before {
    content:""
  }

which means we can't share style across those blob-code-inners or any of their numerous span descendants.  If I just hack away the pseudo-element check in relations_are_shareable, we immediately drop to ~46K ComputedValues, of which most are text and pseudo-elements, with less than 2K element ComputedValues.

I filed bug 1369952 on sorting out how to make the pseudo-element thing work.
Depends on: 1369952
Depends on: 1329361
Assignee: nobody → bzbarsky
Priority: -- → P1
Boris, can you verify that this is fixed when you have a moment?
Flags: needinfo?(bzbarsky)
Priority: P1 → --
In sequential mode I now see ~2000 element ComputedValues allocated here.

In parallel mode with the fix for bug 1385982 I see ~4200 element ComputedValues allocated.

Until bug 1368290 is fixed, we're still allocating tens of thousands of pseudo styles here.  I think we should actually remeasure once that lands to make sure there's nothing else weird going on.  But the element parts are looking pretty good.
Depends on: 1368290
Flags: needinfo?(bzbarsky)
Priority: -- → P3
bz, can you make sure your tree has both bug 1387116 and bug 1368290 applied and remeasure this?
Flags: needinfo?(bzbarsky)
I just went ahead and applied the patch. I get output like [1] on the page from comment 0. That looks to me like the sharing is working. bz, do you agree?

[1]

OTHER PSEUDO COUNT INC: 2661
STYLO INC: 5271
COUNT INC: 5341
OTHER PSEUDO COUNT INC: 2662
STYLO INC: 5272
COUNT INC: 5342
OTHER PSEUDO COUNT INC: 2663
STYLO INC: 5273
COUNT INC: 5343
OTHER PSEUDO COUNT INC: 2664
STYLO INC: 5274
COUNT INC: 5344
OTHER PSEUDO COUNT INC: 2665
STYLO INC: 5275
COUNT INC: 5345
OTHER PSEUDO COUNT INC: 2666
STYLO INC: 5276
COUNT INC: 5346
OTHER PSEUDO COUNT INC: 2667
STYLO INC: 5277
COUNT INC: 5347
OTHER PSEUDO COUNT INC: 2668
STYLO INC: 5278
COUNT INC: 5348
TEXT COUNT INC: 679
STYLO INC: 5279
COUNT INC: 5349
OTHER PSEUDO COUNT INC: 2669
Numbers with all the bits from comment 5 applied, as of today, on a 4-core machine:

  Stylo: 5040 elements, 4875 non-text pseudos, 4082 text
  Stylo-sequential: 1955 elements, 2645 non-text pseudos, 678 text
  Gecko: 1211 elements, 654 non-text pseudos, 317 text

I've double-checked the numbers a few times and they keep landing in the same ballpark; I don't know why there's such a huge difference for "text" between stylo-sequential and stylo-parallel.

In general, these numbers do look like sharing is working for elements inasmuch as we expect it to work: stylo-sequential is about 1.5-2x worse than Gecko and parallel is 2x or more worse than sequential.  As I said, I don't know what's going on with text going from sequential to parallel with stylo...

The non-text pseudos may be affected by bug 1368291; these numbers are without that patch applied.

If I apply that patch, I get numbers like so:

  Stylo: 4935 elements, 4620 non-text pseudos, 3907 text
  Stylo-sequential: 1955 elements, 2415 non-text pseudos, 678 text

so the "non-text pseudos" bucket does get a bit smaller, at least.
Flags: needinfo?(bzbarsky)
I think it's fine to resolve this, fwiw.  To recap, current numbers are:

  Stylo: 4935 elements, 4620 non-text pseudos, 3907 text
  Stylo-sequential: 1955 elements, 2415 non-text pseudos, 678 text

and numbers when we started were (stylo-sequential only):

  27k elements, 39k non-text pseudos, 43k text
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.