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

Client: LMDB Database Test (new) #3129

Closed
wants to merge 2 commits into from
Closed

Client: LMDB Database Test (new) #3129

wants to merge 2 commits into from

Conversation

holgerd77
Copy link
Member

I experimented a bit more with the LMDB integration, since I think we shouldn't give up too quickly on this, since potential client-wide gains would be just too significant if we could integrate.

Even if by no means clean I will push my work based on #2986 here since this is a bit reduced with no blockchain integration e.g. and therefore has less code changes and it gets somewhat easier to play with and adopt.

So @scorbajio mentioned in his integration PR that the client would start but it would take several (!) minutes until execution/sync starts at all. We should definitely have a closer look here and try to fix this, since this has some large likelyhood to point us to what we are still doing structurally wrong. This is by no means "normal" LMDB behavior.

The one thing I noticed from the LMDB docs is that LMDB get is synchronous. This was definitely not yet considered in the previous PR, so I think it makes a lot of sense to take this as a starting point.

In the current overhaul I've made the very last DB call sync by interface addition (so this would be backwards compatible), this doesn't bring the change yet though.

So I think we should make the (somewhat harder) test and see what happens if the whole call stack (so from StateManager -> Trie -> DB) is made synchronous. A bit hairy to admint. But we might be able to at lest test this in some not too dramatic way by just copy-paste code over and name sync methods similarly what I've done for the interface.

So that might already be it. If LMDB is made for synchronous gets and this is called in an async manner, might be that things block each other and this is eventually the reason client is not properly starting.

As a reminder why we are doing this:

grafik

These are promises/results from a benchmark run. Take with a grain of salt (from the LMDB website/GitHub), but other benchmarks do point to larger gains too.

So at least worth to go one level deeper and test this to the level that we are sure about our results.

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #3129 (e3fc2b3) into master (4bd9438) will decrease coverage by 0.82%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.80% <ø> (ø)
blockchain 92.27% <ø> (ø)
client ?
common 98.19% <ø> (ø)
ethash ∅ <ø> (∅)
evm 71.93% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 90.30% <ø> (ø)
trie 89.76% <ø> (+0.04%) ⬆️
tx 96.36% <ø> (ø)
util 87.58% <100.00%> (+0.01%) ⬆️
vm 76.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

Will close the LMDB test PRs for now. This is open for quite some time and did not lead to anything usable up till now. I additionally stumbled upon some more detailed benchmarks (cannot re-find the reference unfortunately) stating that LMDB will not bring performance gains for larget datasets.

RocksDB (Facebook) might still be an option to explore. We'll see.

@holgerd77 holgerd77 closed this Feb 14, 2024
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.

1 participant