nsXPLookAndFeel::GetIntImpl etc are slow

NEW
Unassigned

Status

()

Core
Widget
P3
minor
a year ago
11 months ago

People

(Reporter: mats, Unassigned)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tpi:+)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
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. :(
(Reporter)

Comment 2

a year ago
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...
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.

Updated

11 months ago
Priority: -- → P3
Whiteboard: tpi:+
You need to log in before you can comment on or make changes to this bug.