1631929 - Ability to mark StyleGenericCalcNode functions as MOZ_NEVER_INLINE

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[(Away)]

We had to disable clang's new pass manager in a few build flavors due to very long build times. I tracked this down to a bad interaction with the inliner and StyleGenericCalcNode as described in https://bugs.llvm.org/show_bug.cgi?id=45253#c3.

Now that we understand the problem, instead of disabling the new optimizer altogether, we could do a workaround that keeps the inlining in check. I believe the key players in the inlining cycle are StyleGenericCalcNode's destructor and copy constructor. If we mark these as MOZ_NEVER_INLINE then the cycle breaks. I confirmed this locally by manually editing the generated code.

From chat with Emilio it sounds like it should be feasible to add the capability for such annotations to cbindgen.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

https://github.com/eqrion/cbindgen/pull/515 will allow us to just add this line around here:

/// cbindgen:destructor-attributes=MOZ_NEVER_INLINE

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So we can do different things here, because both the PR and the change above would not be breaking. I can do a cbindgen release with that PR, land the annotations here, and be done, or I can (on top) update cbindgen in-tree (which forces everyone to update it).

Your call whether this is severe enough to enforce everybody to use this, or we can roll with the cbindgen release and the annotations, and tell the people hitting long compile times to use themselves a newer cbindgen.

Thoughts? Either way is fine for me, both are relatively straight-forward. Just trying to not annoy people too much because the last cbindgen update was two weeks ago (bug 1628754).

Comment 3

[(Away)]

I'm completely fine for this to wait until there's a stronger reason to update cbindgen. It won't hurt to keep the affected builds on the old optimizer until then.

Comment 4

[(Away)]

I think we're gonna need this in order to land the clang-10 upgrade, the set of affected build jobs has shifted around. :/ If anyone encounters the issue locally, it would be annoying and non-obvious to debug, so I'm thinking this might be worth the hit of updating the in-tree requirement, sadly.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Sure thing!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1631929 - Update cbindgen version. r=#build,dmajor

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1631929 - Have the destructor of CalcNode be never inline. r=dmajor

You tell me which other things need it... Copy-constructor? operator==?
and such?

Depends on D73384

Comment 8

[(Away)]

IIUC, the cbindgen update from a couple days ago pulled in the necessary pieces for this, right?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yes!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0451875e3335
Have the destructor of CalcNode be never inline. r=dmajor

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0451875e3335