Closed Bug 1214802 Opened 4 years ago Closed 4 years ago

gfxEnv - consolidate all environment variable checking related to graphics in one place

Categories

(Core :: Graphics, enhancement)

43 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: milan, Assigned: milan)

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(1 file, 3 obsolete files)

Milan Sreckovic <msreckovic@mozilla.com> wrote:
> Do we have a place where we track different environment variables that we
> use for special behaviours, triage, debugging, etc., related to graphics?
> Things like MOZ_GL_SPEW or LIBGL_ALWAYS_INDIRECT and I’m sure others.

Benoit Girard <bgirard@mozilla.com> wrote:
> I feel like gfxPrefs.h is pretty handy organization tool. Maybe gfxEnv.h
> would be a handy addition to wrap and organize all the getenv() calls. It's
> always best to generate documentation from the code. The documentation
> could simply point to that header in this case.

Vladimir Vukicevic <vladimir@mozilla.com> wrote:
> Yeah, I like gfxEnv, especially since it could actually cache results instead
> of various pieces of code needing to do that. Would then let us document
> everything in one header file, and get rid of the scattered getenv calls.
Whiteboard: [gfx-noted]
Assignee: nobody → milan
Simple start, just for the cases where we check the existence.
No attempt to consolidate the four #defines at this point, that can come later.  Tried to comment on as many as possible.
Attachment #8679782 - Attachment is obsolete: true
Attachment #8679797 - Flags: review?(botond)
Comment on attachment 8679797 [details] [diff] [review]
Envvar "existence" consolidation in gfxEnv. r=botond

Review of attachment 8679797 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed (particularly the one about paint dumping; the rest are just cosmetic suggestions). Thanks!

::: gfx/thebes/gfxEnv.h
@@ +25,5 @@
> +
> +#define DECL_GFX_ENV_ONCE(Env, Name)                \
> +  static bool Name() {                              \
> +    static int m##Name = -1;                        \
> +    if (m##Name < 0) {                              \

You can avoid doing the bookkeeping manually like so:

// Helper function, can be re-used in the other macros
static bool IsEnvSet(const char* aName) {
  char* val = PR_GetEnv(aName);
  return (val != 0 && *val != '\0');
}

#define DECL_GFX_ENV_ONCE(Env, Name)          \
  static bool Name() {                        \
    static bool s##Name = IsEnvSet(Name);     \
    return s##Name;                           \
  }

The variable initialization will only happen on first call. (The runtime cost will probably be the same, as the compiler still needs to emit code to check if it's the first call, it's just hidden now.)

@@ +59,5 @@
> +    return false;                                   \
> +  }
> +#endif
> +
> +class gfxEnv;

Don't need this line.

@@ +64,5 @@
> +class gfxEnv final
> +{
> +public:
> +  // This is where DECL_GFX_ENV for each of the environment variables should go.
> +  // We will keep these in an alphabetical order to make it easier to see if

(Maybe mention that the alpha order is by env var name, not accessor name. Matters for some things like ("MOZ_GFX_VR_NO_DISTORTION", VRNoDistortion)).

@@ +91,5 @@
> +
> +  // Dumping the layer list in LayerSorter
> +  DECL_GFX_ENV_ONCE("MOZ_DUMP_LAYER_SORT_LIST", DumpLayerSortList);
> +
> +  // Paint dumping, only in DEBUG

Previously these were #ifdef MOZ_DUMP_PAINTING, which can be defined even without DEBUG (e.g. if --enable-dump-painting is passed to configure). As currently written, the patch would disable paint dumping on configurations that have MOZ_DUMP_PAINTING but not DEBUG.

As the code that actually does the paint dumping is also #ifdef MOZ_DUMP_PAINTING, we should be able to change these env vars to non-debug and not suffer any performance penalty.

(This applies to DumpCompositorTextures too.)

::: gfx/thebes/gfxPlatform.cpp
@@ +315,2 @@
>  #else
> +  // Release builds use telemetry bu default, but will crash instead

Fix typo (bu -> by) since you're touching this line anyways.
Attachment #8679797 - Flags: review?(botond) → review+
Bizarre, I completely read #ifdef MOZ_PAINT_DUMPING inside of gfxUtils as #ifdef DEBUG.  Yikes.
Comment on attachment 8680155 [details] [diff] [review]
Envvar "existence" consolidation in gfxEnv. Carry r=botond

Review of attachment 8680155 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxEnv.h
@@ +117,5 @@
> +  gfxEnv(const gfxEnv&) = delete;
> +  gfxEnv& operator=(const gfxEnv&) = delete;
> +};
> +
> +#undef DECL_GFX_ENV /* Don't need it outside of this file */

You probably mean:

#undef DECL_GFX_ENV_LIVE
#undef DECL_GFX_ENV_ONCE
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
https://hg.mozilla.org/mozilla-central/rev/e9b1d8eb222f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.