-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Autodesk: [hdSt] Persistent run-time-populated MaterialX codegen cache #3661
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
base: dev
Are you sure you want to change the base?
Autodesk: [hdSt] Persistent run-time-populated MaterialX codegen cache #3661
Conversation
| using HdInstanceKey = uint64_t; | ||
| using HdInstanceRegistryMutex = std::mutex; | ||
| using HdInstanceRegistryLock = std::unique_lock<HdInstanceRegistryMutex>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, these were typedefs in the templated class definition of HdInstance but they never depended on template parameters. They've also been referenced in the HdInstanceRegistry implementation below, where they had to be namespaced with HdInstance::. I'm extending the HdInstanceRegistry implementation in order to optionally support persistence, and these verbose typedefs were hurting the readability in that code, so I'm making them standalone.
| explicit HdInstance(HdInstanceKey key, | ||
| ValueType const &value, | ||
| HdInstanceRegistryLock &®istryLock, | ||
| REGISTRY *registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, it was enough for the instance to hold a pointer to the hash map container owned by the registry, because it would just directly put values in it. But now the registry can implement a custom persistence mechanism, so we need the instance to hold a pointer to the registry and call its methods instead.
| /// is used to present a consistent interface to clients in cases | ||
| /// where shared resource registration is disabled. | ||
| explicit HdInstance(KeyType const &key) | ||
| explicit HdInstance(HdInstanceKey key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know that the key is always just an integer, so it's better to pass it around by value.
| /// The DERIVED template parameter can be used in a Curiously Recurring | ||
| /// Template Pattern to extend this class with a persistent cache backing the | ||
| /// in-memory dictionary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this opens up the possibility to use the same pattern for other types of resources.
| /// Copy constructor. Need as HdInstanceRegistryBase is placed in a map | ||
| /// and mutex is not copy-constructible, so can't use default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't seem to be correct: these objects are never stored by value in the USD codebase, so this constructor shouldn't be necessary and this class should comply with the rule of zero (right now, it doesn't: the copy operator is deleted below but it's copy-constructible).
| return it; | ||
| } | ||
|
|
||
| VALUE value = static_cast<DERIVED*>(this)->LoadFromDisk(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading from disk, if implemented by the derived class.
| pxr_register_test(testHdStMaterialXShaderGen_testCacheSerialization | ||
| COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testHdStMaterialXShaderGen --testCacheSerialization" | ||
| EXPECTED_RETURN_CODE 0 | ||
| STDOUT_REDIRECT shadergen_testCacheSerialization.out | ||
| DIFF_COMPARE shadergen_testCacheSerialization.out | ||
| TESTENV testHdStMaterialXShaderGen | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new test that tests if metadata is serialized correctly by the new MaterialX codegen cache. It reuses an existing test executable testHdStMaterialXShaderGen with a new command-line argument.
| #ifdef PXR_MATERIALX_SUPPORT_ENABLED | ||
| if (!isVolume) { | ||
| _materialXGfx = HdSt_ApplyMaterialXFilter(&surfaceNetwork, materialId, | ||
| _materialXCodegenResult = HdSt_ApplyMaterialXFilter(&surfaceNetwork, materialId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to return a shared pointer to a MaterialX::Shader object. Unfortunately, a MaterialX::Shader is not serializable, and source code is only one part of its state. So, to make caching feasible, I extract all the data that the downstream pipeline needs from MaterialX::Shader, encapsulate it in an object called "codegen result" and cache that to disk.
| HdSt_MaterialParamVector const& fallbackParams = | ||
| mxCodegenResult.GetFallbackParams(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallback parameters are one necessary part of the codegen result.
| // MaterialX parameter Information | ||
| const auto* variable = paramsBlock[i]; | ||
| const auto varType = HdStMaterialXHelpers::GetMxTypeDesc(variable); | ||
| for (HdSt_MaterialParam const& fallbackParam : fallbackParams) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code was populating the fallback parameters from the MaterialX shader.
In the new code, they are retrieved from MaterialX in the codegen result's constructor if codegen takes place at run time, or read from disk if the codegen happened on a previous run and was cached, and stored in the codegen result object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! :)
| // MaterialX glslfxShader. | ||
| else { | ||
| std::string separator; | ||
| const auto varValue = variable->getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this has moved to the codegen result's constructor.
| if (!param.fallbackValue.IsEmpty()) { | ||
| materialParams->push_back(std::move(param)); | ||
| } | ||
| for (TfToken textureParamName : mxCodegenResult.GetTextureParams()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, texture parameters are cached in the codegen result.
| mx::ShaderPtr mxShader = HdSt_GenMaterialXShader( | ||
| mtlxDoc, stdLibraries, searchPaths, mxHdInfo, apiName); | ||
|
|
||
| return std::make_shared<HdSt_MaterialXCodegenResult>(*mxShader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a codegen result from the MaterialX Shader, and destroying the latter because we've already extracted everything we will ever need from it.
| // TfHashAppend hashes TfTokens not as strings, but as pointers to interned | ||
| // strings which are not stable from run to run | ||
| void _AppendPersistentHash( | ||
| Tf_HashState& hashState, TfToken const& token) | ||
| { | ||
| TfHashAppend(hashState, token.GetString()); | ||
| } | ||
|
|
||
| // A VtValue may store a TfToken, in which case we need to convert it to a | ||
| // string in order to avoid the same pitfall as above | ||
| void _AppendPersistentHash(Tf_HashState& hashState, VtValue const& vtValue) | ||
| { | ||
| if (vtValue.IsHolding<TfToken>()) { | ||
| TfHashAppend(hashState, vtValue.Get<TfToken>().GetString()); | ||
| } else { | ||
| TfHashAppend(hashState, vtValue.GetHash()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without these workarounds, the cache always generated new, random hashes, and could never find an existing file in the cache directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to dig around internally and see if we have a function that does this already. If not, I may try to promote something like this to pxr/base. I think there are definitely some other cases where the TfHash isn't what you want for a fingerprint, though I'm not sure whether you'd hit them in a material network traversal.
| HdSt_MaterialXShaderRegistry* const materialXShaderRegistry = | ||
| resourceRegistry->GetMaterialXShaderRegistry(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a specialized instance registry that supports a persistent cache. To minimize include dependencies, I expose it to clients of HdStResourceRegistry by pointer.
| TF_DEFINE_ENV_SETTING(HDST_MTLX_CODEGEN_CACHE_DIR_PATH, "", | ||
| "Path to the directory of the persistent MaterialX codegen cache"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Environment variable which can be used to set the path to the directory where the cached files are written. It serves as the default value for an HdSt render setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as my guiding light for this kind of feature, I'm using the NVidia GL shader disk cache, which I think gets configured by environment variable or the magic nvidia control panel.
From Autodesk's perspective, what kind of control interface would you like for the cache? Env var? Something in C++ so you can put it in a settings panel? Plugin-based so people can configure weird site-specific multilevel caches or something? Curious to hear what the long term plans for something like this would be.
| bool val = false; | ||
| issValue >> val; | ||
| return VtValue(val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These implementations mostly moved here from materialXFilter.cpp
| { _tokens->name, JsValue(param.name) }, | ||
| { _tokens->type, JsValue(param.fallbackValue.GetTypeName()) }, | ||
| { _tokens->value, JsValue(osValue.str()) } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serializing parameters to the JSON file.
| HdRenderSettingDescriptor{ | ||
| "Path to the directory of the persistent MaterialX codegen cache", | ||
| HdStRenderSettingsTokens->hdstMtlxCodegenCacheDirPath, | ||
| VtValue(HdSt_MaterialXShaderRegistry::GetCacheDirPathEnvSetting()) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache directory exposed as a Storm render setting. The env var supplies the default value.
| _HgiToResourceRegistryMap::GetInstance().GetOrCreateRegistry( | ||
| _hgi | ||
| #ifdef PXR_MATERIALX_SUPPORT_ENABLED | ||
| , cacheDirPath.c_str() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the render setting to the registry.
| , _materialXShaderRegistry( | ||
| std::make_unique<HdSt_MaterialXShaderRegistry>(mtlxCacheDirPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MaterialX shader registry is now owned by unique pointer to simplify include dependencies.
| TfToken(name), value); | ||
| }; | ||
|
|
||
| addFallbackParam("BoolParamTrue", VtValue(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the serialization/deserialization of all supported data types.
| std::move(textureParams)); | ||
|
|
||
| std::stringstream ss; | ||
| codegenResult0.SaveMetadata(ss); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serializing the codegen result we've just populated to a string.
|
|
||
| JsValue jsMetadata = JsParseStream(ss); | ||
|
|
||
| HdSt_MaterialXCodegenResult codegenResult1( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deserializing into a new codegen result object.
| ((stormMsaaSampleCount, "storm:msaaSampleCount")) | ||
|
|
||
| #ifdef PXR_MATERIALX_SUPPORT_ENABLED | ||
| #define HDST_MTLX_CODEGEN_CACHE_DIR_PATH_TOKEN (hdstMtlxCodegenCacheDirPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A token for the render setting's name.
|
Filed as internal issue #USD-11070 (This is an automated message. See here for more information.) |
When testing [parallel MaterialX codegen in Storm](PixarAnimationStudios/OpenUSD#3567), and comparing the generated code between test runs using a [persistent MaterialX cache](PixarAnimationStudios/OpenUSD#3661), I noticed that the results were varying around `float` literal formatting. It turned out to be due to a data race on `static` variables controlling the formatting settings. This is similar to #2378 but limited to multithreaded codegen.
tcauchois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! I think the MaterialXCodeGenResult refactor is landable now, but left some comments about the disk-backed instance registry.
| public: | ||
| friend class HdInstanceRegistryBase<VALUE, HdInstanceRegistry<VALUE>>; | ||
|
|
||
| void SaveToDisk( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're anticipating the need for a more sophisticated management layer for the cache, and think that should live outside of hdSt. We have a big requirements list for it (including stuff like location on disk, size limit, eviction policy, security concerns, fingerprinting, hooks for testing); we don't expect you to hit all of that, but we also don't think the instance registry is a good place to park that complexity, since it's just a bit of memoization code.
If the control flow works out (which it looks like it does) I think the idea of the instance registry calling out to the persistent cache seems neat, though! But I wonder if there's a way to get that extensibility without all of the recurrent templates.
| // MaterialX parameter Information | ||
| const auto* variable = paramsBlock[i]; | ||
| const auto varType = HdStMaterialXHelpers::GetMxTypeDesc(variable); | ||
| for (HdSt_MaterialParam const& fallbackParam : fallbackParams) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! :)
| // TfHashAppend hashes TfTokens not as strings, but as pointers to interned | ||
| // strings which are not stable from run to run | ||
| void _AppendPersistentHash( | ||
| Tf_HashState& hashState, TfToken const& token) | ||
| { | ||
| TfHashAppend(hashState, token.GetString()); | ||
| } | ||
|
|
||
| // A VtValue may store a TfToken, in which case we need to convert it to a | ||
| // string in order to avoid the same pitfall as above | ||
| void _AppendPersistentHash(Tf_HashState& hashState, VtValue const& vtValue) | ||
| { | ||
| if (vtValue.IsHolding<TfToken>()) { | ||
| TfHashAppend(hashState, vtValue.Get<TfToken>().GetString()); | ||
| } else { | ||
| TfHashAppend(hashState, vtValue.GetHash()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to dig around internally and see if we have a function that does this already. If not, I may try to promote something like this to pxr/base. I think there are definitely some other cases where the TfHash isn't what you want for a fingerprint, though I'm not sure whether you'd hit them in a material network traversal.
| TF_DEFINE_ENV_SETTING(HDST_MTLX_CODEGEN_CACHE_DIR_PATH, "", | ||
| "Path to the directory of the persistent MaterialX codegen cache"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as my guiding light for this kind of feature, I'm using the NVidia GL shader disk cache, which I think gets configured by environment variable or the magic nvidia control panel.
From Autodesk's perspective, what kind of control interface would you like for the cache? Env var? Something in C++ so you can put it in a settings panel? Plugin-based so people can configure weird site-specific multilevel caches or something? Curious to hear what the long term plans for something like this would be.
Description of Change(s)
This change extends the existing MaterialX shader registry (an in-memory cache) with a persistent on-disk cache as a performance optimization. If we've generated a MaterialX shader with a unique ID on a previous run of the application and encounter the same ID on a subsequent run, we avoid redoing the expensive codegen process and instead restore the necessary data from cached files.
The cache directory can be set with the
HDST_MTLX_CODEGEN_CACHE_DIR_PATHenvironment variable or thehdstMtlxCodegenCacheDirPathrender setting. It's left up to the application to manage the cache, such as:For reference, the optimization reduces the duration of
HdRenderIndex::SyncAllfor https://github.com/usd-wg/assets/tree/main/full_assets/OpenChessSet from about 550ms to about 250ms in our measurements.Link to proposal (if applicable)
Fixes Issue(s)
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)