1377352 - nsXPLookAndFeel::GetIntImpl etc are slow

Status: RESOLVED WORKSFORME

(Core :: Widget, defect, P3)

Description

[Mats Palmgren (inactive)]

I'm looking at the perf profile for Gmail in bug 1371668 comment 0 and
the following stack (inverted) shows up there:

mozilla::ScrollFrameHelper::BuildDisplayList()
mozilla::ScrollFrameHelper::AppendScrollPartsTo()
mozilla::LookAndFeel::GetInt(mozilla::LookAndFeel::IntID,int)
nsLookAndFeel::GetIntImpl(mozilla::LookAndFeel::IntID,int &)
nsXPLookAndFeel::GetIntImpl(mozilla::LookAndFeel::IntID,int &)

I suspect the problem here is that the compiler can't devirtualize
these calls.

Comment 1

[Mats Palmgren (inactive)]

Unfortunately though, bug 1338004 changed:
http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/widget/nsXPLookAndFeel.h#45
to return the base class rather than the nsLookAndFeel leaf class,
so making nsLookAndFeel 'final' (bug 1377348) probably won't help
this bug. :(

Comment 2

[Mats Palmgren (inactive)]

It might work if we move nsXPLookAndFeel::GetInstance() to the header
so that it can be inlined:
http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/widget/nsXPLookAndFeel.cpp#260
and then split 'sInstance' into two separate variables, each having
the concrete types, then return those in the branches, something like:

  static nsLookAndFeel sInstanceLookAndFeel;
  static widget::HeadlessLookAndFeel sInstanceHeadless;
  if (sInstanceLookAndFeel) {
    return sInstanceLookAndFeel;
  } else if (!gfxPlatform::IsHeadless()) {
    return sInstanceLookAndFeel = new nsLookAndFeel();
  } else if (sInstanceHeadless) {
    return sInstanceHeadless;
  } else {
    return sInstanceHeadless = new widget::HeadlessLookAndFeel();
  }

then the compiler might be able to call the concrete method
based on which branch was taken...

Comment 3

[Brendan Dahl [:bdahl]]

It seems odd that this is showing up in profiles as I see it's only called ~10k times while browsing gmail and youtube for a few minutes. There is a loop in nsXPLookAndFeel::GetIntImpl over the preference stored values but that's also pretty small.

I'm not sure it's possible, but we could reduce a number of layers here if we instead used a single lookup in an array (there are ~53 ints) of all the cached int values and then relied on each platform specific lookAndFeel to update these values.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

We have such caches now: LookAndFeel::GetInt in the fast path is just https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/widget/nsXPLookAndFeel.cpp#949-954