Last Comment Bug 699482 - Talos complains about GfxDriverInfo static constructors, they might slow down startup
: Talos complains about GfxDriverInfo static constructors, they might slow down...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on: 704143 711656
Blocks: 704710 706702
  Show dependency treegraph
 
Reported: 2011-11-03 11:10 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-03-10 21:27 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0, refactor static constructors and other things. (57.90 KB, patch)
2011-11-09 02:20 PST, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch v1.1, refactor static constructors and other things. (60.47 KB, patch)
2011-11-11 20:36 PST, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch v1.2, refactor static constructors and other things. (59.64 KB, patch)
2011-11-22 18:56 PST, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-11-03 11:10:46 PDT
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] 2011-11-03 11:18:34 PDT
Yes.
Comment 2 Doug Sherk (:drs) (inactive) 2011-11-03 12:12:24 PDT
I agree, that sounds like a good idea. I will do this.
Comment 3 Doug Sherk (:drs) (inactive) 2011-11-09 02:20:41 PST
Created attachment 573143 [details] [diff] [review]
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) 2011-11-09 02:26:30 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=fc40e38753d2
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-11-11 13:07:50 PST
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) 2011-11-11 20:36:50 PST
Created attachment 573997 [details] [diff] [review]
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) 2011-11-11 20:40:20 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=15d1003ad8c2
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-11-18 20:01:03 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/515736d18227
Comment 9 Phil Ringnalda (:philor, back in August) 2011-11-18 22:53:59 PST
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) 2011-11-22 18:56:01 PST
Created attachment 576386 [details] [diff] [review]
Patch v1.2, refactor static constructors and other things.

Fixed reftest failure.
Comment 11 Doug Sherk (:drs) (inactive) 2011-11-22 18:57:34 PST
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) 2011-12-01 21:48:05 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=abe278c327db

Updated push with other stuff.
Comment 13 Doug Sherk (:drs) (inactive) 2011-12-15 00:27:12 PST
https://tbpl.mozilla.org/?rev=fe937bac6e75
Comment 14 Ed Morley [:emorley] 2011-12-15 01:05:12 PST
https://hg.mozilla.org/mozilla-central/rev/f9203039a956

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

Note You need to log in before you can comment on or make changes to this bug.