Skip to content

Commit

Permalink
fix a HwProgram leak related to depth variant pre-caching (#8276)
Browse files Browse the repository at this point in the history 
* fix a HwProgram leak related to depth variant pre-caching

Materials that don't use a custom depth can share the same program for
all depth variants. We take advantage of that by pre-caching the
depths variants when a program is loaded. The depth variants themselves
are owned by the default material which is created first. When a
material is destroyed it skips deletion of those variants which will
be truly destroyed when the default material is destroyed.

Unfortunately, the default material doesn't contain all depth variants,
in particular, it doesn't have the VSM ones (because it's an unlit
material).  On top of that, the way it pre-cached depth variant had
a bug that made it miss some picking variants.

Because of that, these variants were skipped during material destruction
but never actually destroyed, since the default material didn't know
anything about them.

This PR fixes this by making the pre-caching completely lazy. When
any material needs such a "precacheable" variant, it first checks if
the default material has it. If not, it creates the variant, caches it
locally and forwards it to the default material, which implicitly
becomes the owner. Just like before newly created material precache
everything the default material already has.

With this mechanism, it's impossible to have a depth variant without it
also existing in the default material.

This also simplify the non-public invalidate() method, which doesn't
need to populate the pre-cached programs, since this will happen
naturally when needed.

There was also another issue where post-process materials were
handled like regular materials. This didn't cause an problem but was
inefficient since these only have a handful of variants.

* Update filament/src/details/Material.cpp

Co-authored-by: Powei Feng <[email protected]>

---------

Co-authored-by: Powei Feng <[email protected]>
  • Loading branch information
@pixelflinger @poweifeng
pixelflinger and poweifeng authored Nov 20, 2024
1 parent 98585bd commit 64eb27a
Show file tree
Hide file tree
Showing 5 changed files with 198 additions and 97 deletions.
25 changes: 19 additions & 6 deletions filament/src/details/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,19 +529,31 @@ void FEngine::shutdown() {
mPerViewDescriptorSetLayoutSsrVariant.terminate(mHwDescriptorSetLayoutFactory, driver);
mPerRenderableDescriptorSetLayout.terminate(mHwDescriptorSetLayoutFactory, driver);

driver.destroyRenderPrimitive(mFullScreenTriangleRph);
driver.destroyRenderPrimitive(std::move(mFullScreenTriangleRph));

destroy(mFullScreenTriangleIb);
mFullScreenTriangleIb = nullptr;

destroy(mFullScreenTriangleVb);
mFullScreenTriangleVb = nullptr;

destroy(mDummyMorphTargetBuffer);
mDummyMorphTargetBuffer = nullptr;

destroy(mDefaultIblTexture);
mDefaultIblTexture = nullptr;

destroy(mDefaultIbl);
mDefaultIbl = nullptr;

destroy(mDefaultColorGrading);
mDefaultColorGrading = nullptr;

destroy(mDefaultMaterial);
mDefaultMaterial = nullptr;

destroy(mUnprotectedDummySwapchain);
mUnprotectedDummySwapchain = nullptr;

/*
* clean-up after the user -- we call terminate on each "leaked" object and clear each list.
Expand All @@ -558,6 +570,7 @@ void FEngine::shutdown() {

// this must be done after Skyboxes and before materials
destroy(mSkyboxMaterial);
mSkyboxMaterial = nullptr;

cleanupResourceList(std::move(mBufferObjects));
cleanupResourceList(std::move(mIndexBuffers));
Expand All @@ -574,12 +587,12 @@ void FEngine::shutdown() {

cleanupResourceListLocked(mFenceListLock, std::move(mFences));

driver.destroyTexture(mDummyOneTexture);
driver.destroyTexture(mDummyOneTextureArray);
driver.destroyTexture(mDummyZeroTexture);
driver.destroyTexture(mDummyZeroTextureArray);
driver.destroyTexture(std::move(mDummyOneTexture));
driver.destroyTexture(std::move(mDummyOneTextureArray));
driver.destroyTexture(std::move(mDummyZeroTexture));
driver.destroyTexture(std::move(mDummyZeroTextureArray));

driver.destroyRenderTarget(mDefaultRenderTarget);
driver.destroyRenderTarget(std::move(mDefaultRenderTarget));

/*
* Shutdown the backend...
Expand Down
216 changes: 130 additions & 86 deletions filament/src/details/Material.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,18 +321,20 @@ FMaterial::FMaterial(FEngine& engine, const Material::Builder& builder,
parser->getSpecularAntiAliasingThreshold(&mSpecularAntiAliasingThreshold);
}

processBlendingMode(parser);
processSpecializationConstants(engine, builder, parser);
processPushConstants(engine, parser);
processDepthVariants(engine, parser);
processDescriptorSets(engine, parser);
parser->hasCustomDepthShader(&mHasCustomDepthShader);

mPerViewLayoutIndex = ColorPassDescriptorSet::getIndex(
mIsVariantLit,
mReflectionMode == ReflectionMode::SCREEN_SPACE ||
mRefractionMode == RefractionMode::SCREEN_SPACE,
!(mVariantFilterMask & +UserVariantFilterBit::FOG));

processBlendingMode(parser);
processSpecializationConstants(engine, builder, parser);
processPushConstants(engine, parser);
processDescriptorSets(engine, parser);
precacheDepthVariants(engine);

#if FILAMENT_ENABLE_MATDBG
// Register the material with matdbg.
matdbg::DebugServer* server = downcast(engine).debug.server;
Expand All @@ -346,47 +348,21 @@ FMaterial::FMaterial(FEngine& engine, const Material::Builder& builder,
FMaterial::~FMaterial() noexcept = default;

void FMaterial::invalidate(Variant::type_t variantMask, Variant::type_t variantValue) noexcept {
DriverApi& driverApi = mEngine.getDriverApi();
if (mMaterialDomain == MaterialDomain::SURFACE) {
auto& cachedPrograms = mCachedPrograms;
for (size_t k = 0, n = VARIANT_COUNT; k < n; ++k) {
Variant const variant(k);
if ((k & variantMask) == variantValue) {
if (UTILS_LIKELY(!mIsDefaultMaterial)) {
// The depth variants may be shared with the default material, in which case
// we should not free it now.
bool const isSharedVariant =
Variant::isValidDepthVariant(variant) && !mHasCustomDepthShader;
if (isSharedVariant) {
// we don't own this variant, skip.
continue;
}
}
driverApi.destroyProgram(cachedPrograms[k]);
cachedPrograms[k].clear();
}
}

if (UTILS_UNLIKELY(!mIsDefaultMaterial && !mHasCustomDepthShader)) {
FMaterial const* const pDefaultMaterial = mEngine.getDefaultMaterial();
for (Variant const variant: pDefaultMaterial->mDepthVariants) {
pDefaultMaterial->prepareProgram(variant);
if (!cachedPrograms[variant.key]) {
cachedPrograms[variant.key] = pDefaultMaterial->getProgram(variant);
}
}
}
} else if (mMaterialDomain == MaterialDomain::POST_PROCESS) {
auto& cachedPrograms = mCachedPrograms;
for (size_t k = 0, n = POST_PROCESS_VARIANT_COUNT; k < n; ++k) {
if ((k & variantMask) == variantValue) {
driverApi.destroyProgram(cachedPrograms[k]);
cachedPrograms[k].clear();
}
// Note: This API is not public at the moment, so it's okay to have some debugging logs
// and extra checks.
if (mMaterialDomain == MaterialDomain::SURFACE &&
!mIsDefaultMaterial &&
!mHasCustomDepthShader) {
// it would be unsafe to invalidate any of the cached depth variant
if (UTILS_UNLIKELY(!((variantMask & Variant::DEP) && !(variantValue & Variant::DEP)))) {
slog.w << io::hex << "FMaterial::invalidate("
<< +variantMask << ", " << +variantValue
<< ") would corrupt the depth variant cache" << io::endl;
}
} else if (mMaterialDomain == MaterialDomain::COMPUTE) {
// TODO: handle compute variants if any
variantMask |= Variant::DEP;
variantValue &= ~Variant::DEP;
}
destroyPrograms(mEngine, variantMask, variantValue);
}

void FMaterial::terminate(FEngine& engine) {
Expand Down Expand Up @@ -626,10 +602,41 @@ Program FMaterial::getProgramWithVariants(
}

void FMaterial::createAndCacheProgram(Program&& p, Variant variant) const noexcept {
auto program = mEngine.getDriverApi().createProgram(std::move(p));
mEngine.getDriverApi().setDebugTag(program.getId(), mName);
FEngine const& engine = mEngine;
DriverApi& driverApi = mEngine.getDriverApi();

// Check if the default material has this program cached
if (mMaterialDomain == MaterialDomain::SURFACE &&
!mIsDefaultMaterial && !mHasCustomDepthShader &&
Variant::isValidDepthVariant(variant)) {
FMaterial const* const pDefaultMaterial = engine.getDefaultMaterial();
if (pDefaultMaterial) {
auto program = pDefaultMaterial->mCachedPrograms[variant.key];
if (program) {
mCachedPrograms[variant.key] = program;
return;
}
}
}

auto program = driverApi.createProgram(std::move(p));
driverApi.setDebugTag(program.getId(), mName);
assert_invariant(program);
mCachedPrograms[variant.key] = program;

// If the default material doesn't already have this program cached, and all caching conditions
// are met (Surface Domain and no custom depth shader), cache it now.
// New Materials will inherit these program automatically.
if (mMaterialDomain == MaterialDomain::SURFACE &&
!mIsDefaultMaterial && !mHasCustomDepthShader &&
Variant::isValidDepthVariant(variant)) {
FMaterial const* const pDefaultMaterial = engine.getDefaultMaterial();
if (pDefaultMaterial && !pDefaultMaterial->mCachedPrograms[variant.key]) {
// set the tag to the default material name
driverApi.setDebugTag(program.getId(), mName);
pDefaultMaterial->mCachedPrograms[variant.key] = program;
}
}
}

size_t FMaterial::getParameters(ParameterInfo* parameters, size_t count) const noexcept {
Expand Down Expand Up @@ -734,22 +741,76 @@ void FMaterial::onQueryCallback(void* userdata, VariantList* pVariants) {
#endif // FILAMENT_ENABLE_MATDBG


void FMaterial::destroyPrograms(FEngine& engine) {
void FMaterial::destroyPrograms(FEngine& engine,
Variant::type_t const variantMask, Variant::type_t const variantValue) {

DriverApi& driverApi = engine.getDriverApi();
auto& cachedPrograms = mCachedPrograms;
for (size_t k = 0, n = VARIANT_COUNT; k < n; ++k) {
const Variant variant(k);
if (!mIsDefaultMaterial) {
// The depth variants may be shared with the default material, in which case
// we should not free it now.
bool const isSharedVariant = Variant::isValidDepthVariant(variant) && !mHasCustomDepthShader;
if (isSharedVariant) {
// we don't own this variant, skip.
continue;

switch (mMaterialDomain) {
case MaterialDomain::SURFACE: {
if (mIsDefaultMaterial || mHasCustomDepthShader) {
// default material or we have custom depth shaders, we destroy all variants
for (size_t k = 0, n = VARIANT_COUNT; k < n; ++k) {
if ((k & variantMask) == variantValue) {
// Only destroy if the handle is valid. Not strictly needed, but we have a lot
// of variants, and this generates traffic in the command queue.
if (cachedPrograms[k]) {
driverApi.destroyProgram(std::move(cachedPrograms[k]));
}
}
}
} else {
// The depth variants may be shared with the default material, in which case
// we should not free them now.

// During Engine::shutdown(), auto-cleanup destroys the default material first,
// so this can be null, but this is only used for debugging.
UTILS_UNUSED_IN_RELEASE
auto UTILS_NULLABLE pDefaultMaterial = engine.getDefaultMaterial();

for (size_t k = 0, n = VARIANT_COUNT; k < n; ++k) {
if ((k & variantMask) == variantValue) {
// Only destroy if the handle is valid. Not strictly needed, but we have a lot
// of variant, and this generates traffic in the command queue.
if (cachedPrograms[k]) {
if (Variant::isValidDepthVariant(Variant(k))) {
// By construction this should always be true, because this
// field is populated only when a material creates the program
// for this variant.
// During Engine::shutdown, auto-cleanup destroys the
// default material first
assert_invariant(!pDefaultMaterial ||
pDefaultMaterial->mCachedPrograms[k]);
// we don't own this variant, skip, but clear the entry.
cachedPrograms[k].clear();
continue;
}

driverApi.destroyProgram(std::move(cachedPrograms[k]));
}
}
}
}
break;
}
case MaterialDomain::POST_PROCESS: {
for (size_t k = 0, n = POST_PROCESS_VARIANT_COUNT; k < n; ++k) {
if ((k & variantMask) == variantValue) {
// Only destroy if the handle is valid. Not strictly needed, but we have a lot
// of variant, and this generates traffic in the command queue.
if (cachedPrograms[k]) {
driverApi.destroyProgram(std::move(cachedPrograms[k]));
}
}
}
break;
}
case MaterialDomain::COMPUTE: {
// Compute programs don't have variants
driverApi.destroyProgram(std::move(cachedPrograms[0]));
break;
}
driverApi.destroyProgram(cachedPrograms[k]);
cachedPrograms[k].clear();
}
}

Expand Down Expand Up @@ -1014,34 +1075,17 @@ void FMaterial::processPushConstants(FEngine& engine, MaterialParser const* pars
});
}

void FMaterial::processDepthVariants(FEngine& engine, MaterialParser const* const parser) {
parser->hasCustomDepthShader(&mHasCustomDepthShader);

if (UTILS_UNLIKELY(mIsDefaultMaterial)) {
assert_invariant(mMaterialDomain == MaterialDomain::SURFACE);
filaflat::MaterialChunk const& materialChunk{ parser->getMaterialChunk() };
auto variants = FixedCapacityVector<Variant>::with_capacity(materialChunk.getShaderCount());
materialChunk.visitShaders([&variants](
ShaderModel, Variant variant, ShaderStage) {
if (Variant::isValidDepthVariant(variant)) {
variants.push_back(variant);
}
});
std::sort(variants.begin(), variants.end(),
[](Variant lhs, Variant rhs) { return lhs.key < rhs.key; });
auto pos = std::unique(variants.begin(), variants.end());
variants.resize(std::distance(variants.begin(), pos));
std::swap(mDepthVariants, variants);
}

if (mMaterialDomain == MaterialDomain::SURFACE) {
if (UTILS_UNLIKELY(!mIsDefaultMaterial && !mHasCustomDepthShader)) {
FMaterial const* const pDefaultMaterial = engine.getDefaultMaterial();
auto& cachedPrograms = mCachedPrograms;
for (Variant const variant: pDefaultMaterial->mDepthVariants) {
pDefaultMaterial->prepareProgram(variant);
cachedPrograms[variant.key] = pDefaultMaterial->getProgram(variant);
}
void FMaterial::precacheDepthVariants(FEngine& engine) {
// if possible pre-cache all depth variants from the default material
if (mMaterialDomain == MaterialDomain::SURFACE &&
!mIsDefaultMaterial &&
!mHasCustomDepthShader) {
FMaterial const* const pDefaultMaterial = engine.getDefaultMaterial();
assert_invariant(pDefaultMaterial);
auto const allDepthVariants = VariantUtils::getDepthVariants();
for (auto const variant: allDepthVariants) {
assert_invariant(Variant::isValidDepthVariant(variant));
mCachedPrograms[variant.key] = pDefaultMaterial->mCachedPrograms[variant.key];
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions filament/src/details/Material.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,9 @@ class FMaterial : public Material {

uint32_t generateMaterialInstanceId() const noexcept { return mMaterialInstanceId++; }

void destroyPrograms(FEngine& engine);
void destroyPrograms(FEngine& engine,
Variant::type_t variantMask = 0,
Variant::type_t variantValue = 0);

// return the id of a specialization constant specified by name for this material
std::optional<uint32_t> getSpecializationConstantId(std::string_view name) const noexcept ;
Expand Down Expand Up @@ -282,7 +284,7 @@ class FMaterial : public Material {

void processPushConstants(FEngine& engine, MaterialParser const* parser);

void processDepthVariants(FEngine& engine, MaterialParser const* parser);
void precacheDepthVariants(FEngine& engine);

void processDescriptorSets(FEngine& engine, MaterialParser const* parser);

Expand Down Expand Up @@ -331,7 +333,6 @@ class FMaterial : public Material {
SamplerInterfaceBlock mSamplerInterfaceBlock;
BufferInterfaceBlock mUniformInterfaceBlock;
SubpassInfo mSubpassInfo;
utils::FixedCapacityVector<Variant> mDepthVariants; // only populated with default material

using BindingUniformInfoContainer = utils::FixedCapacityVector<std::tuple<
uint8_t, utils::CString, backend::Program::UniformInfo>>;
Expand Down
3 changes: 3 additions & 0 deletions libs/filabridge/include/private/filament/Variant.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <filament/MaterialEnums.h>

#include <utils/compiler.h>
#include <utils/bitset.h>
#include <utils/Slice.h>

Expand Down Expand Up @@ -271,6 +272,8 @@ namespace VariantUtils {
utils::Slice<Variant> getLitVariants() noexcept UTILS_PURE;
// list of unlit variants
utils::Slice<Variant> getUnlitVariants() noexcept UTILS_PURE;
// list of depth variants
utils::Slice<Variant> getDepthVariants() noexcept UTILS_PURE;
}

} // namespace filament
Expand Down
Loading

0 comments on commit 64eb27a

Please sign in to comment.