From d691ed2f3bde7ff5dd99bf74c8b2ce531807a362 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 12 Feb 2024 19:33:44 +0100 Subject: [PATCH] GPU: Use std::string instead of C strings for code gen output The benefits are removing unnecessary reallocations of the string data and unnecessary recalculations of the size, better type safety, and more automatic memory management. Pull Request: https://projects.blender.org/blender/blender/pulls/118045 --- .../draw/engines/eevee/eevee_shaders_extra.cc | 12 ++--- .../draw/engines/eevee_next/eevee_shader.cc | 16 +++--- source/blender/gpu/GPU_material.hh | 16 +++--- source/blender/gpu/intern/gpu_codegen.cc | 54 +++++++------------ 4 files changed, 42 insertions(+), 56 deletions(-) diff --git a/source/blender/draw/engines/eevee/eevee_shaders_extra.cc b/source/blender/draw/engines/eevee/eevee_shaders_extra.cc index 7d5fedf4926..4fadcb51946 100644 --- a/source/blender/draw/engines/eevee/eevee_shaders_extra.cc +++ b/source/blender/draw/engines/eevee/eevee_shaders_extra.cc @@ -131,7 +131,7 @@ void eevee_shader_material_create_info_amend(GPUMaterial *gpumat, attr_load << "void attrib_load()\n"; attr_load << "{\n"; - attr_load << ((codegen.attr_load) ? codegen.attr_load : ""); + attr_load << ((!codegen.attr_load.empty()) ? codegen.attr_load : ""); attr_load << "}\n\n"; std::stringstream vert_gen, frag_gen, geom_gen; @@ -152,20 +152,18 @@ void eevee_shader_material_create_info_amend(GPUMaterial *gpumat, { frag_gen << frag; - if (codegen.material_functions) { - frag_gen << codegen.material_functions; - } + frag_gen << codegen.material_functions; frag_gen << "Closure nodetree_exec()\n"; frag_gen << "{\n"; if (is_volume) { - frag_gen << ((codegen.volume) ? codegen.volume : "return CLOSURE_DEFAULT;\n"); + frag_gen << ((!codegen.volume.empty()) ? codegen.volume : "return CLOSURE_DEFAULT;\n"); } else { - frag_gen << ((codegen.surface) ? codegen.surface : "return CLOSURE_DEFAULT;\n"); + frag_gen << ((!codegen.surface.empty()) ? codegen.surface : "return CLOSURE_DEFAULT;\n"); } frag_gen << "}\n\n"; - if (codegen.displacement && (is_hair || is_mesh)) { + if (!codegen.displacement.empty() && (is_hair || is_mesh)) { info.define("EEVEE_DISPLACEMENT_BUMP"); frag_gen << "vec3 displacement_exec()\n"; diff --git a/source/blender/draw/engines/eevee_next/eevee_shader.cc b/source/blender/draw/engines/eevee_next/eevee_shader.cc index ee9cc18e0f2..caf73c541d0 100644 --- a/source/blender/draw/engines/eevee_next/eevee_shader.cc +++ b/source/blender/draw/engines/eevee_next/eevee_shader.cc @@ -526,7 +526,7 @@ void ShaderModule::material_create_info_ammend(GPUMaterial *gpumat, GPUCodegenOu std::stringstream attr_load; attr_load << "void attrib_load()\n"; attr_load << "{\n"; - attr_load << ((codegen.attr_load) ? codegen.attr_load : ""); + attr_load << ((!codegen.attr_load.empty()) ? codegen.attr_load : ""); attr_load << "}\n\n"; std::stringstream vert_gen, frag_gen, comp_gen; @@ -544,7 +544,7 @@ void ShaderModule::material_create_info_ammend(GPUMaterial *gpumat, GPUCodegenOu } if (!is_compute) { - const bool use_vertex_displacement = (codegen.displacement != nullptr) && + const bool use_vertex_displacement = (!codegen.displacement.empty()) && (displacement_type != MAT_DISPLACEMENT_BUMP) && (!ELEM(geometry_type, MAT_GEOM_WORLD, @@ -561,9 +561,9 @@ void ShaderModule::material_create_info_ammend(GPUMaterial *gpumat, GPUCodegenOu } if (!is_compute && pipeline_type != MAT_PIPE_VOLUME_OCCUPANCY) { - frag_gen << ((codegen.material_functions) ? codegen.material_functions : "\n"); + frag_gen << ((!codegen.material_functions.empty()) ? codegen.material_functions : "\n"); - if (codegen.displacement) { + if (!codegen.displacement.empty()) { /* Bump displacement. Needed to recompute normals after displacement. */ info.define("MAT_DISPLACEMENT_BUMP"); @@ -576,25 +576,25 @@ void ShaderModule::material_create_info_ammend(GPUMaterial *gpumat, GPUCodegenOu frag_gen << "Closure nodetree_surface()\n"; frag_gen << "{\n"; frag_gen << " closure_weights_reset();\n"; - frag_gen << ((codegen.surface) ? codegen.surface : "return Closure(0);\n"); + frag_gen << ((!codegen.surface.empty()) ? codegen.surface : "return Closure(0);\n"); frag_gen << "}\n\n"; frag_gen << "float nodetree_thickness()\n"; frag_gen << "{\n"; /* TODO(fclem): Better default. */ - frag_gen << ((codegen.thickness) ? codegen.thickness : "return 0.1;\n"); + frag_gen << ((!codegen.thickness.empty()) ? codegen.thickness : "return 0.1;\n"); frag_gen << "}\n\n"; info.fragment_source_generated = frag_gen.str(); } if (is_compute) { - comp_gen << ((codegen.material_functions) ? codegen.material_functions : "\n"); + comp_gen << ((!codegen.material_functions.empty()) ? codegen.material_functions : "\n"); comp_gen << "Closure nodetree_volume()\n"; comp_gen << "{\n"; comp_gen << " closure_weights_reset();\n"; - comp_gen << ((codegen.volume) ? codegen.volume : "return Closure(0);\n"); + comp_gen << ((!codegen.volume.empty()) ? codegen.volume : "return Closure(0);\n"); comp_gen << "}\n\n"; info.compute_source_generated = comp_gen.str(); diff --git a/source/blender/gpu/GPU_material.hh b/source/blender/gpu/GPU_material.hh index e0d1d5ed169..c4451cef48b 100644 --- a/source/blender/gpu/GPU_material.hh +++ b/source/blender/gpu/GPU_material.hh @@ -8,6 +8,8 @@ #pragma once +#include + #include "DNA_customdata_types.h" /* for eCustomDataType */ #include "DNA_image_types.h" #include "DNA_listBase.h" @@ -130,14 +132,14 @@ enum eGPUDefaultValue { }; struct GPUCodegenOutput { - char *attr_load; + std::string attr_load; /* Node-tree functions calls. */ - char *displacement; - char *surface; - char *volume; - char *thickness; - char *composite; - char *material_functions; + std::string displacement; + std::string surface; + std::string volume; + std::string thickness; + std::string composite; + std::string material_functions; GPUShaderCreateInfo *create_info; }; diff --git a/source/blender/gpu/intern/gpu_codegen.cc b/source/blender/gpu/intern/gpu_codegen.cc index 0e386888ac0..e6b55c93778 100644 --- a/source/blender/gpu/intern/gpu_codegen.cc +++ b/source/blender/gpu/intern/gpu_codegen.cc @@ -288,13 +288,6 @@ class GPUCodegen { ~GPUCodegen() { - MEM_SAFE_FREE(output.attr_load); - MEM_SAFE_FREE(output.surface); - MEM_SAFE_FREE(output.volume); - MEM_SAFE_FREE(output.thickness); - MEM_SAFE_FREE(output.displacement); - MEM_SAFE_FREE(output.composite); - MEM_SAFE_FREE(output.material_functions); MEM_SAFE_FREE(cryptomatte_input_); delete create_info; BLI_freelistN(&ubo_inputs_); @@ -327,22 +320,16 @@ class GPUCodegen { void set_unique_ids(); void node_serialize(std::stringstream &eval_ss, const GPUNode *node); - char *graph_serialize(eGPUNodeTag tree_tag, - GPUNodeLink *output_link, - const char *output_default = nullptr); - char *graph_serialize(eGPUNodeTag tree_tag); - - static char *extract_c_str(std::stringstream &stream) - { - auto string = stream.str(); - return BLI_strdup(string.c_str()); - } + std::string graph_serialize(eGPUNodeTag tree_tag, + GPUNodeLink *output_link, + const char *output_default = nullptr); + std::string graph_serialize(eGPUNodeTag tree_tag); }; void GPUCodegen::generate_attribs() { if (BLI_listbase_is_empty(&graph.attributes)) { - output.attr_load = nullptr; + output.attr_load.clear(); return; } @@ -397,7 +384,7 @@ void GPUCodegen::generate_attribs() iface.smooth(to_type(iface_type), var_name); } - output.attr_load = extract_c_str(load_ss); + output.attr_load = load_ss.str(); } void GPUCodegen::generate_resources() @@ -584,12 +571,12 @@ void GPUCodegen::node_serialize(std::stringstream &eval_ss, const GPUNode *node) nodes_total_++; } -char *GPUCodegen::graph_serialize(eGPUNodeTag tree_tag, - GPUNodeLink *output_link, - const char *output_default) +std::string GPUCodegen::graph_serialize(eGPUNodeTag tree_tag, + GPUNodeLink *output_link, + const char *output_default) { if (output_link == nullptr && output_default == nullptr) { - return nullptr; + return ""; } std::stringstream eval_ss; @@ -605,7 +592,7 @@ char *GPUCodegen::graph_serialize(eGPUNodeTag tree_tag, } if (!has_nodes) { - return nullptr; + return ""; } if (output_link) { @@ -616,12 +603,12 @@ char *GPUCodegen::graph_serialize(eGPUNodeTag tree_tag, eval_ss << "return " << output_default << ";\n"; } - char *eval_c_str = extract_c_str(eval_ss); - BLI_hash_mm2a_add(&hm2a_, (uchar *)eval_c_str, eval_ss.str().size()); - return eval_c_str; + std::string str = eval_ss.str(); + BLI_hash_mm2a_add(&hm2a_, reinterpret_cast(str.c_str()), str.size()); + return str; } -char *GPUCodegen::graph_serialize(eGPUNodeTag tree_tag) +std::string GPUCodegen::graph_serialize(eGPUNodeTag tree_tag) { std::stringstream eval_ss; LISTBASE_FOREACH (GPUNode *, node, &graph.nodes) { @@ -629,9 +616,9 @@ char *GPUCodegen::graph_serialize(eGPUNodeTag tree_tag) node_serialize(eval_ss, node); } } - char *eval_c_str = extract_c_str(eval_ss); - BLI_hash_mm2a_add(&hm2a_, (uchar *)eval_c_str, eval_ss.str().size()); - return eval_c_str; + std::string str = eval_ss.str(); + BLI_hash_mm2a_add(&hm2a_, reinterpret_cast(str.c_str()), str.size()); + return str; } void GPUCodegen::generate_cryptomatte() @@ -708,11 +695,10 @@ void GPUCodegen::generate_graphs() } /* Tag only the nodes needed for the current function */ gpu_nodes_tag(func_link->outlink, GPU_NODE_TAG_FUNCTION); - char *fn = graph_serialize(GPU_NODE_TAG_FUNCTION, func_link->outlink); + const std::string fn = graph_serialize(GPU_NODE_TAG_FUNCTION, func_link->outlink); eval_ss << "float " << func_link->name << "() {\n" << fn << "}\n\n"; - MEM_SAFE_FREE(fn); } - output.material_functions = extract_c_str(eval_ss); + output.material_functions = eval_ss.str(); /* Leave the function tags as they were before serialization */ LISTBASE_FOREACH (GPUNodeGraphFunctionLink *, funclink, &graph.material_functions) { gpu_nodes_tag(funclink->outlink, GPU_NODE_TAG_FUNCTION);