Skip to content

perf: gc might try to populate the lookahead buffer each time #1110

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ryan-CW-Code
Copy link

Sorry my English is very poor, so I use machine translation.
When the user incorrectly sets lookahead_size, if lookahead_size > (block_count / 8), gc might try to populate the lookahead buffer each time.
I'm not sure if this PR is the optimal solution. Perhaps it should help users set the correct value during the mount stage.
Because similar processing is also done in the lfs_format_ and lfs_alloc_scan functions, in fact, this processing can be completely optimized out.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17112 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17112 B (+0.0%) 1448 B (+0.0%) 812 B (+0.0%) Lines 2434/2595 lines (+0.0%)
Readonly 6230 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1283/1616 branches (+0.0%)
Threadsafe 17964 B (+0.0%) 1448 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17184 B (+0.0%) 1448 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18776 B (+0.0%) 1752 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17924 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented Jun 1, 2025

Interesting, it looks like you're right, this is needed to avoid redundant lookahead scans when 8*lookahead_size > block_count. Can bring this in (next patch?) and thanks for the PR!

I'm not sure if this PR is the optimal solution. Perhaps it should help users set the correct value during the mount stage.
Because similar processing is also done in the lfs_format_ and lfs_alloc_scan functions, in fact, this processing can be completely optimized out.

This gets tricky when block_count is not a multiple of 8. Unlikely, for large filesystems, but useful for carving out blocks for partition tables, reserved storage, XiP, etc.

But you're right this doesn't feel very optimal.

Eventually (the "Config API rework" in #1111), I would like to drop the idea that struct lfs_config needs to be statically allocated. If instead we store everything in lfs_t in RAM, we can apply lfs_min during the mount stage and simplify this logic. By default this would add RAM, but in theory in the "Config API rework" that RAM could be saved by defining LFS_CFG_LOOKAHEAD_SIZE at compile time.

@geky geky added next patch needs work nothing broken but not ready yet labels Jun 1, 2025
@geky-bot
Copy link
Collaborator

geky-bot commented Jun 3, 2025

Tests passed ✓, Code: 17112 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17112 B (+0.0%) 1448 B (+0.0%) 812 B (+0.0%) Lines 2435/2596 lines (+0.0%)
Readonly 6230 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1283/1616 branches (+0.0%)
Threadsafe 17964 B (+0.0%) 1448 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17184 B (+0.0%) 1448 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18776 B (+0.0%) 1752 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17924 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented Jun 3, 2025

Looks great, thanks for this. Will bring this in on the next patch release.

@geky geky removed the needs work nothing broken but not ready yet label Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants