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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This changeset:

   http://hg.mozilla.org/integration/mozilla-inbound/rev/1f1d6394148c

gives:

   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?

Comment 1

[Mike Hommey [:glandium]]

Yes.

Comment 2

[Doug Sherk (:drs) (inactive)]

I agree, that sounds like a good idea. I will do this.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, refactor static constructors and other things.

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.

Comment 4

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=fc40e38753d2

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

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:
  FEATURE_NO_INFO  -> FEATURE_STATUS_OK
  FEATURE_BEING_PROCESSED -> FEATURE_STATUS_UNKNOWN

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];

.Elements()

::: 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.

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, refactor static constructors and other things.

Addressed code review comments plus some small mistakes I noticed.

Comment 7

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=15d1003ad8c2

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/515736d18227

Comment 9

[Phil Ringnalda (:philor)]

Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa94798c5728, 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

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, refactor static constructors and other things.

Fixed reftest failure.

Comment 11

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=8ca6453ee424

Note that this was pushed to try with 704710, so they should be landed together.

Comment 12

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=abe278c327db

Updated push with other stuff.

Comment 13

[Doug Sherk (:drs) (inactive)]

https://tbpl.mozilla.org/?rev=fe937bac6e75

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9203039a956

(Please post actual cset and set target milestone when landing. Thanks :-))