Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance - Replaced glGetError with glDebugMessageCallback #16984

Merged
merged 1 commit into from Oct 5, 2019

Conversation

@vesuvian
Copy link
Contributor

vesuvian commented Aug 25, 2019

Some cursive profiling showed that a bunch of time was being spent calling glGetError about 1000 times a second on startup. Replacing this with a single callback to glDebugMessageCallback has entirely removed those calls and has simplified a lot of existing code. Further, glDebugMessageCallback will catch any cases where previously someone may have forgotten to call glGetError

@vesuvian vesuvian changed the title perf: Replaced expensive calls to glGetError with a single subscripti… Performance - Replaced glGetError with glDebugMessageCallback Aug 25, 2019
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 25, 2019

This is a great idea, but has a major problem that glDebugMessageCallback is an OpenGL 4.3 addition, while OpenRA is currently limited to GL 2.1 + framebuffer extensions for compatibility. This crashes for me on startup because my system (and anyone else running macOS) only supports GL 4.1.

We are planning to migrate our rendering backend over to https://github.com/mellinoe/veldrid within the next couple of releases, which should win us some significant performance improvements by using DX/Vulkan/Metal instead.

@vesuvian

This comment has been minimized.

Copy link
Contributor Author

vesuvian commented Aug 25, 2019

Ahh that's a shame, I thought I was on to something! Thanks for the clarification.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 30, 2019

It will still be good to use this on systems that do support it, which we can do by adding a DebugMessagesCallback flag to the GLFeatures enum, setting it if SDL.SDL_GL_ExtensionSupported("KHR_debug") == SDL.SDL_bool.SDL_TRUE, and changing CheckGLError to be a no-op if the flag is set rather than removing it completely.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 30, 2019

I tested this on a couple of machines that support the debug extension, but it doesn't trigger when I intentionally try to do something bogus. https://www.khronos.org/opengl/wiki/Example/OpenGL_Error_Testing_with_Message_Callbacks suggests that we need to activate this with a glEnable(GL_DEBUG_OUTPUT).

@vesuvian vesuvian force-pushed the vesuvian:perf/glerror branch from 5e0c917 to cf9379a Sep 1, 2019
@vesuvian

This comment has been minimized.

Copy link
Contributor Author

vesuvian commented Sep 2, 2019

Turns out the enums for debugging are entirely different to the error enums, so I ended up with two slightly different paths for logging.

For now I have it set to log and except on anything with medium or high severity:
https://i.gyazo.com/ea6e585c689e3f0fea570d4c6a5f8c8b.png

Generated logs look something like:
High - GL Debug API Output: Error - GL_INVALID_ENUM error generated. at OpenRA.Platforms.Default.OpenGL.DebugMessageHandler(Int32 source, Int32 type, UInt32 id, Int32 severity, Int32 length, StringBuilder message, IntPtr userparam) at OpenRA.Platforms.Default.OpenGL.Initialize() at OpenRA.Platforms.Default.Sdl2GraphicsContext.InitializeOpenGL() at OpenRA.Platforms.Default.Sdl2PlatformWindow..ctor(Size requestWindowSize, WindowMode windowMode, Int32 batchSize) at OpenRA.Platforms.Default.DefaultPlatform.CreateWindow(Size size, WindowMode windowMode, Int32 batchSize) at OpenRA.Renderer..ctor(IPlatform platform, GraphicSettings graphicSettings) at OpenRA.Game.Initialize(Arguments args) at OpenRA.Game.InitializeAndRun(String[] args) at OpenRA.Program.Main(String[] args)

I was able to play through a mission level without any problems.

@vesuvian vesuvian force-pushed the vesuvian:perf/glerror branch 2 times, most recently from 4e211a6 to b07f2da Sep 2, 2019
@RoosterDragon

This comment has been minimized.

Copy link
Member

RoosterDragon commented Sep 13, 2019

Could you rebase this to account for #17010?

I'm interested in this - testing on my machine throws a couple of errors on startup - so we might need to get this tested on a decent variety of drivers and squash any latent bugs it is picking up.

@vesuvian vesuvian force-pushed the vesuvian:perf/glerror branch from b07f2da to c2d35f0 Sep 14, 2019
@vesuvian

This comment has been minimized.

Copy link
Contributor Author

vesuvian commented Sep 14, 2019

@RoosterDragon rebased!

@vesuvian vesuvian force-pushed the vesuvian:perf/glerror branch from c2d35f0 to 34417f2 Sep 14, 2019
@RoosterDragon

This comment has been minimized.

Copy link
Member

RoosterDragon commented Sep 14, 2019

No high or mediums under NVIDIA, no high under Intel, but this medium on startup for Intel:

Medium - GL Debug API Output: Performance - API_ID_RECOMPILE_FRAGMENT_SHADER performance warning has been generated. Fragment shader recompiled due to state change.
   at OpenRA.Platforms.Default.OpenGL.DebugMessageHandler(Int32 source, Int32 type, UInt32 id, Int32 severity, Int32 length, StringBuilder message, IntPtr userparam)
   at OpenRA.Platforms.Default.Sdl2GraphicsContext.DrawPrimitives(PrimitiveType pt, Int32 firstVertex, Int32 numVertices)
   at OpenRA.Renderer.DrawBatch[T](IVertexBuffer`1 vertices, Int32 firstVertex, Int32 numVertices, PrimitiveType type)
   at OpenRA.Renderer.DrawBatch(Vertex[] vertices, Int32 numVertices, PrimitiveType type)
   at OpenRA.Graphics.SpriteRenderer.Flush()
   at OpenRA.Renderer.set_CurrentBatchRenderer(IBatchRenderer value)
   at OpenRA.Renderer.Flush()
   at OpenRA.Renderer.EndFrame(IInputHandler inputHandler)
   at OpenRA.Mods.Common.LoadScreens.LogoStripeLoadScreen.Display()
   at OpenRA.ModData.PrepareMap(String uid)
   at OpenRA.Game.StartGame(String mapUID, WorldType type)
   at OpenRA.Game.LoadShellMap()
   at OpenRA.Mods.Common.LoadScreens.BlankLoadScreen.StartGame(Arguments args)
   at OpenRA.Game.InitializeMod(String mod, Arguments args)
   at OpenRA.Game.Initialize(Arguments args)
   at OpenRA.Game.InitializeAndRun(String[] args)
   at OpenRA.Program.Main(String[] args)
OpenGL Information - Intel(R) HD Graphics 4600
OpenGL Information:
Vendor: Intel
Renderer: Intel(R) HD Graphics 4600
GL Version: 3.2.0 - Build 20.19.15.4835
Shader Version: 1.50 - Build 20.19.15.4835
Available extensions:
GL_EXT_blend_minmax
GL_EXT_blend_subtract
GL_EXT_blend_color
GL_EXT_abgr
GL_EXT_texture3D
GL_EXT_clip_volume_hint
GL_EXT_compiled_vertex_array
GL_SGIS_texture_edge_clamp
GL_SGIS_generate_mipmap
GL_EXT_draw_range_elements
GL_SGIS_texture_lod
GL_EXT_rescale_normal
GL_EXT_packed_pixels
GL_EXT_texture_edge_clamp
GL_EXT_separate_specular_color
GL_ARB_multitexture
GL_ARB_map_buffer_alignment
GL_ARB_conservative_depth
GL_EXT_texture_env_combine
GL_EXT_bgra
GL_EXT_blend_func_separate
GL_EXT_secondary_color
GL_EXT_fog_coord
GL_EXT_texture_env_add
GL_ARB_texture_cube_map
GL_ARB_transpose_matrix
GL_ARB_internalformat_query
GL_ARB_internalformat_query2
GL_ARB_texture_env_add
GL_IBM_texture_mirrored_repeat
GL_ARB_texture_mirrored_repeat
GL_EXT_multi_draw_arrays
GL_SUN_multi_draw_arrays
GL_NV_blend_square
GL_ARB_texture_compression
GL_3DFX_texture_compression_FXT1
GL_EXT_texture_filter_anisotropic
GL_ARB_texture_border_clamp
GL_ARB_point_parameters
GL_ARB_texture_env_combine
GL_ARB_texture_env_dot3
GL_ARB_texture_env_crossbar
GL_EXT_texture_compression_s3tc
GL_ARB_shadow
GL_ARB_window_pos
GL_EXT_shadow_funcs
GL_EXT_stencil_wrap
GL_ARB_vertex_program
GL_EXT_texture_rectangle
GL_ARB_fragment_program
GL_EXT_stencil_two_side
GL_ATI_separate_stencil
GL_ARB_vertex_buffer_object
GL_EXT_texture_lod_bias
GL_ARB_occlusion_query
GL_ARB_fragment_shader
GL_ARB_shader_objects
GL_ARB_shading_language_100
GL_ARB_texture_non_power_of_two
GL_ARB_vertex_shader
GL_NV_texgen_reflection
GL_ARB_point_sprite
GL_ARB_fragment_program_shadow
GL_EXT_blend_equation_separate
GL_ARB_depth_texture
GL_ARB_texture_rectangle
GL_ARB_draw_buffers
GL_ARB_color_buffer_float
GL_ARB_half_float_pixel
GL_ARB_texture_float
GL_ARB_pixel_buffer_object
GL_ARB_texture_barrier
GL_EXT_framebuffer_object
GL_ARB_draw_instanced
GL_ARB_half_float_vertex
GL_ARB_occlusion_query2
GL_EXT_draw_buffers2
GL_WIN_swap_hint
GL_EXT_texture_sRGB
GL_ARB_multisample
GL_EXT_packed_float
GL_EXT_texture_shared_exponent
GL_ARB_texture_rg
GL_ARB_texture_compression_rgtc
GL_NV_conditional_render
GL_ARB_texture_swizzle
GL_EXT_texture_swizzle
GL_ARB_texture_gather
GL_ARB_sync
GL_ARB_cl_event
GL_ARB_framebuffer_sRGB
GL_EXT_packed_depth_stencil
GL_ARB_depth_buffer_float
GL_EXT_transform_feedback
GL_ARB_transform_feedback2
GL_ARB_draw_indirect
GL_EXT_framebuffer_blit
GL_EXT_framebuffer_multisample
GL_ARB_framebuffer_object
GL_ARB_framebuffer_no_attachments
GL_EXT_texture_array
GL_EXT_texture_integer
GL_ARB_map_buffer_range
GL_ARB_texture_buffer_range
GL_EXT_texture_snorm
GL_ARB_blend_func_extended
GL_INTEL_performance_query
GL_ARB_copy_buffer
GL_ARB_sampler_objects
GL_NV_primitive_restart
GL_ARB_seamless_cube_map
GL_ARB_seamless_cubemap_per_texture
GL_ARB_uniform_buffer_object
GL_ARB_depth_clamp
GL_ARB_vertex_array_bgra
GL_ARB_shader_bit_encoding
GL_ARB_draw_buffers_blend
GL_ARB_geometry_shader4
GL_EXT_geometry_shader4
GL_ARB_texture_query_lod
GL_ARB_explicit_attrib_location
GL_ARB_draw_elements_base_vertex
GL_EXT_shader_integer_mix
GL_ARB_instanced_arrays
GL_ARB_base_instance
GL_ARB_fragment_coord_conventions
GL_EXT_gpu_program_parameters
GL_ARB_texture_buffer_object_rgb32
GL_ARB_texture_rgb10_a2ui
GL_ARB_texture_multisample
GL_ARB_vertex_type_2_10_10_10_rev
GL_ARB_vertex_type_10f_11f_11f_rev
GL_ARB_timer_query
GL_EXT_timer_query
GL_ARB_tessellation_shader
GL_ARB_vertex_array_object
GL_ARB_provoking_vertex
GL_ARB_sample_shading
GL_ARB_texture_cube_map_array
GL_EXT_gpu_shader4
GL_ARB_gpu_shader5
GL_ARB_gpu_shader_fp64
GL_INTEL_fragment_shader_ordering
GL_ARB_fragment_shader_interlock
GL_ARB_clip_control
GL_ARB_shader_subroutine
GL_ARB_transform_feedback3
GL_ARB_get_program_binary
GL_ARB_separate_shader_objects
GL_ARB_shader_precision
GL_ARB_vertex_attrib_64bit
GL_ARB_viewport_array
GL_ARB_transform_feedback_instanced
GL_ARB_compressed_texture_pixel_storage
GL_ARB_shader_atomic_counters
GL_ARB_shading_language_packing
GL_ARB_shader_image_load_store
GL_ARB_shading_language_420pack
GL_ARB_texture_storage
GL_EXT_texture_storage
GL_ARB_compute_shader
GL_ARB_vertex_attrib_binding
GL_ARB_texture_view
GL_ARB_fragment_layer_viewport
GL_ARB_multi_draw_indirect
GL_ARB_program_interface_query
GL_ARB_shader_image_size
GL_ARB_shader_storage_buffer_object
GL_ARB_texture_storage_multisample
GL_ARB_buffer_storage
GL_AMD_vertex_shader_layer
GL_AMD_vertex_shader_viewport_index
GL_ARB_query_buffer_object
GL_EXT_polygon_offset_clamp
GL_ARB_clear_texture
GL_ARB_texture_mirror_clamp_to_edge
GL_ARB_debug_output
GL_ARB_enhanced_layouts
GL_KHR_debug
GL_ARB_arrays_of_arrays
GL_ARB_texture_query_levels
GL_ARB_invalidate_subdata
GL_ARB_clear_buffer_object
GL_INTEL_map_texture
GL_ARB_texture_compression_bptc
GL_ARB_ES2_compatibility
GL_ARB_ES3_compatibility
GL_ARB_robustness
GL_ARB_robust_buffer_access_behavior
GL_EXT_texture_sRGB_decode
GL_ARB_copy_image
GL_KHR_blend_equation_advanced
GL_EXT_direct_state_access
GL_ARB_stencil_texturing
GL_ARB_texture_stencil8
GL_ARB_explicit_uniform_location
GL_ARB_multi_bind
GL_ARB_indirect_parameters
WGL_EXT_depth_float
WGL_ARB_buffer_region
WGL_ARB_extensions_string
WGL_ARB_make_current_read
WGL_ARB_pixel_format
WGL_ARB_pbuffer
WGL_EXT_extensions_string
WGL_EXT_swap_control
WGL_EXT_swap_control_tear
WGL_ARB_multisample
WGL_ARB_pixel_format_float
WGL_ARB_framebuffer_sRGB
WGL_ARB_create_context
WGL_ARB_create_context_profile
WGL_EXT_pixel_format_packed_float
WGL_EXT_create_context_es_profile
WGL_EXT_create_context_es2_profile
WGL_NV_DX_interop
WGL_INTEL_cl_sharing
WGL_NV_DX_interop2
WGL_ARB_create_context_robustness
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 14, 2019

@teinarss found another medium under GLES on NVIDIA (see #17087)

Medium - GL Debug API Output: Performance - Pixel-path performance warning: Pixel transfer is synchronized with 3D rendering.

@vesuvian vesuvian force-pushed the vesuvian:perf/glerror branch from a7a742f to d526f95 Sep 14, 2019
@vesuvian

This comment has been minimized.

Copy link
Contributor Author

vesuvian commented Sep 14, 2019

@pchote medium severity is now printed to console

@ghost

This comment has been minimized.

Copy link

ghost commented Sep 14, 2019

Intel(R) HD Graphics 4600

Just on a side note, something is not right with this chip or the drivers for it on Windows. I had to switch to Linux because since 2 releases ago (?) the screen turns black while the game goes on in the background. I have heard from different people that they too have problems with this chip on windows when running OpenRA.

@RoosterDragon

This comment has been minimized.

Copy link
Member

RoosterDragon commented Sep 14, 2019

@matjaeck: I would suggest opening a separate issue with any details you can provide. OS version, driver version and any insight into what causes the problem might be helpful (e.g. when you say running in the background, are you running in windowed mode or alt-tabbing away from a fullscreen window? Does the problem only occur after doing certain things or after certain amounts of time?)

If there are others who can chime in with any info that'd be great too.

Plus the usual advice of check your driver is up to date in case that fixes it.

@vesuvian vesuvian force-pushed the vesuvian:perf/glerror branch from d526f95 to fff756d Sep 14, 2019
@vesuvian

This comment has been minimized.

Copy link
Contributor Author

vesuvian commented Sep 14, 2019

Pushed defensive TryGets

@ghost

This comment has been minimized.

Copy link

ghost commented Sep 14, 2019

@RoosterDragon Sorry, I confused it with the Intel(R) HD Graphics 520. I'll report if the problem persists after updating.

Copy link
Member

RoosterDragon left a comment

👍 Nice work @vesuvian

@ghost

This comment has been minimized.

Copy link

ghost commented Sep 14, 2019

For the record, the latest drivers seem to have fixed my issue. This is the slightly different medium level warning I see in the console on startup:

Medium - GL Debug API Output: Performance - API_ID_RECOMPILE_FRAGMENT_SHADER performance warning has been generated. Fragment shader recompiled due to state change.
   bei OpenRA.Platforms.Default.OpenGL.BuildErrorText(Int32 source, Int32 type, Int32 severity, StringBuilder message)
   bei OpenRA.Platforms.Default.OpenGL.DebugMessageHandler(Int32 source, Int32 type, UInt32 id, Int32 severity, Int32 length, StringBuilder message, IntPtr userparam)
   bei OpenRA.Platforms.Default.Sdl2GraphicsContext.DrawPrimitives(PrimitiveType pt, Int32 firstVertex, Int32 numVertices)
   bei OpenRA.Renderer.DrawBatch[T](IVertexBuffer`1 vertices, Int32 firstVertex, Int32 numVertices, PrimitiveType type)
   bei OpenRA.Graphics.SpriteRenderer.Flush()
   bei OpenRA.Renderer.EndFrame(IInputHandler inputHandler)
   bei OpenRA.Mods.Common.LoadScreens.LogoStripeLoadScreen.Display()
   bei OpenRA.ModData.PrepareMap(String uid)
   bei OpenRA.Game.StartGame(String mapUID, WorldType type)
   bei OpenRA.Game.LoadShellMap()
   bei OpenRA.Mods.Common.LoadScreens.BlankLoadScreen.StartGame(Arguments args)
   bei OpenRA.Game.InitializeMod(String mod, Arguments args)
   bei OpenRA.Game.Initialize(Arguments args)
   bei OpenRA.Game.InitializeAndRun(String[] args)
   bei OpenRA.Program.Main(String[] args)```
Copy link
Member

abcdefg30 left a comment

Works for me. I suppose we want someone on a mac test this before merging though.


static void DebugMessageHandler(int source, int type, uint id, int severity, int length, StringBuilder message, IntPtr userparam)
{
string error;

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Sep 27, 2019

Member

What is this variable for? As far as I can see it's only used directly and once after being written to.

This comment has been minimized.

Copy link
@vesuvian

vesuvian Sep 28, 2019

Author Contributor

You can't define the same variable in different branches of a switch statement. I opted to build the error messages on one line and then throw/log/write on another for readability and maintainability.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Sep 28, 2019

Member

What I wanted to say was that I wouldn't define a variable at all, as it doesn't seem to be used more than once anyway(?).

This comment has been minimized.

Copy link
@vesuvian

vesuvian Sep 28, 2019

Author Contributor

I can certainly inline the usage if you insist, my argument against is that Nesting(Methods(), Like(), This()) makes for code that is harder to read and maintain.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Sep 28, 2019

Member

I would disagree for this particular instance since it should be pretty clear what the code is doing regardless (especially since the method is named appropriately) but don't really care either way. So I guess you can leave it as is unless someone else wants it changed too.

@pchote
pchote approved these changes Oct 5, 2019
@pchote pchote merged commit 36c48e1 into OpenRA:bleed Oct 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.