Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Fix hot reload for shaders on Impeller #49739

Merged
dnfield merged 4 commits into from
Jan 16, 2024
Merged

[Impeller] Fix hot reload for shaders on Impeller #49739

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5570027
Fix hot reload of shaders fo rImpeller
dnfield Jan 12, 2024
f8092f7
Fix reloading of related shader pipelines
dnfield Jan 12, 2024
5366846
Merge remote-tracking branch 'upstream/main' into gles_reload
dnfield Jan 12, 2024
e0b9457
license bot
dnfield Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions impeller/entity/contents/content_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,14 @@ class ContentContext {
const std::function<std::shared_ptr<Pipeline<PipelineDescriptor>>()>&
create_callback) const;

/// Used by hot reload/hot restart to clear a cached pipeline from
/// GetCachedRuntimeEffectPipeline.
void ClearCachedRuntimeEffectPipeline(

@jonahwilliams jonahwilliams Jan 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its possible that from frame to frame you will get different values for options, which means you may not invalidate all of the pipeline descriptors associated with this shader consider this scenario:

Frame 1: Use and cache shader with Rect geometry, triangle geometry is used.
Frame 2: Hot reload, Use shader with tessellated geometry, triangle strip is used. Pipeline descripor matching shader name + triangle strip is removed.
Frame 3: Hot reload, use shader with rect geometry, old pipeline descriptor from frame 1 is recycled.

You'd need to clear out all possible pipelines for a given shader variant. Since this is a debug only feature, you could iterate the cache and evict everything that has a name match. Or switch to a two level cache.

@dnfield dnfield Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shader comes in as dirty initially. That would mean evicting everything with the same name every time we got a new geometry, right?

@jonahwilliams jonahwilliams Jan 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a one to many mapping of shaders to pipelines. So if the shader is dirty, then all pipelines (regardless of the ContentContextOptions must be marked dirty). You'd probably need to special case the first time you see a shader?

@dnfield dnfield Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really are worried about this case I think we need to just plain have a service extension that kills the cache. It's just hard to get that right with rendering so that the cache gets killed and some frame isn't in flight that puts the shader back into the cache before the new shader version comes through.

@jonahwilliams jonahwilliams Jan 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace frame numbers with two kinds of draws in the same frame. If you don't invalidate all pipeline descriptors marching the name, then only the first will get updated when you got reload

@dnfield dnfield Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, I have a sample program now that uses different blend mode.

Invalidating all the ones with the same name ... seems to work. Let's just do it.

@dnfield dnfield Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've updated the evict method to just evict by name rather than including the options, and written some tests for it. This works locally for hot reload with a simple app where the blend modes are different on the same shader.

const std::string& unique_entrypoint_name,
const ContentContextOptions& options) const {
runtime_effect_pipelines_.erase({unique_entrypoint_name, options});
}

/// @brief Retrieve the currnent host buffer for transient storage.
///
/// This is only safe to use from the raster threads. Other threads should
Expand Down
30 changes: 15 additions & 15 deletions impeller/entity/contents/runtime_effect_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,22 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
std::shared_ptr<const ShaderFunction> function = library->GetFunction(
runtime_stage_->GetEntrypoint(), ShaderStage::kFragment);

//--------------------------------------------------------------------------
/// Resolve geometry and content context options.
///

auto geometry_result =
GetGeometry()->GetPositionBuffer(renderer, entity, pass);
auto options = OptionsFromPassAndEntity(pass, entity);
if (geometry_result.prevent_overdraw) {
options.stencil_compare = CompareFunction::kEqual;
options.stencil_operation = StencilOperation::kIncrementClamp;
}
options.primitive_type = geometry_result.type;

if (function && runtime_stage_->IsDirty()) {
renderer.ClearCachedRuntimeEffectPipeline(runtime_stage_->GetEntrypoint(),
options);
context->GetPipelineLibrary()->RemovePipelinesWithEntryPoint(function);
library->UnregisterFunction(runtime_stage_->GetEntrypoint(),
ShaderStage::kFragment);
Expand Down Expand Up @@ -119,13 +134,6 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
runtime_stage_->SetClean();
}

//--------------------------------------------------------------------------
/// Resolve geometry.
///

auto geometry_result =
GetGeometry()->GetPositionBuffer(renderer, entity, pass);

//--------------------------------------------------------------------------
/// Set up the command. Defer setting up the pipeline until the descriptor set
/// layouts are known from the uniforms.
Expand Down Expand Up @@ -276,14 +284,6 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
}

/// Now that the descriptor set layouts are known, get the pipeline.

auto options = OptionsFromPassAndEntity(pass, entity);
if (geometry_result.prevent_overdraw) {
options.stencil_compare = CompareFunction::kEqual;
options.stencil_operation = StencilOperation::kIncrementClamp;
}
options.primitive_type = geometry_result.type;

auto create_callback =
[&]() -> std::shared_ptr<Pipeline<PipelineDescriptor>> {
PipelineDescriptor desc;
Expand Down
36 changes: 29 additions & 7 deletions impeller/entity/entity_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
#include "impeller/renderer/command.h"
#include "impeller/renderer/pipeline_descriptor.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/testing/mocks.h"
#include "impeller/renderer/vertex_buffer_builder.h"
#include "impeller/typographer/backends/skia/text_frame_skia.h"
#include "impeller/typographer/backends/skia/typographer_context_skia.h"
Expand Down Expand Up @@ -2139,14 +2140,10 @@ TEST_P(EntityTest, RuntimeEffect) {
ASSERT_TRUE(runtime_stage);
ASSERT_TRUE(runtime_stage->IsDirty());

bool first_frame = true;
bool expect_dirty = true;
Pipeline<PipelineDescriptor>* first_pipeline = nullptr;
auto callback = [&](ContentContext& context, RenderPass& pass) -> bool {
if (first_frame) {
first_frame = false;
} else {
assert(runtime_stage->IsDirty() == false);
}
EXPECT_EQ(runtime_stage->IsDirty(), expect_dirty);

auto contents = std::make_shared<RuntimeEffectContents>();
contents->SetGeometry(Geometry::MakeCover());
Expand All @@ -2168,13 +2165,38 @@ TEST_P(EntityTest, RuntimeEffect) {
Entity entity;
entity.SetContents(contents);
bool result = contents->Render(context, entity, pass);
if (!first_pipeline) {

if (expect_dirty) {
EXPECT_NE(first_pipeline, pass.GetCommands().back().pipeline.get());
first_pipeline = pass.GetCommands().back().pipeline.get();
} else {
EXPECT_EQ(pass.GetCommands().back().pipeline.get(), first_pipeline);
}

expect_dirty = false;
return result;
};

// Simulate some renders and hot reloading of the shader.
auto content_context = GetContentContext();
{
RenderTarget target;
testing::MockRenderPass mock_pass(GetContext(), target);
callback(*content_context, mock_pass);
callback(*content_context, mock_pass);

// Dirty the runtime stage.
runtime_stages = OpenAssetAsRuntimeStage("runtime_stage_example.frag.iplr");
runtime_stage =
runtime_stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())];

ASSERT_TRUE(runtime_stage->IsDirty());
expect_dirty = true;

callback(*content_context, mock_pass);
callback(*content_context, mock_pass);
}

ASSERT_TRUE(OpenPlaygroundHere(callback));
}

Expand Down