Closed Bug 1429126 Opened 2 years ago Closed 2 years ago

stylo: high anonymous box resolution overhead on tp6 amazon

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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
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.
The next patch adds a cpp file, which shuffles the unified buckets and
exposes missing includes.

MozReview-Commit-ID: EP88MXYNEuS
Attachment #8941269 - Flags: review?(emilio)
MozReview-Commit-ID: I2b3wILKwNp
Attachment #8941270 - Flags: review?(emilio)
MozReview-Commit-ID: L5J26pAQ6y1
Attachment #8941271 - Flags: review?(emilio)
Attachment #8941269 - Flags: review?(emilio) → review+
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.
Attachment #8941270 - Flags: review?(emilio) → review+
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 :)
Attachment #8941271 - Flags: review?(emilio) → review+
Pushed by bholley@mozilla.com:
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
Blocks: 1429529
It's too late for 58.
You need to log in before you can comment on or make changes to this bug.