perf: Merkle traits focus on hash, not full contents #450

snoyberg · 2025-08-10T15:56:49Z

This is a first step towards implement more aggressive caching of Lockable values, which will reduce overall memory usage for most Kolme applications. This commit does not compile or pass tests, further commits will continue refining this by adding the actual cache implementation to MerkleLockable, and then fixing compilation in the rest of the repo.

I'll continue pushing more commits to this PR to implement remaining functionality.

This is a first step towards implement more aggressive caching of Lockable values, which will reduce overall memory usage for most Kolme applications. This commit does _not_ compile or pass tests, further commits will continue refining this by adding the actual cache implementation to MerkleLockable, and then fixing compilation in the rest of the repo.

cloudflare-workers-and-pages · 2025-08-11T06:07:36Z

Deploying kolme with Cloudflare Pages

Latest commit:	`f911e61`
Status:	✅ Deploy successful!
Preview URL:	https://2e1cae1d.kolme.pages.dev
Branch Preview URL:	https://merkle-map-value-caching.kolme.pages.dev

View logs

snoyberg · 2025-08-11T06:10:47Z

This code took me a while to get working, and I finished it up on a plane, so I don't know how reliable it is. In theory it should reduce memory needs for Kolme apps by pushing more sharing of merkle map data.

Copilot

Pull Request Overview

This PR refactors the Merkle traits to focus on hash-based caching instead of full content caching, as a first step toward implementing more aggressive caching of Lockable values to reduce memory usage. The changes introduce a new hash-based caching system for MerkleLockable values while maintaining backward compatibility.

Key changes:

Replace get_merkle_contents() and set_merkle_contents() methods with hash-based equivalents
Introduce a new global cache system for MerkleLockable values using TypeId and hash as keys
Update API functions to return and work with hashes instead of full MerkleContents objects

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
packages/merkle-map/src/traits.rs	Replace content-based trait methods with hash-based equivalents
packages/merkle-map/src/impls/lockable/locked.rs	New cache implementation with TypeId-based key system
packages/merkle-map/src/impls/lockable.rs	Update MerkleLockable to use new hash-based caching
packages/merkle-map/src/api.rs	Modify save/load functions to work with hashes instead of contents
packages/merkle-map/tests/*.rs	Update tests to work with hash return values
packages/kolme-store/src/*.rs	Update trait signatures and implementations

Copilot · 2025-08-11T06:12:21Z

packages/merkle-map/src/impls/lockable/locked.rs

+    value: LockValue,
+}
+
+/// The cache itself


Missing documentation for the global cache. This static cache will persist for the entire application lifetime and could grow unbounded. Consider adding documentation about cache eviction policies and memory management.

Suggested change

/// The cache itself

/// The global cache for locked values.

///

/// This static cache persists for the entire application lifetime.

/// Entries are evicted when their associated [`CleanupLockKey`] is dropped,

/// which happens when there are no more strong references to the key.

/// As a result, memory usage is tied to the lifetime of these keys and their values.

/// There is no explicit upper bound on cache size; if keys are never dropped,

/// the cache may grow unbounded. Care should be taken to avoid memory leaks

/// by ensuring keys are released when no longer needed.

Copilot · 2025-08-11T06:12:21Z

packages/merkle-map/src/impls/lockable/locked.rs

+}
+
+impl<T: Send + Sync + 'static> Locked<T> {
+    pub(super) fn new(lock_key: LockKey, inner: Arc<T>) -> Self {


Potential race condition in the double-checked locking pattern. After acquiring the write lock, the code should check again if the value exists before inserting to avoid duplicate insertions.

Copilot · 2025-08-11T06:12:22Z

packages/merkle-map/src/impls/lockable.rs

+        let once_locked = OnceLock::new();
+        assert!(once_locked.set(locked).is_ok());
+        Some(Self {
+            locked: Arc::new(once_locked),


[nitpick] The variable name 'once_locked' is confusing since it's not locked yet. Consider renaming to 'once_cell' or 'lock_cell' to better reflect its purpose as a container.

Suggested change

locked: Arc::new(once_locked),

let once_cell = OnceLock::new();

assert!(once_cell.set(locked).is_ok());

Some(Self {

locked: Arc::new(once_cell),

Copilot · 2025-08-11T06:12:22Z

packages/merkle-map/src/impls/lockable.rs

+        let locked = Locked::<T>::get(Self::lock_key_for(hash))?;
+        let inner = locked.value().clone();
+        let once_locked = OnceLock::new();
+        assert!(once_locked.set(locked).is_ok());


Using assert! for error handling in production code is problematic. If the OnceLock is already set, this will panic. Consider using expect() with a descriptive message or proper error handling.

Suggested change

assert!(once_locked.set(locked).is_ok());

once_locked.set(locked).expect("OnceLock was already set when loading MerkleLockable by hash");

Copilot · 2025-08-11T06:12:22Z

packages/merkle-map/src/impls/node.rs

+        if let Some(leaf) = MerkleDeserializeRaw::load_merkle_by_hash(hash) {
+            Some(Node::Leaf(leaf))
+        } else {
+            MerkleDeserializeRaw::load_merkle_by_hash(hash).map(Node::Tree)


[nitpick] The type inference for the first branch is unclear. Consider explicitly specifying the type or adding a comment to clarify that this attempts to load as a leaf first, then as a tree.

Suggested change

MerkleDeserializeRaw::load_merkle_by_hash(hash).map(Node::Tree)

// Try to load as a leaf first; if that fails, try to load as a tree.

if let Some(leaf) = <MerkleLockable<LeafContents<K, V>> as MerkleDeserializeRaw>::load_merkle_by_hash(hash) {

Some(Node::Leaf(leaf))

} else {

<MerkleLockable<TreeContents<K, V>> as MerkleDeserializeRaw>::load_merkle_by_hash(hash).map(Node::Tree)

Copilot · 2025-08-11T06:12:22Z

packages/merkle-map/tests/deep.rs

+    let hash = save(&mut store, &m).await.unwrap();
+    let contents = merkle_map::api::load_merkle_contents(&mut store, hash)
+        .await
+        .unwrap();


This test modification introduces a dependency on an internal API function 'load_merkle_contents'. Consider using public APIs or extracting this functionality into a test helper to avoid tight coupling with internals.

Suggested change

.unwrap();

let contents = load_merkle_contents_helper(&mut store, hash).await.unwrap();

Base automatically changed from disable-mold-on-mac to main August 10, 2025 17:04

snoyberg added 2 commits August 10, 2025 21:37

feat: value caching for MerkleLockable

29e6725

fix: modify rest of repo to match new type signatures

2e45d06

snoyberg requested review from psibi and Copilot August 11, 2025 06:09

snoyberg marked this pull request as ready for review August 11, 2025 06:10

Copilot AI reviewed Aug 11, 2025

View reviewed changes

snoyberg added 2 commits August 11, 2025 09:13

test: add the maps-are-cached test case

e0dda3d

Merge remote-tracking branch 'origin/main' into merkle-map-value-caching

f911e61

psibi approved these changes Aug 11, 2025

View reviewed changes

snoyberg merged commit b7670bf into main Aug 11, 2025
3 checks passed

snoyberg deleted the merkle-map-value-caching branch August 11, 2025 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf: Merkle traits focus on hash, not full contents #450

perf: Merkle traits focus on hash, not full contents #450

Uh oh!

snoyberg commented Aug 10, 2025

Uh oh!

cloudflare-workers-and-pages bot commented Aug 11, 2025 •

edited

Loading

Uh oh!

snoyberg commented Aug 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 11, 2025

Uh oh!

Copilot AI Aug 11, 2025

Uh oh!

Copilot AI Aug 11, 2025

Uh oh!

Copilot AI Aug 11, 2025

Uh oh!

Copilot AI Aug 11, 2025

Uh oh!

Copilot AI Aug 11, 2025

Uh oh!

Uh oh!

Uh oh!

-/// The cache itself
+/// The global cache for locked values.
+///
+/// This static cache persists for the entire application lifetime.
+/// Entries are evicted when their associated [`CleanupLockKey`] is dropped,
+/// which happens when there are no more strong references to the key.
+/// As a result, memory usage is tied to the lifetime of these keys and their values.
+/// There is no explicit upper bound on cache size; if keys are never dropped,
+/// the cache may grow unbounded. Care should be taken to avoid memory leaks
+/// by ensuring keys are released when no longer needed.

-            locked: Arc::new(once_locked),
+        let once_cell = OnceLock::new();
+        assert!(once_cell.set(locked).is_ok());
+        Some(Self {
+            locked: Arc::new(once_cell),

	assert!(once_locked.set(locked).is_ok());
	once_locked.set(locked).expect("OnceLock was already set when loading MerkleLockable by hash");

	.unwrap();
	let contents = load_merkle_contents_helper(&mut store, hash).await.unwrap();

perf: Merkle traits focus on hash, not full contents #450

perf: Merkle traits focus on hash, not full contents #450

Uh oh!

Conversation

snoyberg commented Aug 10, 2025

Uh oh!

cloudflare-workers-and-pages bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying kolme with Cloudflare Pages

Uh oh!

snoyberg commented Aug 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Aug 11, 2025 •

edited

Loading