fix: now farmer piece cache sync percentage base on total cache size #3654

tediou5 · 2025-07-29T09:38:52Z

Thanks to the previous refactoring, it's now easy to retrieve stored_pieces.
Added a helper function to retrieve stored_pieces.

// first start
2025-07-29T08:45:00.887876Z DEBUG subspace_farmer::farmer_cache: Identified piece indices that should be cached count=10134
2025-07-29T08:48:52.478604Z  INFO subspace_farmer::farmer_cache: Piece cache sync 0.00% complete (0 B / 9.9 GiB)
2025-07-29T09:03:02.331834Z  INFO subspace_farmer::farmer_cache: Piece cache sync 0.10% complete (100.0 MiB / 9.8 GiB)

// restart

// before
2025-07-29T09:05:40.406689Z DEBUG subspace_farmer::farmer_cache: Identified piece indices that should be cached count=10034
2025-07-29T09:06:54.225429Z  INFO subspace_farmer::farmer_cache: Piece cache sync 0.00% complete (0 B / 9.8 GiB)
2025-07-29T09:07:39.959417Z  INFO subspace_farmer::farmer_cache: Piece cache sync 0.10% complete (10.0 MiB / 9.8 GiB)

// now
2025-07-29T09:12:54.610036Z DEBUG subspace_farmer::farmer_cache: Identified piece indices that should be cached stored_count=102 count=10024
2025-07-29T09:13:36.207475Z  INFO subspace_farmer::farmer_cache: Piece cache sync 1.09% complete (110.0 MiB / 9.9 GiB)
2025-07-29T09:16:00.496766Z  INFO subspace_farmer::farmer_cache: Piece cache sync 1.19% complete (120.0 MiB / 9.9 GiB)

// again
2025-07-29T09:33:30.134762Z DEBUG subspace_farmer::farmer_cache: Identified piece indices that should be cached stored_count=122 count=10004
2025-07-29T09:36:39.527984Z  INFO subspace_farmer::farmer_cache: Piece cache sync 1.28% complete (130.0 MiB / 9.9 GiB)

As you can see from the log changes, a restart no longer resets the percentage calculation but instead continues from the previous count; additionally, stored_count is now printed in the Debug log.

(For testing purposes, I set INTERMEDIATE_CACHE_UPDATE_INTERVAL to 10 for convenience; under normal circumstances, the printing interval is much longer)

Code contributor checklist:

I have read, understood and followed contributing guide

immunefi-magnus · 2025-07-29T09:38:57Z

🛡️ Immunefi PR Reviews

We noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below:

🔗 Send this PR in for review

Once submitted, we'll take care of assigning a reviewer and follow up here.

tediou5 · 2025-07-29T09:46:03Z

However, I noticed during testing that the byte sizes sometimes differ (e.g., 9.9Gib or 9.8Gib). I haven't looked into this issue closely, but perhaps it's caused by precision loss?

From my perspective, I think it might be more intuitive to directly use the number of pieces here.

teor2345

However, I noticed during testing that the byte sizes sometimes differ (e.g., 9.9Gib or 9.8Gib). I haven't looked into this issue closely, but perhaps it's caused by precision loss?

A slight variation in cache size seems like a low priority, but it's worth opening a ticket in case it's caused by a more serious bug.

Can you see any places where that precision loss could be happening? All the calculations we're doing are in integers, and the piece cache size calculations are deterministic.

The byte_size formatting uses f64:
https://docs.rs/bytesize/1.3.3/src/bytesize/lib.rs.html#193

but it is simple code that should be deterministic, as long as the total piece cache capacity is the same. (Which is something we should confirm.)

Let's treat this as a separate bug? We can add debug logs for it in this PR.

If the change was happening in the number of pieces downloaded, or to download, it could be a race condition. But there's no concurrency on the total cache capacity.

Let's open another ticket and post full debug/trace logs there?

From my perspective, I think it might be more intuitive to directly use the number of pieces here.

Most users don't know how big a piece is, but logging gigabytes will give them a rough idea of how many pieces still need to be downloaded.

teor2345 · 2025-07-29T20:35:37Z

crates/subspace-farmer/src/farmer_cache.rs

            count = %piece_indices_to_store.len(),
            "Identified piece indices that should be cached",
        );

-        let pieces_to_download_total = piece_indices_to_store.len();
+        let pieces_to_download_total = piece_indices_to_store.len() + stored_count;


Try replacing this with caches.total_capacity(), because that's a simpler calculation.

And debug log the total capacity above, to help diagnose the variable maximum capacity bug.

Using caches.total_capacity() here might not be suitable. For the vast majority of farmers, the total_capacity will likely never be filled. I think it's fine to use for display purposes, but using it to calculate a percentage for synchronization progress could be misleading.

For the vast majority of farmers, the total_capacity will likely never be filled.

Well, that's once source of variability: if the attached node is synced to a different segment index, then the total available number of pieces will change, so the target number of pieces might also change.

Please also add a debug log of the total cache capacity.

Please also add a debug log of the total cache capacity.

Currently, total_capacity is PieceCache::max_num_elements, which is the max number of pieces in this cache. So, should I display this number directly? Or should I display the bytesize similar to your logic below?

This PR is about to merge, and it's not that important, so let's skip it for now.

But if we add it in future, matching the other numbers in the debug log is more useful. So if it went in the "Identified piece indices that should be cached" debug log, it would be a piece count.

crates/subspace-farmer/src/farmer_cache/piece_cache_state.rs

tediou5

Thank you for your review.
I'll try to reproduce the byte_size issue separately with a unit test. If it warrants further discussion, I'll open a separate issue for it. I agree that this is a low-priority issue.

tediou5 · 2025-07-30T01:24:06Z

crates/subspace-farmer/src/farmer_cache.rs

            count = %piece_indices_to_store.len(),
            "Identified piece indices that should be cached",
        );

-        let pieces_to_download_total = piece_indices_to_store.len();
+        let pieces_to_download_total = piece_indices_to_store.len() + stored_count;


Using caches.total_capacity() here might not be suitable. For the vast majority of farmers, the total_capacity will likely never be filled. I think it's fine to use for display purposes, but using it to calculate a percentage for synchronization progress could be misleading.

crates/subspace-farmer/src/farmer_cache/piece_cache_state.rs

tediou5 · 2025-07-30T02:40:30Z

A slight variation in cache size seems like a low priority, but it's worth opening a ticket in case it's caused by a more serious bug.

Oh, I've found the reason. It's caused by another project on my server. Please ignore this issue.

vedhavyas

LGTM

crates/subspace-farmer/src/farmer_cache.rs

teor2345

Thanks!

…rcentage base on total cache size)

teor2345 · 2025-07-31T23:52:18Z

Thank you again for this change, running a farmer feels much better with logs like:

2025-07-30T04:21:14.424060Z INFO subspace_farmer::farmer_cache: Piece cache sync 90.34% complete (135.1 GiB / 149.5 GiB)
...
2025-07-30T05:17:23.632911Z INFO subspace_farmer::farmer_cache: Piece cache sync 92.04% complete (137.6 GiB / 149.5 GiB)

Previously that would have shown as:

2025-07-30T04:21:14.424060Z INFO subspace_farmer::farmer_cache: Piece cache sync 0.00% complete (0.0 GiB / 14.4 GiB)
...
2025-07-30T05:17:23.632911Z INFO subspace_farmer::farmer_cache: Piece cache sync 17.36% complete (2.5 GiB / 14.4 GiB)

Which doesn't give much sense of true farmer progress.

fix: now farmer piece cache sync percentage base on total cache size

423b6cc

tediou5 requested a review from nazar-pc as a code owner July 29, 2025 09:38

tediou5 mentioned this pull request Jul 29, 2025

Farmer piece cache percentage is based on remaining pieces, which is confusing after a restart #3614

Closed

teor2345 self-assigned this Jul 29, 2025

teor2345 self-requested a review July 29, 2025 21:08

teor2345 added bug Something isn't working farmer Farming library/app nice-to-have Non-critical but nice to have labels Jul 29, 2025

teor2345 reviewed Jul 29, 2025

View reviewed changes

tediou5 commented Jul 30, 2025

View reviewed changes

tediou5 requested a review from teor2345 July 30, 2025 01:37

vedhavyas reviewed Jul 30, 2025

View reviewed changes

teor2345 reviewed Jul 30, 2025

View reviewed changes

crates/subspace-farmer/src/farmer_cache.rs Outdated Show resolved Hide resolved

chore: calculate stored_count using stored_pieces_offsets()

83f36b9

tediou5 force-pushed the fix/switching-percent-total-cache-size branch from 38a715a to 83f36b9 Compare July 30, 2025 04:03

teor2345 enabled auto-merge July 30, 2025 04:08

teor2345 disabled auto-merge July 30, 2025 04:08

teor2345 enabled auto-merge July 30, 2025 04:08

teor2345 approved these changes Jul 30, 2025

View reviewed changes

teor2345 added this pull request to the merge queue Jul 30, 2025

Merged via the queue into autonomys:main with commit 58325f3 Jul 30, 2025
12 checks passed

nazar-pc pushed a commit to nazar-pc/abundance that referenced this pull request Jul 30, 2025

Backport autonomys/subspace#3654 (fix: now farmer piece cache sync pe…

cefb0b1

…rcentage base on total cache size)

tediou5 deleted the fix/switching-percent-total-cache-size branch August 1, 2025 16:53

fix: now farmer piece cache sync percentage base on total cache size #3654

fix: now farmer piece cache sync percentage base on total cache size #3654

Uh oh!

Conversation

tediou5 commented Jul 29, 2025

Code contributor checklist:

Uh oh!

immunefi-magnus bot commented Jul 29, 2025

🛡️ Immunefi PR Reviews

Uh oh!

tediou5 commented Jul 29, 2025

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

teor2345 Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

tediou5 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

teor2345 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

teor2345 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

tediou5 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

teor2345 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tediou5 left a comment

Choose a reason for hiding this comment

Uh oh!

tediou5 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tediou5 commented Jul 30, 2025

Uh oh!

vedhavyas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teor2345 commented Jul 31, 2025

Uh oh!

Uh oh!