Closed Bug 699482 Opened 9 years ago Closed 9 years ago

Talos complains about GfxDriverInfo static constructors, they might slow down startup


(Core :: Graphics, defect)

Not set





(Reporter: bjacob, Assigned: drs)




(1 file, 2 obsolete files)

This changeset:


   Talos Regression :( Number of Constructors increase 1.2% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound

This changeset only generalizes to other platforms static ctors that already exist on windows. So the problem preexists.

Can someone suggest what is the right way to fix this. Here's a suggestion: replace the static global constants, by static local constants in functions, getting initialized the first time that the function is called.

Like this:

const GfxDriverInfo* getDriverInfo()
  static GfxDriverInfo* sGfxDriverInfo = nsnull;
  if (!sGfxDriverInfo) {
    sGfxDriverInfo = ... allocate and initialize ... ;
  return sGfxDriverInfo;

sounds like a plan?
I agree, that sounds like a good idea. I will do this.
Assignee: nobody → dsherk
This patch moves all static initialization of GfxDriverInfo and DeviceFamily
classes to the point that they're actually used. It also converts all static
GfxDriverInfo arrays into nsTArray<GfxDriverInfo> so that they can be used
interchangeably with the downloadable blocklist.

This patch also introduces a new phase of blocklist checking called
BEING_PROCESSED, which is the status set when a blocklist check is currently
being processed. NO_INFO now only means that we have confirmed that a device is
not blocked.
Attachment #573143 - Flags: review?(bjacob)
Comment on attachment 573143 [details] [diff] [review]
Patch v1.0, refactor static constructors and other things.

Review of attachment 573143 [details] [diff] [review]:

::: widget/public/nsIGfxInfo.idl
@@ +118,5 @@
>    /* We don't explicitly block or discourage the feature. Which means we'll try getting it from the
>     * hardware, and see what happens. */
>    const long FEATURE_NO_INFO = 1;
> +  /* We don't know the status of the feature yet. The analysis probably hasn't finished yet. */
> +  const long FEATURE_BEING_PROCESSED = 2;

I rather think that we need to rename:

This should be done in a separate patch (please file a bug), but could you at lease rename FEATURE_BEING_PROCESSED -> FEATURE_STATUS_UNKNOWN in this patch. My rationale is that we need a status code for when we just don't have information, so FEATURE_BEING_PROCESSED seems overly specific (what matters is not that it is being processed, but that we don't know yet its status).

::: widget/src/windows/GfxInfo.cpp
@@ -930,5 @@
> -  const GfxDriverInfo *info;
> -  if (aDriverInfo)
> -    info = aDriverInfo;
> -  else
> -    info = &gDriverInfo[0];


::: widget/src/xpwidgets/GfxDriverInfo.cpp
@@ +117,5 @@
>    if (mDeleteDevices)
>      delete[] mDevices;
>  }
> +
> +const GfxDeviceFamily GfxDriverInfo::GetDeviceFamily(DeviceFamily id)

Ah OK. I see that GfxDeviceFamily is a typedef for PRUint32* so that's fine.

::: widget/src/xpwidgets/GfxDriverInfo.h
@@ +42,5 @@
>  #define __mozilla_widget_GfxDriverInfo_h__
>  #define V(a,b,c,d) GFX_DRIVER_VERSION(a,b,c,d)
> +#define IMPLEMENT_DRIVER_BLOCKLIST(os, vendor, devices, feature, featureStatus, driverComparator, driverVersion, suggestedVersion) \

IMPLEMENT is a bit vague. I'd say APPEND_TO or something like that.

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +617,5 @@
> +}
> +
> +PRInt32
> +GfxInfoBase::FindBlocklistedDeviceInList(const nsTArray<GfxDriverInfo>& info,
> +                                         nsAString& aVersion,

can you please rename aVersion to aSuggestedVersion.

@@ +619,5 @@
> +PRInt32
> +GfxInfoBase::FindBlocklistedDeviceInList(const nsTArray<GfxDriverInfo>& info,
> +                                         nsAString& aVersion,
> +                                         PRInt32 aFeature,
> +                                         OperatingSystem os)

why does the OS have to be provided, whereas other stuff like device information we query automatically?

Actually, below we are doing some automatic OS detection using the preprocessor.
Attachment #573143 - Flags: review?(bjacob) → review-
Addressed code review comments plus some small mistakes I noticed.
Attachment #573143 - Attachment is obsolete: true
Attachment #573997 - Flags: review?(bjacob)
Attachment #573997 - Flags: review?(bjacob) → review+
Backed out again in, we're hoping this was the one that was causing Android R3 to die with a decidedly unhelpful "TEST-UNEXPECTED-FAIL | | exception while running reftests" while running scrolling/opacity-mixed-scrolling-1.html
Blocks: 704710
Fixed reftest failure.
Attachment #573997 - Attachment is obsolete: true
Attachment #576386 - Flags: review?(bjacob)
Try push:

Note that this was pushed to try with 704710, so they should be landed together.
No longer blocks: 668008
Blocks: 706702
Try push:

Updated push with other stuff.
Attachment #576386 - Flags: review?(bjacob) → review+
Closed: 9 years ago
Resolution: --- → FIXED

(Please post actual cset and set target milestone when landing. Thanks :-))
Target Milestone: --- → mozilla11
Depends on: 704143
Depends on: 711656
You need to log in before you can comment on or make changes to this bug.