Skip to content

Commit

Permalink
Fix strong reference count for cache pools held in a Context
Browse files Browse the repository at this point in the history
Previously, cache pools held in a Context were incorrectly held only by
a weak ptr, rather than a strong ptr, leading to presently unused caches being
prematurely evicted.

PiperOrigin-RevId: 657675274
Change-Id: I4e59299493c8d95431b95f88fff380d304f2da91
  • Loading branch information
jbms authored and copybara-github committed Jul 30, 2024
1 parent 2f65bf2 commit 028dd25
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 3 deletions.
2 changes: 1 addition & 1 deletion tensorstore/context_resource_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class ContextResourceTraitsConcept : public ContextResourceTraits<Provider> {
/// If there are no nested context resources, this need not be defined.
void UnbindContext(Spec& spec, const ContextSpecBuilder& builder) const;

/// Optional. Increments the strong reference count associated with
/// Optional. Increments the strong reference count.
///
/// A `Resource` object is assumed to represent a weak reference by default.
/// This method acquires an additional strong reference, if supported. If
Expand Down
49 changes: 49 additions & 0 deletions tensorstore/driver/zarr3/driver_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1361,4 +1361,53 @@ TEST(FullShardWriteTest, WithoutTransaction) {
TENSORSTORE_ASSERT_OK(future);
}

TEST(ZarrDriverTest, MetadataCache) {
TENSORSTORE_ASSERT_OK_AND_ASSIGN(
auto context,
Context::FromJson(
{{"cache_pool", {{"total_bytes_limit", 1024 * 1024 * 10}}}}));

TENSORSTORE_ASSERT_OK_AND_ASSIGN(
auto mock_key_value_store_resource,
context.GetResource<tensorstore::internal::MockKeyValueStoreResource>());
auto mock_kvstore = *mock_key_value_store_resource;
mock_kvstore->forward_to = tensorstore::GetMemoryKeyValueStore();
mock_kvstore->log_requests = true;

::nlohmann::json json_spec{
{"driver", "zarr3"},
{"kvstore",
{
{"driver", "mock_key_value_store"},
{"path", "prefix/"},
}},
{"schema",
{
{"domain", {{"shape", {4}}}},
{"dtype", "uint16"},
}},
{"recheck_cached_metadata", false},
{"recheck_cached_data", false},
};

TENSORSTORE_ASSERT_OK_AND_ASSIGN(auto spec,
tensorstore::Spec::FromJson(json_spec));

{
TENSORSTORE_ASSERT_OK_AND_ASSIGN(
auto store,
tensorstore::Open(spec, context, tensorstore::OpenMode::create)
.result());
mock_kvstore->request_log.pop_all();
}

{
// Reopening uses cached metadata without revalidation.
TENSORSTORE_ASSERT_OK_AND_ASSIGN(
auto store,
tensorstore::Open(spec, context, tensorstore::OpenMode::open).result());
EXPECT_THAT(mock_kvstore->request_log.pop_all(), ::testing::SizeIs(0));
}
}

} // namespace
4 changes: 2 additions & 2 deletions tensorstore/internal/cache/cache_pool_resource.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ struct CachePoolResourceTraits
static Spec GetSpec(const Resource& pool, const ContextSpecBuilder& builder) {
return pool->limits();
}
static void AcquireStrongReference(const Resource& p) {
static void AcquireContextReference(const Resource& p) {
internal_cache::StrongPtrTraitsCachePool::increment(p.get());
}
static void ReleaseStrongReference(const Resource& p) {
static void ReleaseContextReference(const Resource& p) {
internal_cache::StrongPtrTraitsCachePool::decrement(p.get());
}
};
Expand Down

0 comments on commit 028dd25

Please sign in to comment.