Details

Reviewers

Fabien
bytesofman

Group Reviewers

Restricted Project

Commits

rABCbb6a7ba5dda1: [Chronik] Optimize SpentByWriter using merge ops

Summary

Speed up SpendByWriter significantly using merge ops: When chronikreindexing blocks 22000 to 221000, SpendByWriter::insert goes from 4.38s to 0.19s. When chronikreindexing the first 300000 blocks, it seems to shave off roughly 1700s, from 10059s to 8315.98s.

Depends on D14505.

Test Plan

ninja check-crates && ninja check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tobias_ruck created this revision.Aug 29 2023, 22:41

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 29 2023, 22:41

Harbormaster completed remote builds in B24892: Diff 41996.Aug 29 2023, 23:00

Fix catch_merge_error

Add tests to test_catch_merge

Harbormaster completed remote builds in B24900: Diff 42008.Aug 30 2023, 12:19

tobias_ruck edited the summary of this revision. (Show Details)Aug 30 2023, 12:28

Harbormaster completed remote builds in B24901: Diff 42009.Aug 30 2023, 12:32

Rebase from master

Harbormaster completed remote builds in B24903: Diff 42012.Aug 30 2023, 13:03

tobias_ruck published this revision for review.Aug 30 2023, 13:09

Fix comment in apply_merge_spent_by

Harbormaster completed remote builds in B24905: Diff 42015.Aug 30 2023, 13:50

improvement looks great. code appears tested and effective.

tbh I really don't know what's going on here. Could you expand a bit on the diff summary?

using merge ops

What does this mean / what was being done before?

chronik/chronik-db/src/io/group_history.rs
169 ↗	(On Diff #42015)	what does `ser` mean here
chronik/chronik-db/src/io/spent_by.rs
558–572 ↗	(On Diff #42015)	what is this doing?

Please split the fix out of the speedup, they can be landed separately

In D14430#323854, @bytesofman wrote:

improvement looks great. code appears tested and effective.

tbh I really don't know what's going on here. Could you expand a bit on the diff summary?

using merge ops

What does this mean / what was being done before?

See D14393 for some history

chronik/chronik-db/src/io/group_history.rs
169 ↗	(On Diff #42015)	I think it's half of serdes
chronik/chronik-db/src/io/spent_by.rs
558–572 ↗	(On Diff #42015)	My understanding is that it's checking that the block disconnects doesn't impact the utxos that were not spent in that block. Previously the test was only checking for the state from the block txs, and not checking the side effects on other utxos.