Skip to content
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

Replace lru with schnellru #2598

Closed
nazar-pc opened this issue Mar 11, 2024 · 4 comments · Fixed by #2736
Closed

Replace lru with schnellru #2598

nazar-pc opened this issue Mar 11, 2024 · 4 comments · Fixed by #2736
Assignees
Labels

Comments

@nazar-pc
Copy link
Member

Similar to paritytech/substrate#13802

@nazar-pc nazar-pc added good first issue Good for newcomers refactor labels Mar 11, 2024
@anujmax
Copy link

anujmax commented Apr 28, 2024

hi @nazar-pc , is it fine if I work on this?

@nazar-pc
Copy link
Member Author

Sure, please

@anujmax
Copy link

anujmax commented May 2, 2024

hi @nazar-pc , I am done with most of the changes, I have a question:

block_authoring_delay is u64,
https://github.com/subspace/subspace/blob/main/crates/sc-consensus-subspace-rpc/src/lib.rs#L278-L291

and schnellru support u32 for LimitedLength capacity.
https://github.com/koute/schnellru/blob/5a8927e9ed4b11fdfb0f2115aa4d2577e30d6f05/src/limiters.rs#L7

Should I cap the capacity of solution_response_senders cache to u32 max ? or do you recommed something else?

@nazar-pc
Copy link
Member Author

nazar-pc commented May 2, 2024

Should I cap the capacity of solution_response_senders cache to u32 max ? or do you recommed something else?

If you look carefully, you'll see that the only reason block_authoring_delay is converted into u64 is because it is originally Slot type that can be converted into u64 easily. Moreover, it is a very small number in practice (4) so you will never ever need to worry about its size, simply do this:

        let solution_response_senders_capacity =
            u32::try_from(block_authoring_delay).expect("Always a tiny constant in the protocol; qed");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

2 participants