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

Blockchain rwlock. #4117

Closed
wants to merge 7 commits into from
Closed

Conversation

Frozen
Copy link
Contributor

@Frozen Frozen commented Apr 4, 2022

Issue

This PR brings RwLocks to blockchain methods.
As i know, Leveldb reads and writes are atomic, and creating additional lock will beat performance. Especially while write lock, no read operations will performe, but on the other hand, no other disc operations at the same time, that will increase block insertion time.

Disadvantages

  1. This will beat performance for read operations, maybe dramatically

Advantages

  1. Simple and clear lock mechanics. More important, it is also safe. It also does not impose any restrictions on the use, you can do multiple non-atomic operations under one lock.
  2. Probably better block insertion time (on loaded server).
  3. This will allow to insert multiple blocks at the same time.
  4. Write lock time could be reduced in future.

Possible problems

Imagine we have 2 methods get and cache that atomic by itself but not together. In 2 different goroutines it could be:
Снимок экрана 2022-05-10 в 22 22 34

or
image

Second case has 2 problems:

  • We can lose all data proceeded by second thread, it would be overwritten by first cache.
  • Possible do duplicate work.

IRL example

for example we proceed getting crosslinks with ReadCrosslinks, validating them and putting next with AddPendingCrosslinks. Even we don't lose crosslinks here, data can be duplicated. see node/node_cross_link.go:59

Test

Unit Test Coverage

Before:

<!-- copy/paste 'go test -cover' result in the directory you made change -->

After:

<!-- copy/paste 'go test -cover' result in the directory you made change -->

Test/Run Logs

Operational Checklist

  1. Does this PR introduce backward-incompatible changes to the on-disk data structure and/or the over-the-wire protocol?. (If no, skip to question 8.)

    NO

  2. Does this PR introduce backward-incompatible changes NOT related to on-disk data structure and/or over-the-wire protocol? (If no, continue to question 11.)

    NO

  3. Does this PR introduce significant changes to the operational requirements of the node software, such as >20% increase in CPU, memory, and/or disk usage?

MAYBE, have to check

TODO

@rlan35
Copy link
Contributor

rlan35 commented Apr 7, 2022

Please add performance comparison to show that this PR makes improvement on the block sync.

@JackyWYX
Copy link
Contributor

One disadvantage of this method is that if the node is super busy inserting the blocks (E.g. mainnet beacon blocks), all read access will be blocked according to the RWLock, which means that the node will be unavailable for most of the time.

Before the change, the blockchain interface had different locks over different fields, which shall be a little more optimized, right? Any benchmarks so far?

@Frozen Frozen closed this Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants