1429126 - stylo: high anonymous box resolution overhead on tp6 amazon

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

See the profile at [1], where we spend ~20% of initial frame construction time doing anonymous box resolution on tp6 amazon.

To improve performance and memory usage, we currently cache inheriting anonymous box styles on the style they inherit from. We do this with an inline singly-linked list though, which means that we can't cache when the parent style is also an anonymous box [2].

I added some diagnostics to break down the cases. When looking up anonymous boxes, it looks like we get 1/3 hits, 1/3 misses due to nested anonymous boxes being uncacheable, and 1/3 other misses. So we should be able to save a few millseconds on tp6 amazon by making these cacheable.

Emilio and I discussed on IRC, and he suggested rejiggering the cache into an inline/out-of-line setup, where we store a single cache entry inline, and otherwise heap-allocate. The thinking is that block wrappers are the common case, and those will always have a single entry. I'm going to give this a try.

See also bug 1401449, which would eliminate the lazy pseudo cache that lives alongside the anonymous box cache.

[1] https://perfht.ml/2mbFz4q
[2] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/layout/style/ServoStyleContext.h#56

Comment 1

[Bobby Holley (:bholley)]

I just ran some numbers as to how often we cache more than one anonymous box, relative to how often we cache any anonymous boxes at all:

tp6 amazon: 342 / 1043
obama wikipedia: 507 / 8634
tp6 facebook: 25 / 178
tp6 youtube: 265 / 2511
github testcase from bug 1369902: 83 / 628

So that's probably ok. Amazon has the worst case, but that's also the site where we should help pageload the most.

Additional, I confirmed that none of the above pages ever has more than 4 entries in the cache, so that seems like a good size for the indirect buffer.

Comment 2

[Bobby Holley (:bholley)]

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=210a91b7007a9dd8db166765dadb996130529bc2
Perf: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac95dbec0c0eecf6570ef834b48a03dbf2de675f
Perf Comparison: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ac95dbec0c0eecf6570ef834b48a03dbf2de675f&framework=1&selectedTimeRange=172800

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Avoid unified bustage. v1

The next patch adds a cpp file, which shuffles the unified buckets and
exposes missing includes.

MozReview-Commit-ID: EP88MXYNEuS

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Rejigger cached anonymous box styles to use a union. v1

MozReview-Commit-ID: I2b3wILKwNp

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove the caching bailout for nested anonymous box styles. v1

MozReview-Commit-ID: L5J26pAQ6y1

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941270 [details] [diff] [review]
Part 2 - Rejigger cached anonymous box styles to use a union. v1

Review of attachment 8941270 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments addressed.

Looks great, thanks!

::: layout/style/CachedAnonBoxStyles.cpp
@@ +10,5 @@
> +
> +namespace mozilla {
> +
> +void
> +CachedAnonBoxStyles::Insert(ServoStyleContext* aStyle)

Let's assert that aStyle is non-null, that aStyle's GetPseudo() is an anon box, and similarly, that it's an inheriting anon box at that?

@@ +14,5 @@
> +CachedAnonBoxStyles::Insert(ServoStyleContext* aStyle)
> +{
> +  if (IsEmpty()) {
> +    RefPtr<ServoStyleContext> s = aStyle;
> +    mBits = reinterpret_cast<uintptr_t>(s.forget().take());

nit: Maybe MOZ_ASSERT(IsDirect()) here, just to sanity-check?

@@ +21,5 @@
> +  } else {
> +    IndirectCache* cache = new IndirectCache();
> +    cache->AppendElement(dont_AddRef(AsDirect()));
> +    cache->AppendElement(aStyle);
> +    mBits = reinterpret_cast<uintptr_t>(cache) | 1;

nit: And ditto but with IsIndirect() here I guess.

@@ +27,5 @@
> +}
> +
> +ServoStyleContext*
> +CachedAnonBoxStyles::Lookup(nsAtom* aAnonBox) const
> +{

Let's assert that aAnonBox is an anon box and that it's an inheriting anon box.

@@ +30,5 @@
> +CachedAnonBoxStyles::Lookup(nsAtom* aAnonBox) const
> +{
> +  if (IsIndirect()) {
> +    IndirectCache& cache = *AsIndirect();
> +    for (size_t i = 0; i < cache.Length(); ++i) {

nit: You can use ranged for loop to avoid the two bound-checks below, and make the code nicer:

for (auto& style : *AsIndirect()) {
    // ...
}

Or maybe specifying the type, as you wish.

@@ +40,5 @@
> +    return nullptr;
> +  }
> +
> +  ServoStyleContext* direct = AsDirect();
> +  return direct && direct->GetPseudo() == aAnonBox ? direct : nullptr;

May be worth to inline this, or maybe not, your call...

@@ +48,5 @@
> +CachedAnonBoxStyles::AddSizeOfIncludingThis(nsWindowSizes& aSizes, size_t* aCVsSize) const
> +{
> +  if (IsIndirect()) {
> +    IndirectCache& cache = *AsIndirect();
> +    for (size_t i = 0; i < cache.Length(); ++i) {

Ditto, re. the loop.

::: layout/style/CachedAnonBoxStyles.h
@@ +14,5 @@
> +
> +namespace mozilla {
> +
> +class ServoStyleContext;
> +

Maybe add a bit of documentation here?

@@ +15,5 @@
> +namespace mozilla {
> +
> +class ServoStyleContext;
> +
> +class CachedAnonBoxStyles {

nit: Brace to the next line.

@@ +33,5 @@
> +
> +  void AddSizeOfIncludingThis(nsWindowSizes& aSizes, size_t* aCVsSize) const;
> +
> +private:
> +  typedef AutoTArray<RefPtr<ServoStyleContext>, 4> IndirectCache;

Please document why 4, maybe pointing to your measurements in this bug?

::: layout/style/ServoStyleContext.h
@@ +48,5 @@
>    }
>  
> +  ServoStyleContext* GetCachedInheritingAnonBoxStyle(nsAtom* aAnonBox) const
> +  {
> +    MOZ_ASSERT(nsCSSAnonBoxes::IsInheritingAnonBox(aAnonBox));

Oh, you assert here... Oh well, maybe it's worth to assert in both places, just to not depend on this assertion? Or just remove this one.

@@ +61,5 @@
>  
>    void SetCachedInheritedAnonBoxStyle(nsAtom* aAnonBox,
>                                        ServoStyleContext* aStyle)
>    {
>      MOZ_ASSERT(!GetCachedInheritingAnonBoxStyle(aAnonBox));

Similarly this one could be removed, if you want to. Or if you want to remove the CachedAnonBoxStyles, this needs to assert that aStyle->GetPseudo() == aAnonBox, I guess.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941271 [details] [diff] [review]
Part 3 - Remove the caching bailout for nested anonymous box styles. v1

Review of attachment 8941271 [details] [diff] [review]:
-----------------------------------------------------------------

Nice :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f679a1bd3a9
Avoid unified bustage. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b690a513d859
Rejigger cached anonymous box styles to use a union. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a20954ce2d02
Remove the caching bailout for nested anonymous box styles. r=emilio

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8f679a1bd3a9
https://hg.mozilla.org/mozilla-central/rev/b690a513d859
https://hg.mozilla.org/mozilla-central/rev/a20954ce2d02

Comment 10

[Julien Cristau [:jcristau]]

It's too late for 58.