feat(blockstore): benchmarks #275

dadepo · 2024-09-18T06:24:52Z

Replicate mostly benches from

https://github.com/anza-xyz/agave/blob/master/ledger/benches/blockstore.rs
and
https://github.com/anza-xyz/agave/blob/master/ledger/benches/protobuf.rs

dadepo · 2024-09-30T11:24:48Z

src/ledger/tests.zig

+    pub const max_iterations = 5;
+
+    // Analogous to [bench_write_small](https://github.com/anza-xyz/agave/blob/cfd393654f84c36a3c49f15dbe25e16a0269008d/ledger/benches/blockstore.rs#L59)
+    pub fn benchWriteSmall() !u64 {


Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchWriteSmall 5 757783 873809 2151500994 795654

Agave at 9c2098450ca7e5271e3690277992fbc910be27d0

running 1 test test bench_write_small ... bench: 23,708,904.20 ns/iter (+/- 222,018.81)

Are your benchmarks from release or debug builds of sig?

Here's what I'm seeing:

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchWriteSmall 5 49872 66281 29995977 57241

test bench_write_small ... bench: 17,406,472.70 ns/iter (+/- 8,623,383.67)

It was indeed a debug build.

Having a ReleaseSmall build I also get similar numbers as you got.

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchWriteSmall 5 30669 41452 13606829 34371

dadepo · 2024-09-30T11:32:48Z

src/ledger/tests.zig

+    }
+
+    // Analogous to [bench_read_sequential]https://github.com/anza-xyz/agave/blob/cfd393654f84c36a3c49f15dbe25e16a0269008d/ledger/benches/blockstore.rs#L78
+    pub fn benchReadSequential() !u64 {


NOTE: debug build

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadSequential 5 133011 156933 102519429 143209

Agave at 9c2098450ca7e5271e3690277992fbc910be27d0

running 1 test test bench_read_sequential ... bench: 2,740,116.70 ns/iter (+/- 304,996.16)

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadSequential 5 6110 8331 603665 7538

test bench_read_sequential ... bench: 2,403,107.92 ns/iter (+/- 584,953.52)

ReleaseSmall

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadSequential 5 3484 3816 12416 3680

dadepo · 2024-09-30T11:37:34Z

src/ledger/tests.zig

+    }
+
+    // Analogous to [bench_read_random]https://github.com/anza-xyz/agave/blob/92eca1192b055d896558a78759d4e79ab4721ff1/ledger/benches/blockstore.rs#L103
+    pub fn benchReadRandom() !u64 {


Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadRandom 5 1993658 2326961 12944783078 2120888

Agave at 9c2098450ca7e5271e3690277992fbc910be27d0

running 1 test test bench_read_random ... bench: 2,820,841.70 ns/iter (+/- 28,719.65)

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadRandom 5 120573 132475 24537031 126483

test bench_read_random ... bench: 2,000,161.67 ns/iter (+/- 541,693.84)

The random performance is a lot worse than agave. The other benchmarks were much closer. It's strange, since the majority of what's being benchmarked here in both cases is rocksdb itself. I would have expected similar performance. Maybe the database needs to be tuned for random access.

I noticed that the agave benchmark reads 4369 shreds, whereas the sig benchmark reads 10499 indices. But that wouldn't explain a 50x slowdown. I also saw that in the agave benchmark, they only read 1/15 of the total number of shreds. Whereas in the sig benchmark, we read all of the shreds. But 4369 is not 1/15 of 10499, so something isn't adding up here.

It looks like benchReadSequential and benchReadRandom use the same input shreds in agave.blockstore.bench_read.shreds.bin. Is that intentional? Maybe each benchmark should use shreds that were generated by the respective agave benchmark, to ensure the data is comparable.

With releasemall, I also got

benchReadRandom 5 53996 72074 38589915 60359

it looks like benchReadSequential and benchReadRandom use the same input shreds in agave.blockstore.bench_read.shreds.bin. Is that intentional? Maybe each benchmark should use shreds that were generated by the respective agave benchmark, to ensure the data is comparable.

I'll look into this.

dadepo · 2024-09-30T11:38:50Z

src/ledger/tests.zig

+    }
+
+    // Analogous to [bench_serialize_write_bincode](https://github.com/anza-xyz/agave/blob/9c2098450ca7e5271e3690277992fbc910be27d0/ledger/benches/protobuf.rs#L88)
+    pub fn benchSerializeWriteBincode() !u64 {


I am not sure if this is 100% identical tobench_serialize_write_bincode in agave has Agave has put_bytes that first serialized the values before insertion. Sig currently does not have similar method.

In this case, the agave benchmark includes both serialization and insertion. So I think it is actually the same

Side note, regarding put_bytes not existing in sig. I tried to abstract away the bytes. Converting between types and bytes is the whole purpose of the Database layer. Ideally the ledger code can just use a Database like it's a hashmap, without worrying about serialization. It feels like a leaky abstraction if the Database layer can be bypassed, exposing its internal data to the business logic of the ledger.

That said, actually we do have a getBytes method, so I didn't do a great job at my goal haha. This is because shreds themselves are already bytes. The only reason getBytes exists is to provide a mechanism for managing the lifetime of the shred bytes without needing to copy them into a new allocation. I think this could actually be handled better using the type system, and we could probably eliminate getBytes completely. I decided was willing to compromise on the abstraction a bit, to get the job done quickly.

dnut · 2024-10-02T13:32:17Z

src/ledger/tests.zig

@@ -425,3 +487,130 @@ pub fn TestDB(scope: []const u8) type {
        }
    };
 }
+
+pub const BenchmarLegger = struct {


Would you be opposed to moving this into another file called benchmarks.zig?

Suggested change

pub const BenchmarLegger = struct {

pub const BenchmarkLedger = struct {

Nope. Won't be opposed. Kept it in the same test file as I noticed a couple of benchmarks were like that...

Done 5fa8282/ab2086b/077cda2

dnut · 2024-10-02T18:11:52Z

src/ledger/tests.zig

@@ -3,6 +3,7 @@
 const std = @import("std");
 const sig = @import("../sig.zig");
 const ledger = @import("lib.zig");
+const transaction_status = @import("./transaction_status.zig");


This can be used as ledger.transaction_status and does not require the file to be imported.

Currently our convention is that you only import root source files from the current module. For example you can import sig.zig if you're in the sig project, and you can import lib.zig from the current folder. But all other imports go through the decls defined by those imports.

This is definitely up for discussion. If you'd like to adopt a different convention, I'm happy to discuss it. But if you have no preference, let's import this transaction_status the ledger decl.

I think Importing via the root source file is indeed better. Updated in 692d561

dnut · 2024-10-02T18:16:23Z

src/ledger/tests.zig

@@ -336,6 +360,22 @@ pub fn loadEntriesFromFile(allocator: Allocator, path: []const u8) ![]const Entr
    return entries.toOwnedSlice();
 }

+fn create_rewards(allocator: std.mem.Allocator, count: usize) !Rewards {


I would like to switch to snake case for functions, but for now, the convention in Sig is to use camel case.

Old habits (from Rust) die hard. Updated in 44fa660

dnut · 2024-10-02T18:26:05Z

src/ledger/tests.zig

+        /// Differs from init in that it uses the std.heap.c_allocator instead
+        /// of the test allocator.
+        pub fn initBench(comptime test_name: []const u8) !*Self {


This actually uses the GPA allocator, just like the normal init function. See that it's hardcoded above with const _allocator = gpa.allocator(). The leak check only exists for the sole purpose of verifying that the deinit function has been called, by taking advantage of the automatic memory leak checks that occur in tests.

My assumption is that you're trying to accomplish two goals here:

use the c allocator for better performance

avoid using the testing allocator because it only works in tests

For reason 1, you'll need to change how TestState works to make it so it can be initialized with an arbitrary allocator. It'll be complicated to make this work with the way the gpa is currently used. Instead, it would probably be easiest to just get rid of the gpa and change init to accept an allocator. For tests, pass in the testing allocator instead of the gpa. This will reduce the amount of debugging info during errors, but that doesn't really matter. Then, you could also remove the leak check since it's no longer needed.

For reason 2, it would already be mitigated if you go with the solution I just mentioned for reason 1. If you don't use that approach, and the leak check is kept around, you might just want to make the leak check optional, and set it to null outside of tests.

If you want some more context on why the gpa and leak check are included here, I'm happy to hop on a huddle to explain it. But you can also feel free to remove it, because it was probably not a great idea for me to put it here in the first place.

Removed in a separate PR #305 to keep the PRs contained.

dnut · 2024-10-02T18:30:01Z

src/ledger/tests.zig

@@ -407,6 +461,14 @@ pub fn TestState(scope: []const u8) type {
            _allocator.destroy(self);
            _ = gpa.detectLeaks();
        }
+
+        pub fn deinitBench(self: *Self) void {


This can probably be removed (see my other comment)

Removed in dbcd012

dnut · 2024-10-02T18:51:53Z

src/ledger/tests.zig

+        defer inline for (.{shreds}) |slice| {
+            deinitShreds(allocator, slice);
+        };


Why is this a loop?

dnut · 2024-10-02T20:46:18Z

src/ledger/tests.zig

+    pub const max_iterations = 5;
+
+    // Analogous to [bench_write_small](https://github.com/anza-xyz/agave/blob/cfd393654f84c36a3c49f15dbe25e16a0269008d/ledger/benches/blockstore.rs#L59)
+    pub fn benchWriteSmall() !u64 {


Are your benchmarks from release or debug builds of sig?

Here's what I'm seeing:

Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchWriteSmall 5 49872 66281 29995977 57241

test bench_write_small ... bench: 17,406,472.70 ns/iter (+/- 8,623,383.67)

dnut · 2024-10-02T20:46:37Z

src/ledger/tests.zig

+    }
+
+    // Analogous to [bench_read_sequential]https://github.com/anza-xyz/agave/blob/cfd393654f84c36a3c49f15dbe25e16a0269008d/ledger/benches/blockstore.rs#L78
+    pub fn benchReadSequential() !u64 {


Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadSequential 5 6110 8331 603665 7538

test bench_read_sequential ... bench: 2,403,107.92 ns/iter (+/- 584,953.52)

dnut · 2024-10-02T20:47:39Z

src/ledger/tests.zig

+    }
+
+    // Analogous to [bench_read_random]https://github.com/anza-xyz/agave/blob/92eca1192b055d896558a78759d4e79ab4721ff1/ledger/benches/blockstore.rs#L103
+    pub fn benchReadRandom() !u64 {


Benchmark Iterations Min(us) Max(us) Variance Mean(us) --------------------------------------------------------------------------------- benchReadRandom 5 120573 132475 24537031 126483

test bench_read_random ... bench: 2,000,161.67 ns/iter (+/- 541,693.84)

The random performance is a lot worse than agave. The other benchmarks were much closer. It's strange, since the majority of what's being benchmarked here in both cases is rocksdb itself. I would have expected similar performance. Maybe the database needs to be tuned for random access.

I noticed that the agave benchmark reads 4369 shreds, whereas the sig benchmark reads 10499 indices. But that wouldn't explain a 50x slowdown. I also saw that in the agave benchmark, they only read 1/15 of the total number of shreds. Whereas in the sig benchmark, we read all of the shreds. But 4369 is not 1/15 of 10499, so something isn't adding up here.

It looks like benchReadSequential and benchReadRandom use the same input shreds in agave.blockstore.bench_read.shreds.bin. Is that intentional? Maybe each benchmark should use shreds that were generated by the respective agave benchmark, to ensure the data is comparable.

dnut · 2024-10-02T21:39:59Z

src/ledger/tests.zig

+    }
+
+    // Analogous to [bench_serialize_write_bincode](https://github.com/anza-xyz/agave/blob/9c2098450ca7e5271e3690277992fbc910be27d0/ledger/benches/protobuf.rs#L88)
+    pub fn benchSerializeWriteBincode() !u64 {


In this case, the agave benchmark includes both serialization and insertion. So I think it is actually the same

Side note, regarding put_bytes not existing in sig. I tried to abstract away the bytes. Converting between types and bytes is the whole purpose of the Database layer. Ideally the ledger code can just use a Database like it's a hashmap, without worrying about serialization. It feels like a leaky abstraction if the Database layer can be bypassed, exposing its internal data to the business logic of the ledger.

That said, actually we do have a getBytes method, so I didn't do a great job at my goal haha. This is because shreds themselves are already bytes. The only reason getBytes exists is to provide a mechanism for managing the lifetime of the shred bytes without needing to copy them into a new allocation. I think this could actually be handled better using the type system, and we could probably eliminate getBytes completely. I decided was willing to compromise on the abstraction a bit, to get the job done quickly.

This PR removes the hardcoded gpa, with manual leak detection from the TestState used in tests. I believe the reason for this was to have more stack trace frames than what the std.testing.allocator offers, but this proved problematic when using the TestingState in other contexts like benchmarking. See #275 (comment)

dadepo added 8 commits September 18, 2024 10:23

Added benchWriteSmall benchmark

2db1e1b

updated test data

b8ef631

Added benchReadSequential

e9eb195

Rename test file

baa2fd8

Added benchReadRandom

6f46e71

Set c allocator

fd8952a

typo

a7e6bfb

Check in test data

fe078fb

0xNineteen changed the title ~~feat(Blockstore): Blockstore benchmark~~ feat(blockstore): benchmarks Sep 23, 2024

dadepo added 5 commits September 29, 2024 14:56

Update test name

439100b

Reuse ledger.insert_shred.insertShredsForTest

7f3c380

Add benchSerializeWriteBincode

4d55ccd

Update title

8c80082

Add benchReadBincode

4528620

dadepo commented Sep 30, 2024

View reviewed changes

dadepo marked this pull request as ready for review September 30, 2024 11:41

dnut self-requested a review October 1, 2024 19:42

dnut requested changes Oct 2, 2024

View reviewed changes

dadepo added 5 commits October 7, 2024 13:19

Moved ledger benchmark to own file

5fa8282

Typo fix

ab2086b

Fmt

077cda2

import via the root source

692d561

camelCase for functions

44fa660

dadepo mentioned this pull request Oct 7, 2024

refactor(ledger): remove manual leak detection in ledger tests #305

Merged

dadepo added 2 commits October 7, 2024 17:55

Merge branch 'main' into dade/blockstore-benchmark

cc8ac6c

Pass std.heap.c_allocator to TestState in ledger benchmark

dbcd012

dadepo added 2 commits October 7, 2024 18:38

Drop benchShreds in favour of re-using testShreds

b84cc5c

Remove unused imports

dcccd63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blockstore): benchmarks #275

feat(blockstore): benchmarks #275

dadepo commented Sep 18, 2024 •

edited

Loading

dadepo Sep 30, 2024

dnut Oct 2, 2024

dadepo Oct 7, 2024 •

edited

Loading

dadepo Sep 30, 2024 •

edited

Loading

dnut Oct 2, 2024

dadepo Oct 7, 2024

dadepo Sep 30, 2024

dnut Oct 2, 2024

dadepo Oct 7, 2024

dadepo Sep 30, 2024

dnut Oct 2, 2024

dnut Oct 2, 2024

dadepo Oct 7, 2024

dadepo Oct 7, 2024 •

edited

Loading

dnut Oct 2, 2024

dadepo Oct 7, 2024

dnut Oct 2, 2024

dadepo Oct 7, 2024

dnut Oct 2, 2024

dadepo Oct 7, 2024

dnut Oct 2, 2024

dadepo Oct 7, 2024

dnut Oct 2, 2024

dnut Oct 2, 2024

dnut Oct 2, 2024

dnut Oct 2, 2024

dnut Oct 2, 2024

	pub const BenchmarLegger = struct {
	pub const BenchmarkLedger = struct {

feat(blockstore): benchmarks #275

Are you sure you want to change the base?

feat(blockstore): benchmarks #275

Conversation

dadepo commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadepo Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

dadepo Sep 30, 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

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

dadepo Oct 7, 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

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

dadepo commented Sep 18, 2024 •

edited

Loading

dadepo Oct 7, 2024 •

edited

Loading

dadepo Sep 30, 2024 •

edited

Loading

dadepo Oct 7, 2024 •

edited

Loading