slow detection of GL extensions on some hardware

tobing

Hi, recently I switched my setup to use a 4k monitor next to the laptop, desktop is extended to two screens. You know, home office and such things.

I found that OpenGL usage becomes really slow during startup, so I did some debugging. The slow function is

is_wgl_extension_supported()
{
   // some code
   _wglGetExtensionsStringARB = (_ALLEGRO_wglGetExtensionsStringARB_t)
      wglGetProcAddress("wglGetExtensionsStringARB");
   // more code
}

and some google search revealed, that it seems to be a known problem for some graphics cards and some drivers, but unknown reasons, that the detection of gl extensions is really slow. Nonetheless, somebody hinted to not call this in a loop anyways, but that's what allegro is doing in

read_pixel_format_ext

called by

get_available_pixel_formats_ext()
{
   // some code
   for (j = i = 0; i < maxindex; i++) {
      ALLEGRO_INFO("-- \n");
      ALLEGRO_INFO("Decoding visual no. %i...\n", i+1);
      eds_list[j] = read_pixel_format_ext(i, testdc);
      // more code
   }
   // more code
}

so I changed is_wgl_extension_supported to get the proc address only once and reuse it:

#SelectExpand
  1static bool is_wgl_extension_supported(const char *extension, HDC dc)
  2{
  3   static _ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB;
  4   static ext_is_initialized = false; // this is a simple blocker, would not be safe for multi-threading
  5   int ret;
  6
  7   /* XXX deprecated in OpenGL 3.0 */
  8   if (!glGetString(GL_EXTENSIONS))
  9      return false;
 10
 11   if(!ext_is_initialized)
 12   {
 13      _wglGetExtensionsStringARB = (_ALLEGRO_wglGetExtensionsStringARB_t)
 14         wglGetProcAddress("wglGetExtensionsStringARB");
 15      ext_is_initialized = true;
 16   }
 17   if (!_wglGetExtensionsStringARB)
 18      return false;
 19
 20   ret = _al_ogl_look_for_an_extension(extension,
 21         (const GLubyte*)_wglGetExtensionsStringARB(dc));
 22
 23   return ret;
 24}

but I'm not sure, iff that would work for all compilers, because it's not old-style-enough, I guess. I'm really much going modern C++ these days, so I forgot how to write it properly for allegro...

And it does the trick, now there's less than a second and allegro is up. As they said in the but report I found, that is still WAY too long, as long as it's not called in a loop, I can live with it.

So maybe one of the Windows / OpenGL experts can apply this or a similar change to wgl_disp.c? Or even other places with similar patterns?

SiegeLord

Looks good to me, let's ship it (https://github.com/liballeg/allegro5/issues/1125).

tobing

Here's a patch, I haven't forked allegro (yet), so I think I can't just create a pull request.

Thanks in advance!

#SelectExpand
  1From 4d45eb36f0873f48672f02328693872c86a55815 Mon Sep 17 00:00:00 2001
  2From: Tobias Scheuer <tobias@scheuer42.de>
  3Date: Sun, 29 Mar 2020 12:24:59 +0200
  4Subject: [PATCH] initialize wgl extension string only once
  5
  6---
  7 src/win/wgl_disp.c | 11 ++++++++---
  8 1 file changed, 8 insertions(+), 3 deletions(-)
  9
 10diff --git a/src/win/wgl_disp.c b/src/win/wgl_disp.c
 11index b8e0878fc..893d56041 100644
 12--- a/src/win/wgl_disp.c
 13+++ b/src/win/wgl_disp.c
 14@@ -68,15 +68,20 @@ typedef struct WGL_DISPLAY_PARAMETERS {
 15 
 16 static bool is_wgl_extension_supported(const char *extension, HDC dc)
 17 {
 18-   _ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB;
 19+   static _ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB;
 20+   static ext_is_initialized = false; // this is a simple blocker, would not be safe for multi-threading
 21    int ret;
 22 
 23    /* XXX deprecated in OpenGL 3.0 */
 24    if (!glGetString(GL_EXTENSIONS))
 25       return false;
 26 
 27-   _wglGetExtensionsStringARB = (_ALLEGRO_wglGetExtensionsStringARB_t)
 28-      wglGetProcAddress("wglGetExtensionsStringARB");
 29+   if(!ext_is_initialized)
 30+   {
 31+       _wglGetExtensionsStringARB = (_ALLEGRO_wglGetExtensionsStringARB_t)
 32+           wglGetProcAddress("wglGetExtensionsStringARB");
 33+       ext_is_initialized = true;
 34+   }
 35    if (!_wglGetExtensionsStringARB)
 36       return false;
 37 
 38-- 
 392.24.1.windows.2

Elias

I think in theory a different OpenGL context could return a different address - so we'd need a separate _wglGetExtensionsStringARB for each thread and clear it when the context gets changed on that thread.

If it doesn't break any of the examples it's probably good enough though for now - but maybe at least should have a // TODO/FIXME comment about what it does.

tobing

This is called in a loop, so maybe the context could be a parameter... just a thought for now, I'll have a look at the code tomorrow and maybe come up with a better patch.

Edit: I have added a parameter to some of the functions, like in attached patch. Now there are more changed lines, but no assumptions about threads and such. I guess that this is better, and performance seems to be quite good as well, maybe even more than the first change.

#SelectExpand
   1From 276d7889c9b3bebdcd538a694154bde9c3600000 Mon Sep 17 00:00:00 2001
   2From: Tobias Scheuer <tobias@scheuer42.de>
   3Date: Sun, 29 Mar 2020 12:24:59 +0200
   4Subject: [PATCH] avoid multiple wgl extension string initialization
   5
   6---
   7 src/win/wgl_disp.c | 48 +++++++++++++++++++++++-----------------------
   8 1 file changed, 24 insertions(+), 24 deletions(-)
   9
  10diff --git a/src/win/wgl_disp.c b/src/win/wgl_disp.c
  11index b8e0878fc..d6972265f 100644
  12--- a/src/win/wgl_disp.c
  13+++ b/src/win/wgl_disp.c
  14@@ -66,17 +66,13 @@ typedef struct WGL_DISPLAY_PARAMETERS {
  15    const char* window_title;
  16 } WGL_DISPLAY_PARAMETERS;
  17 
  18-static bool is_wgl_extension_supported(const char *extension, HDC dc)
  19+static bool is_wgl_extension_supported(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, const char *extension, HDC dc)
  20 {
  21-   _ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB;
  22    int ret;
  23 
  24    /* XXX deprecated in OpenGL 3.0 */
  25    if (!glGetString(GL_EXTENSIONS))
  26       return false;
  27-
  28-   _wglGetExtensionsStringARB = (_ALLEGRO_wglGetExtensionsStringARB_t)
  29-      wglGetProcAddress("wglGetExtensionsStringARB");
  30    if (!_wglGetExtensionsStringARB)
  31       return false;
  32 
  33@@ -441,7 +437,7 @@ static ALLEGRO_EXTRA_DISPLAY_SETTINGS* read_pixel_format_old(int fmt, HDC dc)
  34 }
  35 
  36 
  37-static ALLEGRO_EXTRA_DISPLAY_SETTINGS* read_pixel_format_ext(int fmt, HDC dc)
  38+static ALLEGRO_EXTRA_DISPLAY_SETTINGS* read_pixel_format_ext(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, int fmt, HDC dc)
  39 {
  40    ALLEGRO_EXTRA_DISPLAY_SETTINGS *eds = NULL;
  41 
  42@@ -493,11 +489,11 @@ static ALLEGRO_EXTRA_DISPLAY_SETTINGS* read_pixel_format_ext(int fmt, HDC dc)
  43       return NULL;
  44 
  45    /* If multisampling is supported, query for it. */
  46-   if (is_wgl_extension_supported("WGL_ARB_multisample", dc)) {
  47+   if (is_wgl_extension_supported(_wglGetExtensionsStringARB, "WGL_ARB_multisample", dc)) {
  48       attrib[num_attribs - 3] = WGL_SAMPLE_BUFFERS_ARB;
  49       attrib[num_attribs - 2] = WGL_SAMPLES_ARB;
  50    }
  51-   if (is_wgl_extension_supported("WGL_EXT_depth_float", dc)) {
  52+   if (is_wgl_extension_supported(_wglGetExtensionsStringARB, "WGL_EXT_depth_float", dc)) {
  53       attrib[num_attribs - 1] = WGL_DEPTH_FLOAT_EXT;
  54    }
  55 
  56@@ -623,7 +619,7 @@ static bool change_display_mode(ALLEGRO_DISPLAY *d)
  57 }
  58 
  59 
  60-static HGLRC init_ogl_context_ex(HDC dc, bool fc, int major, int minor)
  61+static HGLRC init_ogl_context_ex(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, HDC dc, bool fc, int major, int minor)
  62 {
  63    HWND testwnd = NULL;
  64    HDC testdc   = NULL;
  65@@ -644,7 +640,7 @@ static HGLRC init_ogl_context_ex(HDC dc, bool fc, int major, int minor)
  66    if (!testrc)
  67       goto bail;
  68 
  69-   if (is_wgl_extension_supported("WGL_ARB_create_context", testdc)) {
  70+   if(is_wgl_extension_supported(_wglGetExtensionsStringARB, "WGL_ARB_create_context", testdc)) {
  71       int attrib[] = {WGL_CONTEXT_MAJOR_VERSION_ARB, major,
  72                       WGL_CONTEXT_MINOR_VERSION_ARB, minor,
  73                       WGL_CONTEXT_FLAGS_ARB, 0,
  74@@ -678,7 +674,7 @@ bail:
  75 }
  76 
  77 
  78-static ALLEGRO_EXTRA_DISPLAY_SETTINGS** get_available_pixel_formats_ext(int *count)
  79+static ALLEGRO_EXTRA_DISPLAY_SETTINGS** get_available_pixel_formats_ext(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, int *count)
  80 {
  81    HWND testwnd = NULL;
  82    HDC testdc   = NULL;
  83@@ -708,8 +704,8 @@ static ALLEGRO_EXTRA_DISPLAY_SETTINGS** get_available_pixel_formats_ext(int *cou
  84    if (!testrc)
  85       goto bail;
  86 
  87-  if (!is_wgl_extension_supported("WGL_ARB_pixel_format", testdc) &&
  88-      !is_wgl_extension_supported("WGL_EXT_pixel_format", testdc)) {
  89+   if(!is_wgl_extension_supported(_wglGetExtensionsStringARB, "WGL_ARB_pixel_format", testdc) &&
  90+      !is_wgl_extension_supported(_wglGetExtensionsStringARB, "WGL_EXT_pixel_format", testdc)) {
  91       ALLEGRO_ERROR("WGL_ARB/EXT_pf not supported.\n");
  92       goto bail;
  93   }
  94@@ -730,7 +726,7 @@ static ALLEGRO_EXTRA_DISPLAY_SETTINGS** get_available_pixel_formats_ext(int *cou
  95    for (j = i = 0; i < maxindex; i++) {
  96       ALLEGRO_INFO("-- \n");
  97       ALLEGRO_INFO("Decoding visual no. %i...\n", i+1);
  98-      eds_list[j] = read_pixel_format_ext(i, testdc);
  99+      eds_list[j] = read_pixel_format_ext(_wglGetExtensionsStringARB, i, testdc);
 100       if (!eds_list[j])
 101          continue;
 102       // Fill vsync setting here and enable/disable it after display creation
 103@@ -817,7 +813,7 @@ static ALLEGRO_EXTRA_DISPLAY_SETTINGS** get_available_pixel_formats_old(int *cou
 104 }
 105 
 106 
 107-static bool select_pixel_format(ALLEGRO_DISPLAY_WGL *d, HDC dc)
 108+static bool select_pixel_format(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, ALLEGRO_DISPLAY_WGL *d, HDC dc)
 109 {
 110    ALLEGRO_EXTRA_DISPLAY_SETTINGS **eds = NULL;
 111    ALLEGRO_CONFIG *sys_cfg = al_get_system_config();
 112@@ -838,7 +834,7 @@ static bool select_pixel_format(ALLEGRO_DISPLAY_WGL *d, HDC dc)
 113    }
 114 
 115    if (!force_old)
 116-      eds = get_available_pixel_formats_ext(&eds_count);
 117+       eds = get_available_pixel_formats_ext(_wglGetExtensionsStringARB, &eds_count);
 118    if (!eds)
 119       eds = get_available_pixel_formats_old(&eds_count, dc);
 120 
 121@@ -879,7 +875,7 @@ static bool select_pixel_format(ALLEGRO_DISPLAY_WGL *d, HDC dc)
 122    return true;
 123 }
 124 
 125-static bool create_display_internals(ALLEGRO_DISPLAY_WGL *wgl_disp)
 126+static bool create_display_internals(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, ALLEGRO_DISPLAY_WGL *wgl_disp)
 127 {
 128    ALLEGRO_DISPLAY     *disp     = (void*)wgl_disp;
 129    ALLEGRO_DISPLAY_WIN *win_disp = (void*)wgl_disp;
 130@@ -921,7 +917,7 @@ static bool create_display_internals(ALLEGRO_DISPLAY_WGL *wgl_disp)
 131    /* WGL display lists cannot be shared with the API currently in use. */
 132    disp->ogl_extras->is_shared = false;
 133 
 134-   if (!select_pixel_format(wgl_disp, wgl_disp->dc)) {
 135+   if(!select_pixel_format(_wglGetExtensionsStringARB, wgl_disp, wgl_disp->dc)) {
 136       destroy_display_internals(wgl_disp);
 137       return false;
 138    }
 139@@ -934,7 +930,7 @@ static bool create_display_internals(ALLEGRO_DISPLAY_WGL *wgl_disp)
 140       if (major == 0)
 141          major = 3;
 142       bool fc = (disp->flags & ALLEGRO_OPENGL_FORWARD_COMPATIBLE) != 0;
 143-      wgl_disp->glrc = init_ogl_context_ex(wgl_disp->dc, fc, major,
 144+      wgl_disp->glrc = init_ogl_context_ex(_wglGetExtensionsStringARB, wgl_disp->dc, fc, major,
 145          minor);
 146    }
 147    else {
 148@@ -1024,7 +1020,9 @@ static ALLEGRO_DISPLAY* wgl_create_display(int w, int h)
 149 
 150    display->ogl_extras = al_calloc(1, sizeof(ALLEGRO_OGL_EXTRAS));
 151 
 152-   if (!create_display_internals(wgl_display)) {
 153+   _ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB
 154+      = (_ALLEGRO_wglGetExtensionsStringARB_t)wglGetProcAddress("wglGetExtensionsStringARB");
 155+   if(!create_display_internals(_wglGetExtensionsStringARB, wgl_display)) {
 156       al_free(display->ogl_extras);
 157       al_free(display);
 158       return NULL;
 159@@ -1331,7 +1329,7 @@ static void wgl_update_display_region(ALLEGRO_DISPLAY *d,
 160 }
 161 
 162 
 163-static bool wgl_resize_helper(ALLEGRO_DISPLAY *d, int width, int height)
 164+static bool wgl_resize_helper(_ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB, ALLEGRO_DISPLAY *d, int width, int height)
 165 {
 166    ALLEGRO_DISPLAY_WGL *wgl_disp = (ALLEGRO_DISPLAY_WGL *)d;
 167    ALLEGRO_DISPLAY_WIN *win_disp = (ALLEGRO_DISPLAY_WIN *)d;
 168@@ -1380,7 +1378,7 @@ static bool wgl_resize_helper(ALLEGRO_DISPLAY *d, int width, int height)
 169 
 170       d->w = width;
 171       d->h = height;
 172-      if (!create_display_internals(wgl_disp))
 173+      if(!create_display_internals(_wglGetExtensionsStringARB, wgl_disp))
 174          return false;
 175 
 176       /* We have a new backbuffer now. */
 177@@ -1435,8 +1433,10 @@ static bool wgl_resize_display(ALLEGRO_DISPLAY *d, int width, int height)
 178 
 179    win_display->ignore_resize = true;
 180 
 181-   if (!wgl_resize_helper(d, width, height)) {
 182-      wgl_resize_helper(d, orig_w, orig_h);
 183+   _ALLEGRO_wglGetExtensionsStringARB_t _wglGetExtensionsStringARB
 184+      = (_ALLEGRO_wglGetExtensionsStringARB_t)wglGetProcAddress("wglGetExtensionsStringARB");
 185+   if(!wgl_resize_helper(_wglGetExtensionsStringARB, d, width, height)) {
 186+      wgl_resize_helper(_wglGetExtensionsStringARB, d, orig_w, orig_h);
 187       ret = false;
 188    } else {
 189       ret = true;
 190-- 
 1912.24.1.windows.2

Thread #618076. Printed from Allegro.cc