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

feat: immutable data structure for memdb #4335

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

maan2003
Copy link
Member

@maan2003 maan2003 commented Feb 15, 2024

currently memdb clones all the database items on each begin_transaction, this is really slow for even slightly large database.

this uses immutable data structures for memdb, so clone in begin_transaction is O(1)

@@ -52,6 +52,7 @@ secp256k1-zkp = { version = "0.7.0", features = [ "use-serde", "bitcoin_hashes",
macro_rules_attribute = "0.1.3"
bitvec = "1.0.1"
parity-scale-codec = { version = "3.5.0", features = ["derive"] }
imbl = "2.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@maan2003 maan2003 changed the title [do not merge] immutable data structure for memdb [wip] immutable data structure for memdb Feb 15, 2024
@maan2003 maan2003 changed the title [wip] immutable data structure for memdb feat: immutable data structure for memdb Feb 15, 2024
@maan2003 maan2003 marked this pull request as ready for review February 15, 2024 11:59
@maan2003 maan2003 requested review from a team as code owners February 15, 2024 11:59
m1sterc001guy
m1sterc001guy previously approved these changes Feb 15, 2024
@github-merge-queue github-merge-queue bot dismissed m1sterc001guy’s stale review February 15, 2024 16:35

The merge-base changed after approval.

dpc
dpc previously approved these changes Feb 15, 2024
@maan2003 maan2003 dismissed dpc’s stale review February 15, 2024 16:35

The merge-base changed after approval.

@dpc dpc enabled auto-merge February 15, 2024 16:36
dpc
dpc previously approved these changes Feb 15, 2024
@maan2003 maan2003 dismissed dpc’s stale review February 15, 2024 16:51

The merge-base changed after approval.

@maan2003
Copy link
Member Author

rebased

Copy link
Contributor

@m1sterc001guy m1sterc001guy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another flaky test? Hard for me to tell if its due to these changes or not

@dpc dpc added this pull request to the merge queue Feb 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 19, 2024
@dpc dpc added this pull request to the merge queue Feb 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 20, 2024
@dpc
Copy link
Contributor

dpc commented Feb 20, 2024

00:04:44 ================= RESULTS ==================
00:04:44 REISSUE: min: 0.5s, avg: 0.7s, median: 0.6s, p90: 0.9s, max: 1.4s, sum: 19.9s
00:04:44 LN SEND: min: 1.3s, avg: 1.6s, median: 1.5s, p90: 2.2s, max: 2.3s, sum: 31.4s
00:04:44 LN RECV: min: 1.1s, avg: 1.6s, median: 1.6s, p90: 1.7s, max: 1.7s, sum: 31.9s
00:04:44 FM PAY: min: 1.4s, avg: 1.9s, median: 1.6s, p90: 1.7s, max: 6.7s, sum: 37.1s
00:04:44 RESTORE: 100.985712469s
00:04:44 thread 'main' panicked at /build/source/devimint/src/tests.rs:284:5:
00:04:44 assertion failed: fm_pay_stats.max.as_secs_f64() < fm_pay_stats.p90.as_secs_f64() * factor
00:04:44 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@maan2003 That assertion ... looks :sus: . Seems to me that you might have improved (along possibly with the executor improvements) common case so much, that is now much faster than a maximum one? Anyway, I'll leave it to you. :)

@maan2003
Copy link
Member Author

maan2003 commented Feb 20, 2024

this can't affect devimint latency tests, in devimint test we only test using rocksdb

likely just due to executor improvments

@maan2003 maan2003 added this pull request to the merge queue Feb 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 20, 2024
@dpc
Copy link
Contributor

dpc commented Feb 20, 2024

this can't affect devimint latency tests, in devimint test we only test using rocksdb

likely just due to executor improvments

I don't know. Failed again in mq, while I don't see failures like that on other PRs.

@elsirion elsirion added this pull request to the merge queue Feb 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 20, 2024
@maan2003
Copy link
Member Author

@dpc
Copy link
Contributor

dpc commented Feb 20, 2024

also failed for another PR
https://github.com/fedimint/fedimint/actions/runs/7977983266/job/21782122000#step:10:4064

Hmm. So what do we do? Change the factor? Seems like usual case got much faster, while max can sometimes take couple of seconds.

@maan2003 maan2003 added this pull request to the merge queue Feb 21, 2024
Merged via the queue into fedimint:master with commit a84f7ee Feb 21, 2024
19 of 20 checks passed
@maan2003 maan2003 deleted the imbl-memdb branch February 21, 2024 11:06
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