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

Consider removing transaction_cache in Sumeragi #3067

Open
Erigara opened this issue Jan 19, 2023 · 2 comments
Open

Consider removing transaction_cache in Sumeragi #3067

Erigara opened this issue Jan 19, 2023 · 2 comments
Labels
Chore This is a small task that can be done at any point in time and is easier than others good first issue Good for newcomers iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested Refactor Improvement to overall code quality

Comments

@Erigara
Copy link
Contributor

Erigara commented Jan 19, 2023

          @Erigara says: `i’m not sure that this will work out because we still need cloning because block contain all transactions`

this is a good point, but we have transaction_cache containing duplicates of transactions. It might still be worth to consider using Arc for that reason. On the other hand, I can hardly believe that transaction_cache in sumeragi is making any difference - especially after these changes are merged. We should remove it if possbile

Originally posted by @mversic in #3043 (comment)

@Erigara Erigara added the Refactor Improvement to overall code quality label Jan 19, 2023
@mversic mversic added the question Further information is requested label Jan 19, 2023
@mversic mversic added good first issue Good for newcomers Chore This is a small task that can be done at any point in time and is easier than others labels Jan 27, 2023
@Erigara Erigara self-assigned this Feb 20, 2023
@Erigara Erigara added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jun 5, 2023
@Erigara Erigara removed their assignment Jun 5, 2023
@appetrosyan
Copy link
Contributor

Cow?

I don't mind the transaction cache, it actually saved us in a couple of situations. Trying to remove it might reveal more undesirable behaviours, so if we can explore this, it'd be nice.

However, the implementation should utilise the redundancy; most transactions are kept in-RAM with no modifications. An Arc is on the right track, but it would be wasting cycles on a non-zero refcount. I personally think that Weak would do just as well if not better.

@appetrosyan appetrosyan pinned this issue Jul 25, 2023
@SamHSmith
Copy link
Contributor

transaction_cache came into existance as a way to not call into queue.rs because it was impressively slow. So if the queue was made actually good it might be fine. Queue needs to be designed keeping in mind that it will get frequently called by sumeragi requesting the same information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore This is a small task that can be done at any point in time and is easier than others good first issue Good for newcomers iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested Refactor Improvement to overall code quality
Projects
None yet
Development

No branches or pull requests

5 participants