From 9942c0083b6426916c55ce2b93bc444f62d05ebc Mon Sep 17 00:00:00 2001 From: jorgep31415 Date: Tue, 19 Nov 2024 13:30:53 -0800 Subject: [PATCH 1/3] [ET-VK] Move `save_cache` from Runtime dtor to model destroy ## Issue `Runtime` is a local static variable. Hence we'd expect the Runtime dtor to be called on program exit. But on Android devices it's not being invoked. This behavior is different than that seen 6 months ago (D57085281). It's unclear what changed. This means the cache is not saved due to the following chain never being invoked. `~Runtime()` > `~Adapter()` > `~ComputePipelineCache()` > `save_cache()`.\ ## Solution Move cache saving to `VulkanBackend.cpp`'s model destroy. This makes sense since the cache is tied to the model and not the runtime. ## Resources https://medium.com/@martin00001313/mastering-static-objects-in-c-initialization-destruction-and-best-practices-760b17734195 Differential Revision: [D66179917](https://our.internmc.facebook.com/intern/diff/D66179917/) [ghstack-poisoned] --- backends/vulkan/runtime/VulkanBackend.cpp | 1 + backends/vulkan/runtime/vk_api/Pipeline.cpp | 2 -- backends/vulkan/runtime/vk_api/Pipeline.h | 3 ++- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backends/vulkan/runtime/VulkanBackend.cpp b/backends/vulkan/runtime/VulkanBackend.cpp index 14cc7df02a1..6c1162ae231 100644 --- a/backends/vulkan/runtime/VulkanBackend.cpp +++ b/backends/vulkan/runtime/VulkanBackend.cpp @@ -626,6 +626,7 @@ class VulkanBackend final : public ::executorch::runtime::BackendInterface { // here. compute_graph->~ComputeGraph(); } + api::context()->adapter_ptr()->compute_pipeline_cache().save_cache(); } }; diff --git a/backends/vulkan/runtime/vk_api/Pipeline.cpp b/backends/vulkan/runtime/vk_api/Pipeline.cpp index 07fc313984a..49bbf083359 100644 --- a/backends/vulkan/runtime/vk_api/Pipeline.cpp +++ b/backends/vulkan/runtime/vk_api/Pipeline.cpp @@ -406,8 +406,6 @@ ComputePipelineCache::~ComputePipelineCache() { return; } - save_cache(); - vkDestroyPipelineCache(device_, pipeline_cache_, nullptr); pipeline_cache_ = VK_NULL_HANDLE; } diff --git a/backends/vulkan/runtime/vk_api/Pipeline.h b/backends/vulkan/runtime/vk_api/Pipeline.h index 8765156deb2..4f42a9bf6bb 100644 --- a/backends/vulkan/runtime/vk_api/Pipeline.h +++ b/backends/vulkan/runtime/vk_api/Pipeline.h @@ -269,9 +269,10 @@ class ComputePipelineCache final { } }; + void save_cache(); + private: std::vector load_cache(); - void save_cache(); // Multiple threads could potentially be adding entries into the cache, so use // a mutex to manage access From 13a640c0101269fb9db44d91a0c3aeb9406652c6 Mon Sep 17 00:00:00 2001 From: jorgep31415 Date: Tue, 19 Nov 2024 13:50:56 -0800 Subject: [PATCH 2/3] Update on "[ET-VK] Move `save_cache` from Runtime dtor to model destroy" ## Issue `Runtime` is a local static variable. Hence we'd expect the Runtime dtor to be called on program exit. But on Android devices it's not being invoked. This behavior is different than that seen 6 months ago (D57085281). It's unclear what changed. This means the cache is not saved due to the following chain never being invoked. `~Runtime()` > `~Adapter()` > `~ComputePipelineCache()` > `save_cache()`.\ ## Solution Move cache saving to `VulkanBackend.cpp`'s model destroy. This makes sense since the cache is tied to the model and not the runtime. ## Resources https://medium.com/martin00001313/mastering-static-objects-in-c-initialization-destruction-and-best-practices-760b17734195 Differential Revision: [D66179917](https://our.internmc.facebook.com/intern/diff/D66179917/) [ghstack-poisoned] --- backends/vulkan/runtime/VulkanBackend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backends/vulkan/runtime/VulkanBackend.cpp b/backends/vulkan/runtime/VulkanBackend.cpp index 6c1162ae231..a3b991ac992 100644 --- a/backends/vulkan/runtime/VulkanBackend.cpp +++ b/backends/vulkan/runtime/VulkanBackend.cpp @@ -621,12 +621,12 @@ class VulkanBackend final : public ::executorch::runtime::BackendInterface { void destroy(DelegateHandle* handle) const override { if (handle != nullptr) { ComputeGraph* compute_graph = static_cast(handle); + compute_graph->context()->adapter_ptr()->compute_pipeline_cache().save_cache(); // ComputeGraph is not trivially destructible. Since // this was constructed manually in init(), we must destroy it manually // here. compute_graph->~ComputeGraph(); } - api::context()->adapter_ptr()->compute_pipeline_cache().save_cache(); } }; From a0909c2eec04b27c173b8447d8ddfc73f57f92d3 Mon Sep 17 00:00:00 2001 From: jorgep31415 Date: Tue, 19 Nov 2024 13:51:42 -0800 Subject: [PATCH 3/3] Update on "[ET-VK] Move `save_cache` from Runtime dtor to model destroy" ## Issue `Runtime` is a local static variable. Hence we'd expect the Runtime dtor to be called on program exit. But on Android devices it's not being invoked. This behavior is different than that seen 6 months ago (D57085281). It's unclear what changed. This means the cache is not saved due to the following chain never being invoked. `~Runtime()` > `~Adapter()` > `~ComputePipelineCache()` > `save_cache()`.\ ## Solution Move cache saving to `VulkanBackend.cpp`'s model destroy. This makes sense since the cache is tied to the model and not the runtime. ## Resources https://medium.com/martin00001313/mastering-static-objects-in-c-initialization-destruction-and-best-practices-760b17734195 Differential Revision: [D66179917](https://our.internmc.facebook.com/intern/diff/D66179917/) [ghstack-poisoned] --- backends/vulkan/runtime/VulkanBackend.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backends/vulkan/runtime/VulkanBackend.cpp b/backends/vulkan/runtime/VulkanBackend.cpp index a3b991ac992..5093d61d2f7 100644 --- a/backends/vulkan/runtime/VulkanBackend.cpp +++ b/backends/vulkan/runtime/VulkanBackend.cpp @@ -621,7 +621,10 @@ class VulkanBackend final : public ::executorch::runtime::BackendInterface { void destroy(DelegateHandle* handle) const override { if (handle != nullptr) { ComputeGraph* compute_graph = static_cast(handle); - compute_graph->context()->adapter_ptr()->compute_pipeline_cache().save_cache(); + compute_graph->context() + ->adapter_ptr() + ->compute_pipeline_cache() + .save_cache(); // ComputeGraph is not trivially destructible. Since // this was constructed manually in init(), we must destroy it manually // here.