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

RESOLVED FIXED in mozilla11

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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?
Yes.
(Assignee)

Comment 2

6 years ago
I agree, that sounds like a good idea. I will do this.
Assignee: nobody → dsherk
(Assignee)

Comment 3

6 years ago
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.
Attachment #573143 - Flags: review?(bjacob)
(Assignee)

Comment 4

6 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=fc40e38753d2
(Reporter)

Comment 5

6 years ago
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.
Attachment #573143 - Flags: review?(bjacob) → review-
(Assignee)

Comment 6

6 years ago
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.
Attachment #573143 - Attachment is obsolete: true
Attachment #573997 - Flags: review?(bjacob)
(Assignee)

Comment 7

6 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=15d1003ad8c2
(Reporter)

Updated

6 years ago
Attachment #573997 - Flags: review?(bjacob) → review+
(Reporter)

Comment 8

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/515736d18227
Status: NEW → ASSIGNED
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
(Assignee)

Updated

6 years ago
Blocks: 704710
(Assignee)

Comment 10

6 years ago
Created attachment 576386 [details] [diff] [review]
Patch v1.2, refactor static constructors and other things.

Fixed reftest failure.
Attachment #573997 - Attachment is obsolete: true
Attachment #576386 - Flags: review?(bjacob)
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Updated

6 years ago
No longer blocks: 668008
(Assignee)

Updated

6 years ago
Blocks: 706702
(Assignee)

Comment 12

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

Updated push with other stuff.
(Reporter)

Updated

6 years ago
Attachment #576386 - Flags: review?(bjacob) → review+
(Assignee)

Comment 13

6 years ago
https://tbpl.mozilla.org/?rev=fe937bac6e75
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/f9203039a956

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

Updated

6 years ago
Depends on: 704143

Updated

6 years ago
Depends on: 711656
You need to log in before you can comment on or make changes to this bug.