Closed Bug 1631929 Opened 5 years ago Closed 4 years ago

Ability to mark StyleGenericCalcNode functions as MOZ_NEVER_INLINE

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: away, Assigned: emilio)

References

Details

Attachments

(1 file, 1 obsolete file)

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.

Flags: needinfo?(emilio)

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

/// cbindgen:destructor-attributes=MOZ_NEVER_INLINE
Assignee: nobody → 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).

Flags: needinfo?(emilio) → needinfo?(dmajor)

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.

Flags: needinfo?(dmajor)

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.

Blocks: clang-10
Flags: needinfo?(emilio)

Sure thing!

Flags: needinfo?(emilio)

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

Depends on D73384

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

Yes!

Attachment #9144897 - Attachment is obsolete: true
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0451875e3335 Have the destructor of CalcNode be never inline. r=dmajor
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Depends on: 1677839
Blocks: 1677839
No longer depends on: 1677839
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: