Skip to content

(fix): cache partial decoder #93

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

Merged
merged 10 commits into from
Apr 10, 2025
Merged

(fix): cache partial decoder #93

merged 10 commits into from
Apr 10, 2025

Conversation

ilan-gold
Copy link
Owner

Fixes #90 (hopefully) - I benchmarked and performance (at least relatively speaking) did not go down, but also did not go up, although our benchmarks are probably not really checking for this behavior.

I went with HashMap because I couldn't see a reason to use BTreeMap (I see it's used elsewhere and that people in rust seem to use it as a drop-in replacement for HashMap because it's faster sometimes? Not really familiar with this TBH).

I am also not sure the key is the right thing to hash on - the "key" here would be a shard, and we want to share the decoder per shard, right?

@LDeakin
Copy link
Collaborator

LDeakin commented Apr 9, 2025

the "key" here would be a shard, and we want to share the decoder per shard, right?

Yep! The partial decoder for the sharding codec retrieves and decodes the shard index only once.

However, the partial decoder cache should only live as long as the batch of requests in retrieve_chunks_and_apply_index. Otherwise, it would perpetually grow and become stale after subsequent writes. I should document this better, but partial decoder initialisation can perform store and codec operations (and may even decode entire chunks with certain codecs!).

So my recommendation is to avoid locks and:

  1. Identify the unique chunks and create partial decoders for each in parallel.
  2. Run partial_decode_into in parallel for each chunk subset

@ilan-gold
Copy link
Owner Author

Separately there seems to be an error about typesize which I would guess is somehow related to my work on that zarr-developers/numcodecs#713

@ilan-gold
Copy link
Owner Author

Yup, https://github.com/zarr-developers/numcodecs/blob/58bc9c4da74e06fbec985d4235f912b31d4d31f6/numcodecs/abc.py#L79-L95 means it is now part of the configuration. So we can probably ignore it, will open a separate PR

@ilan-gold
Copy link
Owner Author

Seems like I did 1. by constructing a HashMap of the keys that require partial mapped to their WithSubset and then parallel-iterate to create tuples that have the key and the decoder before updating the HashMap representing the mapping of key to decoder. 2. Should already be handled

@ilan-gold ilan-gold requested a review from LDeakin April 9, 2025 15:52
src/lib.rs Outdated
Comment on lines 275 to 281
let mut item_map: HashMap<StoreKey, &WithSubset> = HashMap::new().into();
chunk_descriptions
.iter()
.filter(|item| !(is_whole_chunk(item)))
.for_each(|item| {
item_map.insert(item.key().clone(), item);
});
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a cleaner way to do this...

Copy link
Collaborator

@LDeakin LDeakin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just a minor nit on Option -> Result + ensuring that the chunk concurrent limit is used.

ilan-gold and others added 4 commits April 10, 2025 09:37
@ilan-gold ilan-gold enabled auto-merge (squash) April 10, 2025 07:39
@ilan-gold ilan-gold merged commit dabb30c into main Apr 10, 2025
13 of 14 checks passed
@ilan-gold ilan-gold deleted the ig/reuse_decoder branch April 10, 2025 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reuse partial decoders
2 participants