Convert rvgo's memory implementation to radix trie #83

mininny · 2024-10-03T17:53:58Z

Description

Detailed design doc is at: https://www.notion.so/op-tip/Asterisc-Memory-Optimization-Design-Doc-Draft-b64a61f0c844403d99b59b120cc3222b?pvs=4

mininny · 2024-10-03T17:54:41Z

rvgo/fast/radix.go

+	// InvalidateNode invalidates the hash cache along the path to the specified address.
+	InvalidateNode(addr uint64)
+	// GenerateProof generates the Merkle proof for the given address.
+	GenerateProof(addr uint64) [][32]byte


from @protolambda :

You could add a slice as argument, to append the result to, and then return the updated slice header. Then you can allocate a slice with the capacity of the expected proof size once, and write to that in each proof level, to prevent allocations.

I modified the function so that GenerateProof takes a slice as argument, and each radix node fills in the proof slice at the correct index. The proof slice is only allocated once in the initial generateProof call (make([][32]byte, 60)) to reduce the allocations: c5a040b

codecov-commenter · 2024-10-03T18:04:38Z

Codecov Report

Attention: Patch coverage is 86.56716% with 36 lines in your changes missing coverage. Please review.

Project coverage is 73.17%. Comparing base (636210a) to head (f263b31).
Report is 20 commits behind head on master.

Files with missing lines	Patch %	Lines
rvgo/fast/radix.go	88.47%	15 Missing and 13 partials ⚠️
rvgo/fast/page.go	55.55%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   72.30%   73.17%   +0.86%     
==========================================
  Files          16       17       +1     
  Lines        2723     2923     +200     
==========================================
+ Hits         1969     2139     +170     
- Misses        670      689      +19     
- Partials       84       95      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pcw109550 · 2024-10-03T20:51:32Z

rvgo/fast/memory_test.go

+	largeDataset  = 1_000_000
+)
+
+func BenchmarkMemoryOperations(b *testing.B) {


Adding a new file for benchmarking would be good. Also, why these methods are implemented as test? Do they validate something?

I moved the benchmarking to a new file: memory_benchmarking_test.go.
I'm not sure if I understand why you mean by implemented as test. If you're talking about testing.B, it's because the benchmarking capabilities are offered under testing package. Also, there is no validation in the benchmarking test, we only measure the function calls themselves.

pcw109550 · 2024-10-03T20:57:05Z

rvgo/fast/memory_test.go

+	data := make([]byte, 1)
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		addr := mathrand.Uint64() % 1000000 // Confined to a smaller range


We can use mathrand.Intn

Nice, thanks!

pcw109550 · 2024-10-03T21:19:27Z

rvgo/fast/radix.go

+	if p, ok := m.pages[pageIndex]; ok {
+		return p.GenerateProof(addr) // Generate proof from the page.
+	} else {
+		return zeroHashRange(0, 8) // Return zero hashes if the page does not exist.


May I ask why is zeroHashRange(0, 8) ?

It's because the number of proof for a page is 8, where 0 is the leaf page data and 7 is the page's merkle root. I've also added this as a comment.

pcw109550 · 2024-10-03T21:21:50Z

rvgo/fast/radix.go

+		}
+	}
+
+	if depth > 16 {


what about the range between 8 < depth <= 16?

Sorry, the correct code should be depth > 9.
depth 0~7: the intermediate merkle hashes.
depth 8: the leaf nodes pointing to the children nodes
depth 9~: invalid gindex.

pcw109550 · 2024-10-03T21:27:54Z

rvgo/fast/radix.go

+	// Build the radix trie path to the new page, creating nodes as necessary.
+	radixLevel1 := m.radix
+	if (*radixLevel1).Children[branchPaths[0]] == nil {
+		node := &SmallRadixNode[L3]{Depth: 4}


Why are we starting at L3, not L2 or L1?

The L1 is the radix root, and it's type is SmallRadixNode[L2]. This [L2] points to the children node's type.
When we allocate the children of a L1, the L2's children type is SmallRadixNode[L3], where the [L3] is the L2's children type.
So, it seems like we're skipping L2, but the SmallRadixNode[L3] itself is the alias of L2.

pcw109550 · 2024-10-03T21:28:53Z

rvgo/fast/radix.go

+
+	radixLevel2 := (*radixLevel1).Children[branchPaths[0]]
+	if (*radixLevel2).Children[branchPaths[1]] == nil {
+		node := &SmallRadixNode[L4]{Depth: 8}


These Depth value can be evaluated using branchFactors: [10]uint64{4, 4, 4, 4, 4, 4, 4, 8, 8, 8},

pcw109550 · 2024-10-03T21:29:58Z

rvgo/fast/memory_test.go

-		require.Equal(t, r1, r2, "expecting manual page combination to match subtree merkle func")
-	})
+
+	//t.Run("random few pages", func(t *testing.T) {


Why are this test commented out?

I forgot to uncomment & fix the test after I've changed the implementation. I uncommented it and fixed the code :)

mininny added 10 commits September 13, 2024 16:51

Refactor hashmap memory representation to radix-trie based memory

b5c2e04

Clean up code

cf734a6

Add more tests to validate radix behavior

4774875

Reduce the number of cache to half

3eb03b6

Add simple benchmarking tests

4fdf03d

Use array of uint64 instead of boolean list for space efficiency

1dfb0a0

Refactor some tests

b47f409

Refactor radix implementation using generic struct

da17cb2

Improve rw performance

4b88e18

Add comments

89460e6

mininny commented Oct 3, 2024

View reviewed changes

mininny mentioned this pull request Oct 3, 2024

[WIP] Convert rvgo's memory implementation to radix trie testinprod-io/asterisc#6

Closed

pcw109550 reviewed Oct 3, 2024

View reviewed changes

mininny added 2 commits October 5, 2024 16:25

Move benchmarking test to separate file

f8f596b

Reduce slice memory allocation when generating a proof

f263b31

mininny force-pushed the feature/mininny/rvgo-radix-memory branch from c5a040b to f263b31 Compare October 5, 2024 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert rvgo's memory implementation to radix trie #83

Convert rvgo's memory implementation to radix trie #83

mininny commented Oct 3, 2024

mininny Oct 3, 2024

mininny Oct 5, 2024

codecov-commenter commented Oct 3, 2024 •

edited

Loading

pcw109550 Oct 3, 2024

mininny Oct 5, 2024

pcw109550 Oct 3, 2024

mininny Oct 5, 2024

pcw109550 Oct 3, 2024

mininny Oct 5, 2024

pcw109550 Oct 3, 2024

mininny Oct 5, 2024 •

edited

Loading

pcw109550 Oct 3, 2024

mininny Oct 5, 2024

pcw109550 Oct 3, 2024

pcw109550 Oct 3, 2024

mininny Oct 5, 2024

Convert rvgo's memory implementation to radix trie #83

Are you sure you want to change the base?

Convert rvgo's memory implementation to radix trie #83

Conversation

mininny commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2024 • edited Loading

Codecov Report

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

mininny Oct 5, 2024 • 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

codecov-commenter commented Oct 3, 2024 •

edited

Loading

mininny Oct 5, 2024 •

edited

Loading