Batch transactions on persistent frontier writing #13557

georgeee · 2023-07-10T18:06:04Z

Problem: during testing on a private cluster, persistent frontier writes caused a series of consequent long async cycles of length 8s and 9 x 13s (total of > 100s). Long async cycles of more than 10s is a bad sign on its own, but even more so when combined in consequent groups.

Cycles are caused by a job in persistent frontier that dumps 10 blocks into RocksDB.

Explain your changes:

Submit changes to RocksDB in a single batch for all 10 blocks

Explain how you tested your changes:

Tested on a private cluster

Checklist:

Dependency versions are unchanged
- Notify Velocity team if dependencies must change in CI
Modified the current draft of release notes with details on what is completed or incomplete within this project
Document code purpose, how to use it
- Mention expected invariants, implicit constraints
Tests were added for the new behavior
- Document test purpose, significance of failures
- Test names should reflect their purpose
All tests pass (CI will check this if you didn't)
Serialized types are in stable-versioned modules
Does this close issues? List them

Closes #0000

nholland94 · 2023-07-10T20:58:11Z

src/lib/transition_frontier/persistent_frontier/database.ml

-    get t.db ~key:(Arcs parent_hash) ~error:(`Not_found (`Arcs parent_hash))
+    match State_hash.Table.find arcs_cache parent_hash with
+    | None ->
+        get t.db ~key:(Arcs parent_hash) ~error:(`Not_found (`Arcs parent_hash))


Batching these gets will further improve the performance of this by quite a lot. This could be accomplished by splitting the add function into 2 steps, or by implementing a simple monad here (or applicative functor, but ocaml has better tooling for monads).

I may try it, yep

nholland94 · 2023-07-10T21:00:02Z

src/lib/transition_frontier/persistent_frontier/worker.ml

+        let input =
+          List.filter_map input ~f:(function
+            | Diff.Lite.E.E (Best_tip_changed _) as diff ->
+                best_tip_cnt := !best_tip_cnt - 1 ;


NIT: you can write decr best_tip_cnt here (and below).

nholland94 · 2023-07-10T21:05:09Z

src/lib/transition_frontier/persistent_frontier/worker.ml

+        let root_cnt = ref root_cnt in
+        let garbage_prev = ref [] in
+        let input =
+          List.filter_map input ~f:(function


I find this code a bit tricky to reason about, even though I think it's correct. I'd probably prefer this to be written somewhat as follows thought:

partion diffs by type (best tip changed, root transitioned, others)

perform the diff optimizations by folding over the all elements of best tip changed except for the last (do the same with root transitioned)

I think the code will be easier to reason about if implemented this way, instead of having to count the diff types and using mutable logic in the loop to reason about which element is the last in the sequence of those types.

@nholland94 I implemented it with counting because I didn't want this code to rely on that last two items should be best tip change and root transitioned. And if I partition by type, how could I reconstruct the order later?

When you partition by type, each partition will be in the same order the elements occurred in the initial list. The counting allows you to find the last best tip change and last root transition within the full list of diffs. If they list of diffs are partitioned, then the last element of the best tip change list and the last element of the root transition list will be the same diffs you are selecting via this counting mechanism (if I am not mistaken).

So, could you check my last commit? I rewrote it without counters, not sure if in the exact way you had in mind.

Not quite. Let me take a stab at what I was describing and see if I can come up with something a bit clearer. Though if it ends up being more of a hassle than I think it will be, we can just defer refactoring this until later.

deepthiskumar · 2023-07-18T17:38:16Z

!ci-build-me

georgeee · 2023-07-26T13:50:58Z

!ci-build-me

georgeee · 2023-07-26T18:21:22Z

!ci-build-me

nholland94 · 2023-09-13T17:16:10Z

!ci-build-me

georgeee

Fine by me this way. I guess I was stuck in "don't change the order" mentality in that I didn't want last best_tip_diff to occur after last root_transition if it was vice versa originally. But given we update independent parts of DB, it should be totally fine.

georgeee · 2023-09-13T20:36:10Z

!ci-build-me

georgeee · 2023-09-13T22:01:45Z

!ci-build-me

nholland94 · 2023-09-14T01:00:57Z

!approved-for-mainnet

georgeee · 2023-09-14T09:36:51Z

!ci-build-me

georgeee · 2023-09-15T10:56:39Z

!ci-build-me

georgeee · 2023-09-16T23:39:52Z

Evidently, there is a bug in the PR caught by the medium bootstrap integration test
Need to perform some investigation

georgeee · 2023-09-19T18:57:18Z

src/lib/transition_frontier/persistent_frontier/worker.ml

+        let extra_garbage =
+          List.drop_last root_transition_diffs
+          |> Option.value ~default:[]
+          |> List.bind ~f:(fun { new_root; garbage = Lite garbage; _ } ->


Missing just_emitted log for squashed root transitions

georgeee · 2023-09-19T19:02:51Z

src/lib/transition_frontier/persistent_frontier/worker.ml

+              ~metadata:[ ("parent", `String (State_hash.to_base58_check h)) ] ;
+            Deferred.unit
+        | Error (`Not_found `Old_root_transition) ->
+            failwith


Error is copypaste and not specific

georgeee · 2023-09-19T21:19:26Z

!ci-build-me

1. Simplify code of batch writes 2. Use batch get

georgeee · 2023-09-20T21:25:42Z

!ci-build-me

georgeee · 2023-09-20T21:25:47Z

!ci-nightly-me

georgeee · 2023-09-20T21:28:28Z

!ci-nightly-me

deepthiskumar · 2023-09-21T08:20:08Z

!ci-nightly-me

georgeee · 2023-09-21T09:54:08Z

Confirmed that medium bootstrap test is passing now: https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/508#018ab6fa-6a16-4813-926b-f4d068423d35

deepthiskumar · 2023-09-21T12:03:52Z

!approved-for-mainnet

georgeee requested a review from a team as a code owner July 10, 2023 18:06

nholland94 approved these changes Jul 10, 2023

View reviewed changes

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch 2 times, most recently from c896953 to 794aa92 Compare July 12, 2023 15:29

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from cae8f84 to 0f5ce19 Compare July 26, 2023 13:50

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch 2 times, most recently from 0f5ce19 to eaf52ed Compare July 26, 2023 18:17

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from eaf52ed to 1240e03 Compare August 25, 2023 17:29

georgeee self-assigned this Aug 28, 2023

georgeee commented Sep 13, 2023

View reviewed changes

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from 71d875b to 2beb1a0 Compare September 13, 2023 20:35

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from 2beb1a0 to ecb005f Compare September 13, 2023 22:01

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from ecb005f to b90f98e Compare September 14, 2023 09:36

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from b90f98e to c2b827d Compare September 15, 2023 10:56

georgeee commented Sep 19, 2023

View reviewed changes

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from 66023c7 to 4e04a54 Compare September 19, 2023 21:19

georgeee mentioned this pull request Sep 20, 2023

Persistent frontier load is never working #13949

Closed

2 tasks

Batch transactions on persistent frontier writing

ace37e2

georgeee and others added 4 commits September 20, 2023 23:24

Address review requests

f6445e6

1. Simplify code of batch writes 2. Use batch get

Refactor persistent frontier worker diff optimizations

a643fbe

Fix a bug with snarked ledgers not updated

2ef6534

Fix a bug with state hashes being unnecessary queried in DB

354ab60

georgeee force-pushed the georgeee/batch-persistent-frontier-writing branch from 86225f6 to 354ab60 Compare September 20, 2023 21:24

deepthiskumar merged commit aaa9d6f into berkeley Sep 21, 2023
99 of 101 checks passed

deepthiskumar deleted the georgeee/batch-persistent-frontier-writing branch September 21, 2023 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch transactions on persistent frontier writing #13557

Batch transactions on persistent frontier writing #13557

georgeee commented Jul 10, 2023 •

edited

Loading

nholland94 Jul 10, 2023

georgeee Jul 10, 2023

nholland94 Jul 10, 2023

nholland94 Jul 10, 2023

georgeee Jul 10, 2023

nholland94 Jul 12, 2023

georgeee Jul 12, 2023

nholland94 Sep 12, 2023

deepthiskumar commented Jul 18, 2023

georgeee commented Jul 26, 2023

georgeee commented Jul 26, 2023

nholland94 commented Sep 13, 2023

georgeee left a comment

georgeee commented Sep 13, 2023

georgeee commented Sep 13, 2023

nholland94 commented Sep 14, 2023

georgeee commented Sep 14, 2023

georgeee commented Sep 15, 2023

georgeee commented Sep 16, 2023

georgeee Sep 19, 2023

georgeee Sep 19, 2023

georgeee commented Sep 19, 2023

georgeee commented Sep 20, 2023

georgeee commented Sep 20, 2023

georgeee commented Sep 20, 2023

deepthiskumar commented Sep 21, 2023

georgeee commented Sep 21, 2023

deepthiskumar commented Sep 21, 2023

Batch transactions on persistent frontier writing #13557

Batch transactions on persistent frontier writing #13557

Conversation

georgeee commented Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthiskumar commented Jul 18, 2023

georgeee commented Jul 26, 2023

georgeee commented Jul 26, 2023

nholland94 commented Sep 13, 2023

georgeee left a comment

Choose a reason for hiding this comment

georgeee commented Sep 13, 2023

georgeee commented Sep 13, 2023

nholland94 commented Sep 14, 2023

georgeee commented Sep 14, 2023

georgeee commented Sep 15, 2023

georgeee commented Sep 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee commented Sep 19, 2023

georgeee commented Sep 20, 2023

georgeee commented Sep 20, 2023

georgeee commented Sep 20, 2023

deepthiskumar commented Sep 21, 2023

georgeee commented Sep 21, 2023

deepthiskumar commented Sep 21, 2023

georgeee commented Jul 10, 2023 •

edited

Loading