Skip to content

fix(castore): introduce fast path while getting torrent metadata to reduce allocs#611

Open
sambhav-jain-16 wants to merge 4 commits intouber:masterfrom
sambhav-jain-16:torrent-metadata-alloc
Open

fix(castore): introduce fast path while getting torrent metadata to reduce allocs#611
sambhav-jain-16 wants to merge 4 commits intouber:masterfrom
sambhav-jain-16:torrent-metadata-alloc

Conversation

@sambhav-jain-16
Copy link
Copy Markdown
Collaborator

@sambhav-jain-16 sambhav-jain-16 commented May 2, 2026

What?

TorrentMeta.Serialize/Deserialize round-trip svia JSON (json.Marshal + json.Unmarshal) for a MetaInfo. For every blob this allocates a fresh []byte buffer and a new *core.MetaInfo on every call to GetCacheFileMetadata. The fast path short-circuits it when the entry is already in the memory cache it hands back the existing *core.MetaInfo pointer directly, so a metadata read becomes a single pointer assignment with zero heap allocations.

Why only TorrentMetadata ?

TorrentMeta is the only metadata type whose backing value (*core.MetaInfo) is immutable after creation. Other metadata types (e.g. LastAccessTime) are mutable values written on every access.

Testing

Added a benchmark BenchmarkGetCacheFileMetadata_MemoryCache with different blob sizes and metadata sizes
Following is the benchstat comparison.

goos: linux
goarch: amd64
pkg: github.com/uber/kraken/lib/store
cpu: AMD EPYC 7B13
                                                       │ /home/user/kraken/bench-results/run-20260502-161433-n50/before.txt │ /home/user/kraken/bench-results/run-20260502-161433-n50/after.txt │
                                                       │                               sec/op                               │                  sec/op                    vs base                │
GetCacheFileMetadata_MemoryCache/1MB_4pc-96                                                                   7503.00n ± 1%                                 57.97n ± 1%  -99.23% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_64pc-96                                                                27221.50n ± 1%                                 56.61n ± 1%  -99.79% (n=50)
GetCacheFileMetadata_MemoryCache/64MB_256pc-96                                                               89182.50n ± 1%                                 57.67n ± 1%  -99.94% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_4pc_4MBpc-96                                                            7256.50n ± 1%                                 56.99n ± 3%  -99.21% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_16pc_1MBpc-96                                                          11599.50n ± 1%                                 61.23n ± 2%  -99.47% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_1024pc_16KBpc-96                                                      334380.00n ± 2%                                 61.69n ± 4%  -99.98% (p=0.000 n=50)
geomean                                                                                                         28.29µ                                      58.66n       -99.79%

                                                       │ /home/user/kraken/bench-results/run-20260502-161433-n50/before.txt │ /home/user/kraken/bench-results/run-20260502-161433-n50/after.txt │
                                                       │                                B/op                                │                       B/op                         vs base        │
GetCacheFileMetadata_MemoryCache/1MB_4pc-96                                                                   1907.000 ± 0%                                          8.000 ± 0%  -99.58% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_64pc-96                                                                 5103.000 ± 0%                                          8.000 ± 0%  -99.84% (n=50)
GetCacheFileMetadata_MemoryCache/64MB_256pc-96                                                               16494.500 ± 0%                                          8.000 ± 0%  -99.95% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_4pc_4MBpc-96                                                            1898.000 ± 0%                                          8.000 ± 0%  -99.58% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_16pc_1MBpc-96                                                           2732.000 ± 0%                                          8.000 ± 0%  -99.71% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_1024pc_16KBpc-96                                                       67460.500 ± 0%                                          8.000 ± 0%  -99.99% (n=50)
geomean                                                                                                        6.043Ki                                               8.000       -99.87%

                                                       │ /home/user/kraken/bench-results/run-20260502-161433-n50/before.txt │ /home/user/kraken/bench-results/run-20260502-161433-n50/after.txt │
                                                       │                             allocs/op                              │                     allocs/op                      vs base        │
GetCacheFileMetadata_MemoryCache/1MB_4pc-96                                                                     37.000 ± 0%                                          1.000 ± 0%  -97.30% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_64pc-96                                                                  103.000 ± 0%                                          1.000 ± 0%  -99.03% (n=50)
GetCacheFileMetadata_MemoryCache/64MB_256pc-96                                                                 299.000 ± 0%                                          1.000 ± 0%  -99.67% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_4pc_4MBpc-96                                                              37.000 ± 0%                                          1.000 ± 0%  -97.30% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_16pc_1MBpc-96                                                             52.000 ± 0%                                          1.000 ± 0%  -98.08% (n=50)
GetCacheFileMetadata_MemoryCache/16MB_1024pc_16KBpc-96                                                        1072.000 ± 0%                                          1.000 ± 0%  -99.91% (n=50)
geomean                                                                                                          115.3                                               1.000       -99.13%

Copilot AI review requested due to automatic review settings May 2, 2026 15:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves CAStore.GetCacheFileMetadata performance when serving torrent metadata from the in-memory cache by avoiding per-read serialization/deserialization, and adds coverage/benchmarks to validate allocation behavior.

Changes:

  • Add a fast path in CAStore.GetCacheFileMetadata to return the cached *core.MetaInfo pointer directly for *metadata.TorrentMeta.
  • Add a unit test asserting the no-copy behavior when reading metadata from the memory cache.
  • Add a benchmark suite to measure allocations/time across different blob sizes and piece counts for memory-cache metadata reads.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/store/ca_store.go Returns cached *core.MetaInfo directly for torrent metadata reads from memory cache, avoiding JSON round-trips.
lib/store/ca_store_test.go Adds a no-copy unit test and a benchmark measuring metadata read allocations across scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/store/ca_store_test.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/store/ca_store_test.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sambhav-jain-16 sambhav-jain-16 changed the title Torrent metadata alloc fix(castore): introduce fast patch while getting torrent metadata to reduce allocs May 3, 2026
@sambhav-jain-16 sambhav-jain-16 marked this pull request as ready for review May 3, 2026 10:23
@sambhav-jain-16 sambhav-jain-16 changed the title fix(castore): introduce fast patch while getting torrent metadata to reduce allocs fix(castore): introduce fast path while getting torrent metadata to reduce allocs May 3, 2026
Copy link
Copy Markdown
Collaborator

@thijmv thijmv left a comment

Choose a reason for hiding this comment

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

A couple of comments:

Comment thread lib/store/ca_store.go
Comment on lines +485 to 490
// Defensive fallback for any future TorrentMeta-suffix metadata type.
b, err := entry.MetaInfo.Serialize()
if err != nil {
return fmt.Errorf("serialize metainfo: %s", err)
}
return md.Deserialize(b)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to remove the serialisation path until other metadata types are added and return an error instead? In the current implementation, this path would yield a silent invariant violation, as torrent metadata is currently the only supported type.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I want to be as defensive as possible with this change to avoid any regressions. Since this is the current production behavior, let's keep it this way, if you feel strongly against it, we can discuss more.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This function is also called for metadata.Persist here

var pm metadata.Persist
if err := s.cas.GetCacheFileMetadata(name, &pm); err != nil && !os.IsNotExist(err) {
return false, fmt.Errorf("store: %s", err)
}

It's better we keep it this way to be cleaner

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this PR aims to remove the need for serialisation by returning a metadata pointer instead, would it not make more sense to either completely remove it or at least log an error as we expect that path to never be reached?

When we call it using metadata.Persist, the metadata is expected to be fetched from disk. Could you elaborate on the relevance?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@sambhav-jain-16 Do I understand correctly that ALWAYS expect the if check to be true? If so, can we emit an error log in the else case, so we catch a potential invariant violation if it happens?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@thijmv @Anton-Kalpakchiev
I took another look, and I agree with your suggestions.

When we call it using metadata.Persist, the metadata is expected to be fetched from disk. Could you elaborate on the relevance?

I agree on this, given that as of now we are not using the in-mem cache unto upload path, this case shouldn't happen.

I'll make the changes and update

Comment thread lib/store/ca_store_test.go
Comment thread lib/store/ca_store_test.go Outdated
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.

4 participants