1466529 - Stylo is malloc heavy when parsing property values for style.foo = "bar";

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Our style.foo = "bar"; handling seems to be a lot slower (2.5x) than in Blink, 
at least in .color = "some valid value" case.

A testcase I've used for profiling is
https://bugzilla.mozilla.org/attachment.cgi?id=8982654
"withoutmutationobserver" part is perhaps the most valid part for this bug.

13% seems to be malloc/free + 9% locks around malloc/free

I'm not familiar with Stylo code, but could we optimize things like
https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/servo/ports/geckolib/glue.rs#3543
which shows up in the profile.

Could we use more stack allocation? Reuse objects?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

And when we need to clone DeclarationBlock (if there are mutation event listeners or so), cloning is very malloc heavy.
Could we have 1 item cache for DeclarationBlocks or something?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

https://perfht.ml/2JrVGsc

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This week I'm going to be in a profiling training in Paris and this kind of test-case could come handy ;)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #2)
> https://perfht.ml/2JrVGsc

Olli, was this build a --enable-release build? I'm somewhat surprised to see some stuff not getting inlined there.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

It was with export RUSTC_OPT_LEVEL=2

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

oh, wait, perhaps it was nightly. Let me reprofile.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

https://perfht.ml/2JuKeMr this at least is Nightly. Still showing similar memory issues.

Comment 8

[u480271]

Attachment: Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

Spinning on setting nsStyledElement::SetInlineStyleDeclaration causes
high malloc/free as MiscContainer is created for nsAttrValue passed to
Element::SetAttrAndNotify before the old one is delete'd.

Keep a cache of one MiscContainer in nsAttrValue to alleviate this
pattern.

Review commit: https://reviewboard.mozilla.org/r/259000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259000/

Comment 9

[u480271]

withoutmutationobserver post MiscContainer caching: https://perfht.ml/2uM2sAn

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266046

::: dom/base/nsAttrValue.cpp:69
(Diff revision 1)
> +    }
> +  } else if (sMiscContainerCache.compareExchange(nullptr, aCont)) {
> +    return;
> +  }
> +
> +  // We raced with somebody else setting the cache; delete container.

...or there was a cache already. 
(nothing 'raced' in that case, I'd say)

::: dom/base/nsAttrValueInlines.h:125
(Diff revision 1)
>    void Cache();
>    void Evict();
> +
> +protected:
> +  /** Reset to initial empty container */
> +  void Reset() {

{ should be on its own line

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266050

::: dom/base/nsAttrValue.cpp:41
(Diff revision 1)
>  
> +/* static */ MiscContainer*
> +nsAttrValue::AllocMiscContainer()
> +{
> +  MiscContainer* cont = nullptr;
> +  if (MOZ_LIKELY(!IsInServoTraversal())) {

Why do you need this? You should be able to assert `NS_IsMainThread()`, we don't set attributes OMT, only the `style` attribute serialization, which given it's a declaration block should already have a misc container.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266052

::: dom/base/nsAttrValue.cpp:41
(Diff revision 1)
>  
> +/* static */ MiscContainer*
> +nsAttrValue::AllocMiscContainer()
> +{
> +  MiscContainer* cont = nullptr;
> +  if (MOZ_LIKELY(!IsInServoTraversal())) {

Note that SMIL _may_ set attributes when `IsInServoTraversal()` is true, IIRC, but that always happens on the main thread.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

All the stylo-code is somewhat hairy, that either we need to be consistent and handle alloc/dealloc when stylo is running, or assert hard.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #13)
> All the stylo-code is somewhat hairy, that either we need to be consistent
> and handle alloc/dealloc when stylo is running, or assert hard.

Yeah, the only place where we need to handle this is the serialization of the style attribute. Other than that everything should assert NS_IsMainThread.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

I guess this requires stylo peer review, given its unusual integration here with DOM code.

Comment 16

[u480271]

Thanks Olli. I'll follow up with Emilio.

Comment 17

[u480271]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259000/diff/1-2/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266280

::: dom/base/nsAttrValueInlines.h:126
(Diff revision 2)
>    void Evict();
> +
> +protected:
> +  /** Reset to initial empty container */
> +  void Reset()
> +  {

Hmm, this is a bit too much of a generic name given it doesn't free the resources it would have...

Not that the destructor would assert a lot more, but maybe instead of this we should just do:

```
cont->~MiscContainer();
new (cont) MiscContainer();
```

On the only caller?

Comment 19

[u480271]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266538

::: dom/base/nsAttrValueInlines.h:126
(Diff revision 2)
>    void Evict();
> +
> +protected:
> +  /** Reset to initial empty container */
> +  void Reset()
> +  {

I like not adding a new function!

Comment 20

[u480271]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259000/diff/2-3/

Comment 21

[u480271]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259000/diff/3-4/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266594

::: dom/base/nsAttrValue.cpp:55
(Diff revision 4)
> +
> +/* static */ void
> +nsAttrValue::DeallocMiscContainer(MiscContainer* aCont)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!aCont) return;

nit: use braces, please, and move return to its own line:

```
if (!aCont) {
  return;
}
```

Comment 23

[u480271]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

https://reviewboard.mozilla.org/r/259000/#review266598

::: dom/base/nsAttrValue.cpp:55
(Diff revision 4)
> +
> +/* static */ void
> +nsAttrValue::DeallocMiscContainer(MiscContainer* aCont)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!aCont) return;

But, so ugly...

Comment 24

[u480271]

Comment on attachment 8994446 [details]
Bug 1466529 - P1: Cache MiscContainer to avoid malloc/free churn.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259000/diff/4-5/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fe9aa9f75aa
P1: Cache MiscContainer to avoid malloc/free churn. r=emilio,smaug

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0fe9aa9f75aa