Details

Reviewers

Brecht Van Lommel (brecht)
Jeroen Bakker (jbakker)

Maniphest Tasks

T66231: Performance degradation of Principled Shader in Eevee when metallic is turned off

Commits

rBb18c2a3c413b: EEVEE: Refactor of eevee_material.c

Summary

This patch was originally designed to fix T66231 but went a bit over and I had to
redesign a bit more.

This patch is huge but since it fixes an important performance regression, could it
be considered for inclusion in 2.83? Having this refactor in for the LTS could help
backport fixes from future release. I don't have a strong opinion on this though.

I did check for regression tests and already fixed the issues. Only the hair+SSS test
needs to be updated.

These are the modifications:

With DRW modification we reduce the number of passes we need to populate.
Rename passes for consistent naming.
Reduce complexity in code compilation
Cleanup how renderpass accumulation passes are setup, using pass instances.
Make sculpt mode compatible with shadows
Make hair passes compatible with SSS
Error shader and lookdev materials now use standalone materials.
Support default shader (world and material) using a default nodetree internally.
Change BLEND_CLIP to be emulated by gpu nodetree. Making less shader variations.
Use BLI_memblock for cache memory allocation.
Renderpasses are handled by switching a UBO ref bind.

One major hack in this patch is the use of modified pointer as ghash keys.
This rely on the assumption that the keys will never overlap because the
number of options per key will never be bigger than the pointed struct.

The use of one single nodetree to support default material is also a bit hacky
since it won't support concurent usage of this nodetree.
(see EEVEE_shader_default_surface_nodetree)
If anyone has a better idea let me know.

This improve CPU performance a bit in scene with lots of instancing.

A lot more cleanup can go in after this patch (removing unused shaders, routines).
I didn't want to bloat the patch with thoses.

Another change is that objects with shader errors now appear solid magenta instead
of shaded magenta. This is only because of code reuse purpose but could be changed
if really needed.

Diff Detail

Repository

rB Blender

Branch

tmp-T66231 (branched from master)

Build Status

Buildable 7923
Build 7923: arc lint + arc unit

Event Timeline

Clément Foucault (fclem) requested review of this revision.May 6 2020, 7:58 PM

Clément Foucault (fclem) created this revision.

Harbormaster completed remote builds in B7923: Diff 24458.May 6 2020, 7:58 PM

Would this solve T70445: Very slow Eevee performance on Mac with AMD Radeon as well?

@Julian Eisel (Severin), since you have macOS + AMD, can you reproduce the original bug and test that this solves it?

David Mikucki (davidmikucki) added a subscriber: David Mikucki (davidmikucki).May 7 2020, 5:36 PM

Wouter Stomp (wouterstomp) added a subscriber: Wouter Stomp (wouterstomp).May 7 2020, 9:13 PM

Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).May 7 2020, 9:50 PM

This is probably too risky for 2.83, unless we decide to move the release date (which we might do anyway given the number of open high priority bugs)?

Otherwise this could be committed to master and backported to 2.83 for 2.83a.

Is there a branch for this with changes split into smaller commits? It's rather hard to review this as one big patch.

I tested the file from T70445, but I'm pretty confused. The Eevee stats all just show 0.0ms. But also without this patch, the total render time is rather consistent around 1000ms. With it, it seems to increase with each redraw, starting at 1000ms.

So yeah, I'm not a big help so far here, but rather confused.
Radeon Pro 5500M, system-info in case it helps:

system-info.txt9 KBDownload

@Julian Eisel (Severin) Don't use debug info metrics for that. They are currently broken and have high sync overhead issues. Just compare animation playback fps but without any actual animation to avoid DEG update cost.

Is there a branch for this with changes split into smaller commits? It's rather hard to review this as one big patch.

@Brecht Van Lommel (brecht) Unfortunately there isn't. I just kept ammending one commit. I suggest starting at EEVEE_materials_cache_populate.

You can use animation in Blender?! How would I know! :)
You're not going to like this though, it went from about 0.5 to 0.4FPS with the patch. Interestingly enough it doesn't just apply to the viewport, interacting with any editor while Eevee runs is horribly slow. As if it used software rendering.

When running with --debug-gpu I get a bunch of warnings though:

GPUShaderInterface: Warning: Uniform 'attr' not found!
GPUShaderInterface: Warning: Uniform 'colorBuffer' not found!
GPUShaderInterface: Warning: Uniform 'depthBuffer' not found!
GPUShaderInterface: Warning: Uniform 'maxzBuffer' not found!
GPUShaderInterface: Warning: Uniform 'minzBuffer' not found!
GPUShaderInterface: Warning: Uniform 'planarDepth' not found!
GPUShaderInterface: Warning: Uniform 'texHammersley' not found!
GPUShaderInterface: Warning: Uniform 'texJitter' not found!
GPUShaderInterface: Warning: Uniform 'utilTex' not found!
GPUShaderInterface: Warning: Uniform 'horizonBuffer' not found!
GPUShaderInterface: Warning: Uniform 'irradianceGrid' not found!
GPUShaderInterface: Warning: Uniform 'probePlanars' not found!
GPUShaderInterface: Warning: Uniform 'probeCubes' not found!
GPUShaderInterface: Warning: Uniform 'shadowCubeTexture' not found!
GPUShaderInterface: Warning: Uniform 'shadowCascadeTexture' not found!
GPUShaderInterface: Warning: Uniform 'inScattering' not found!
GPUShaderInterface: Warning: Uniform 'inTransmittance' not found!

(If more discussion is needed, that should probably be moved to the report, it doesn't seem too related to the patch.)

Stas (girafic) added a subscriber: Stas (girafic).May 8 2020, 10:17 AM

@Clément Foucault (fclem), please split this up into smaller commits then. The workbench refactor showed that putting everything in one big commits means that a) it will not be properly reviewed and b) if there is a bug it's unclear which of the changes caused the problem. Otherwise we're just repeating the same mistake.

Victor Bonnet (vbonnet) added a subscriber: Victor Bonnet (vbonnet).May 10 2020, 11:55 AM

This patch is not fixing the T70445 issue.

I checked the animation fps on my macbook:

2.80.75: ~2.7fps
master: ~0.3fps
master + patch: ~0.25fps

Cleanup: EEVEE: Remove Unused variable after refactor.
Cleanup: EEVEE: Split LUT generation code from eevee_material.c
Cleanup: EEVEE: Move shader related function to eevee_shaders.c
Cleanup: EEVEE: Remove EEVEE_material_default_render_pass_ubo_get
Cleanup: DRW: Remove unused DRW_shgroup_hair_create/DRW_shgroup_material_hair_create
EEVEE: Fix assert during default shader compilation

Harbormaster completed remote builds in B8047: Diff 24771.May 14 2020, 5:01 PM

I tried to split as much as I could in the branch tmp-eevee-material-refactor. The issue is that there was a bunch of renaming of passes that happened and that is tedious to separate from the actual rewritting of the pass handling. Same goes for RenderPasses, the code was really getting in the way so I completely commented it out during developement. But this also means that the RenderPass code is now rather separated from the Material code.

I add some more cleanup to keep eevee_material.c even more clear.

It seems that this also fixes T71552.

Concerning the slowdown it would be nice if one of you can profile it. I guess it can come from the added CPU overhead.
Some more improvement can be done in this regard (better caching of GPUMaterial) to speedup the multiple lookups.

Note that the default material may not be as optimized as it was (using a UBO per material instead of just updating uniforms). But if that's an issue of using UBOs on OSX that's quite problematic as it's supposed to be the right way to do it.

Clément Foucault (fclem) mentioned this in T70445: Very slow Eevee performance on Mac with AMD Radeon.May 16 2020, 12:32 AM

This patch works perfect for T66231 (see video attached, left Blender instance with this patch, right Blender instance with @Zoltán Némedi (nemediz)'s patch).

With the same setup I tried the archiviz.blend demo file (https://download.blender.org/demo/eevee/archiviz/archiviz.blend) and the performance of this patch is slowing down.

@Stas (girafic) thank you for testing this patch. The slowdown in the second case is not caused by the same bug since all the shaders are sorted (see P1417). So I suspect this is the same issue as T70445 which we still don't know the root cause.

@Stas (girafic) huh it seems this patch is messing the texture binding. Try with P1419 applied on top of this patch to see if that fixes the issue for archvis test file (but will break hair rendering).

Fix subpass iteration

Harbormaster completed remote builds in B8220: Diff 25108.May 26 2020, 3:00 PM

@Clément Foucault (fclem) still slow with P1419

I am reviewing commit by commit the tmp-eevee-material-refactoring branch.

a per commit review is so much understanding what pass instances and linking does and how it will benefit. The commits descriptions allows better code reviewing.

open wanderer.blend and check the volume transmittance pass it is completely black.
Memory leak after using SSS. and cycling through the renderpasses in the viewport. (GPUTexture 224 bytes)
This means we cannot exclude dupli objects from reflections!!!: Should be added to the limitation section in the manual

source/blender/draw/engines/eevee/eevee_materials.c
932	better add a define for these constants to add more meaning to them.

This revision now requires changes to proceed.May 28 2020, 11:54 AM

Dalai Felinto (dfelinto) added inline comments.May 28 2020, 1:01 PM

source/blender/draw/engines/eevee/eevee_materials.c
932	Yes, please. Yesterday I brought this up as well for a different part of the code. For someone not totally familiar with the code things look really `hairy` ;)

Alaska (Alaska) awarded a token.May 28 2020, 1:36 PM

open wanderer.blend and check the volume transmittance pass it is completely black.

I cannot reproduce.

Memory leak after using SSS. and cycling through the renderpasses in the viewport. (GPUTexture 224 bytes)

This seems to also happen in master and 2.83. fixing it here.

This means we cannot exclude dupli objects from reflections!!!: Should be added to the limitation section in the manual

Totally.

Merge branch 'master' into tmp-eevee-material-refactor
Use enums instead of obscure constants.
Merge branch 'master' into tmp-eevee-material-refactor

Harbormaster completed remote builds in B8289: Diff 25240.May 29 2020, 4:02 PM

Julien Leménicier (mistajuliax) added a subscriber: Julien Leménicier (mistajuliax).Jun 1 2020, 10:32 PM

There is still an issue with volume transmittance, but that is also in master.

This revision is now accepted and ready to land.Jun 2 2020, 4:35 PM

Merge branch 'master' into tmp-eevee-material-refactor

Harbormaster completed remote builds in B8329: Diff 25397.Jun 2 2020, 4:49 PM

Closed by commit rBb18c2a3c413b: EEVEE: Refactor of eevee_material.c (authored by Clément Foucault (fclem)). · Explain WhyJun 2 2020, 4:58 PM

This revision was automatically updated to reflect the committed changes.

Clément Foucault (fclem) added a commit: rBb18c2a3c413b: EEVEE: Refactor of eevee_material.c.

EEVEE: Refactor of eevee_material.c
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 24458

source/blender/draw/CMakeLists.txt

source/blender/draw/engines/eevee/eevee_data.c

source/blender/draw/engines/eevee/eevee_engine.c

source/blender/draw/engines/eevee/eevee_lightcache.c

source/blender/draw/engines/eevee/eevee_lightprobes.c

source/blender/draw/engines/eevee/eevee_lookdev.c

source/blender/draw/engines/eevee/eevee_materials.c

source/blender/draw/engines/eevee/eevee_private.h

source/blender/draw/engines/eevee/eevee_render.c

source/blender/draw/engines/eevee/eevee_renderpasses.c

source/blender/draw/engines/eevee/eevee_shaders.c

source/blender/draw/engines/eevee/eevee_shadows.c

source/blender/draw/engines/eevee/eevee_subsurface.c

source/blender/draw/engines/eevee/eevee_volumes.c

source/blender/draw/engines/eevee/shaders/ambient_occlusion_lib.glsl

source/blender/draw/engines/eevee/shaders/bsdf_common_lib.glsl

source/blender/draw/engines/eevee/shaders/lit_surface_frag.glsl

source/blender/draw/engines/eevee/shaders/lit_surface_vert.glsl

source/blender/draw/engines/eevee/shaders/prepass_frag.glsl

source/blender/draw/intern/DRW_render.h

source/blender/draw/intern/draw_manager.h

source/blender/draw/intern/draw_manager_data.c

source/blender/draw/intern/draw_manager_exec.c

source/blender/draw/intern/draw_manager_shader.c

source/blender/gpu/GPU_material.h

source/blender/gpu/intern/gpu_material.c

source/blender/gpu/intern/gpu_texture.c

source/blender/gpu/shaders/material/gpu_shader_material_output_material.glsl

source/blender/nodes/shader/nodes/node_shader_output_material.c

EEVEE: Refactor of eevee_material.cClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 24458

source/blender/draw/CMakeLists.txt

source/blender/draw/engines/eevee/eevee_data.c

source/blender/draw/engines/eevee/eevee_engine.c

source/blender/draw/engines/eevee/eevee_lightcache.c

source/blender/draw/engines/eevee/eevee_lightprobes.c

source/blender/draw/engines/eevee/eevee_lookdev.c

source/blender/draw/engines/eevee/eevee_materials.c

source/blender/draw/engines/eevee/eevee_private.h

source/blender/draw/engines/eevee/eevee_render.c

source/blender/draw/engines/eevee/eevee_renderpasses.c

source/blender/draw/engines/eevee/eevee_shaders.c

source/blender/draw/engines/eevee/eevee_shadows.c

source/blender/draw/engines/eevee/eevee_subsurface.c

source/blender/draw/engines/eevee/eevee_volumes.c

source/blender/draw/engines/eevee/shaders/ambient_occlusion_lib.glsl

source/blender/draw/engines/eevee/shaders/bsdf_common_lib.glsl

source/blender/draw/engines/eevee/shaders/lit_surface_frag.glsl

source/blender/draw/engines/eevee/shaders/lit_surface_vert.glsl

source/blender/draw/engines/eevee/shaders/prepass_frag.glsl

source/blender/draw/intern/DRW_render.h

source/blender/draw/intern/draw_manager.h

source/blender/draw/intern/draw_manager_data.c

source/blender/draw/intern/draw_manager_exec.c

source/blender/draw/intern/draw_manager_shader.c

source/blender/gpu/GPU_material.h

source/blender/gpu/intern/gpu_material.c

source/blender/gpu/intern/gpu_texture.c

source/blender/gpu/shaders/material/gpu_shader_material_output_material.glsl

source/blender/nodes/shader/nodes/node_shader_output_material.c

EEVEE: Refactor of eevee_material.c
ClosedPublic
Actions

Revision Contents
Changeset List