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

Don't lock EpochManager across the system #2745

Closed
ilblackdragon opened this issue May 29, 2020 · 9 comments
Closed

Don't lock EpochManager across the system #2745

ilblackdragon opened this issue May 29, 2020 · 9 comments
Assignees
Labels
A-chain Area: Chain, client & related C-enhancement Category: An issue proposing an enhancement or a PR with one. T-core Team: issues relevant to the core team

Comments

@ilblackdragon
Copy link
Member

ilblackdragon commented May 29, 2020

Currently we are locking EpochManager across the client and view client. We are locking it because it's maintained under RuntimeAdapter which is shared between Client and ViewClient.

There is alternative (a) that @SkidanovAlex suggested:

  • Remove EpochManager from RuntimeAdapter and provide clones of it directly to Client and ViewClient. This will mean caches will be separate and wouldn't require locks.

We had reasoning why we didn't want to move EpochManager from RuntimeAdapter, and it was because we at some point wanted to move it's logic inside the Runtime as a smart contract.

Another alternative, is that redesign EpochManager in a way that Chain maintains it's own information that it needs. This would be in a way similar to previous suggestion, where Chain / Client have a data structure of the fields they need to maintain current validators public keys & related info. EpochManager moves into Runtime, and becomes only collecting proposals and runtime just returns after some state transition new set of validating public keys (and related info). This would also be closer to what this was initially planned.

@bowenwang1996
Copy link
Collaborator

bowenwang1996 commented May 29, 2020

@ilblackdragon I assume this is needed for phase 1. Let's not redesign EpochManager and just apply the bandaid solution that Alex suggested.

@bowenwang1996 bowenwang1996 added the A-chain Area: Chain, client & related label May 29, 2020
@ilblackdragon
Copy link
Member Author

I think before we do anything like this - I want to have a benchmark we can run that clearly shows performance and bottlenecks. Right now we have only hypotheses. Given we only just right now noticing that everything was running on single core.

@bowenwang1996
Copy link
Collaborator

@ilblackdragon agree

@damons
Copy link

damons commented Jun 4, 2020

Moving to Phase 2 per Phase 1 triage.

@stale
Copy link

stale bot commented Jul 1, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Jul 1, 2021
@bowenwang1996 bowenwang1996 added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-core Team: issues relevant to the core team and removed S-stale labels Jul 2, 2021
@stale
Copy link

stale bot commented Sep 30, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale
Copy link

stale bot commented Dec 29, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Dec 29, 2021
@stale
Copy link

stale bot commented Mar 31, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Mar 31, 2022
@matklad
Copy link
Contributor

matklad commented May 30, 2022

was closed by #6549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-chain Area: Chain, client & related C-enhancement Category: An issue proposing an enhancement or a PR with one. T-core Team: issues relevant to the core team
Projects
None yet
Development

No branches or pull requests

5 participants