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

blockstore: fast path for AllKeysChan using the index #372

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

MichaelMure
Copy link
Contributor

Note: I'm not entirely sure I didn't break some assumption.

close #242

Crude benchmark:

func BenchmarkAllKeysChan(b *testing.B) {
	path := filepath.Join(b.TempDir(), "bench-large-v2.car")
	generateRandomCarV2File(b, path, 10<<20) // 10 MiB
	defer os.Remove(path)

	bs, err := blockstore.OpenReadWrite(path, nil)
	if err != nil {
		b.Fatal(err)
	}

	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		c, err := bs.AllKeysChan(context.Background())
		if err != nil {
			b.Fatal(err)
		}
		for range c {
		}
	}
}

Before:

BenchmarkAllKeysChan-12 772 2022389 ns/op 89049 B/op 1617 allocs/op

After

BenchmarkAllKeysChan-12 8824 195779 ns/op 30865 B/op 642 allocs/op

10.3X faster, allocate 2.5X less memory.

v2/blockstore/readwrite.go Outdated Show resolved Hide resolved
@MichaelMure
Copy link
Contributor Author

@rvagg I did what you suggested, the performance boost is even more significant:

Before:

BenchmarkAllKeysChan-12 772 2022389 ns/op 89049 B/op 1617 allocs/op

After

BenchmarkAllKeysChan-12 10000 105874 ns/op 144 B/op 2 allocs/op

19.1X faster, allocate 735X less memory.

@MichaelMure
Copy link
Contributor Author

Removing the BlockstoreUseWholeCIDs check lead to a CI failure though, should I just put it back?

   === RUN   TestBlockstorePutSameHashes
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      readwrite_test.go:242: expected blockstore without UseWholeCIDs to flatten on AllKeysChan
      ```

@MichaelMure
Copy link
Contributor Author

Removing the BlockstoreUseWholeCIDs check lead to a CI failure though, should I just put it back?

Answering my own question, that doesn't fix that test. Yet the previous code with the multihash was passing that test ‽

I'll need someone more versed in that code to figure out if that test makes sense, and if so what should be done about it.

v2/blockstore/readwrite.go Outdated Show resolved Hide resolved
v2/internal/store/insertionindex.go Outdated Show resolved Hide resolved
close ipld#242

Crude benchmark:
```
func BenchmarkAllKeysChan(b *testing.B) {
	path := filepath.Join(b.TempDir(), "bench-large-v2.car")
	generateRandomCarV2File(b, path, 10<<20) // 10 MiB
	defer os.Remove(path)

	bs, err := blockstore.OpenReadWrite(path, nil)
	if err != nil {
		b.Fatal(err)
	}

	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		c, err := bs.AllKeysChan(context.Background())
		if err != nil {
			b.Fatal(err)
		}
		for range c {
		}
	}
}

func BenchmarkAllKeysChanUseWholeCIDs(b *testing.B) {
	path := filepath.Join(b.TempDir(), "bench-large-v2.car")
	generateRandomCarV2File(b, path, 10<<20) // 10 MiB
	defer os.Remove(path)

	bs, err := blockstore.OpenReadWrite(path, nil, carv2.UseWholeCIDs(true))
	if err != nil {
		b.Fatal(err)
	}

	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		c, err := bs.AllKeysChan(context.Background())
		if err != nil {
			b.Fatal(err)
		}
		for range c {
		}
	}
}
```

Before:
> BenchmarkAllKeysChan-12		885	   1256865 ns/op	   88911 B/op	    1617 allocs/op
> BenchmarkAllKeysChanUseWholeCIDs-12	1010	   1253543 ns/op	   57861 B/op	     976 allocs/op

After
> BenchmarkAllKeysChan-12		8971	    135906 ns/op	   30864 B/op	     642 allocs/op
> BenchmarkAllKeysChanUseWholeCIDs-12	13904	     86140 ns/op	     144 B/op	       2 allocs/op

BenchmarkAllKeysChan            --- 9.25X faster, allocate 2.9X less memory
BenchmarkAllKeysChanUseWholeCID --- 14.5X faster, allocate 401X less memory.
@MichaelMure
Copy link
Contributor Author

@rvagg I applied your changes. Final result:

Before:
> BenchmarkAllKeysChan-12		885	   1256865 ns/op	   88911 B/op	    1617 allocs/op
> BenchmarkAllKeysChanUseWholeCIDs-12	1010	   1253543 ns/op	   57861 B/op	     976 allocs/op

After
> BenchmarkAllKeysChan-12		8971	    135906 ns/op	   30864 B/op	     642 allocs/op
> BenchmarkAllKeysChanUseWholeCIDs-12	13904	     86140 ns/op	     144 B/op	       2 allocs/op

BenchmarkAllKeysChan            --- 9.25X faster, allocate 2.9X less memory
BenchmarkAllKeysChanUseWholeCID --- 14.5X faster, allocate 401X less memory.

@rvagg rvagg merged commit a4629d3 into ipld:master Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

blockstore: ReadOnly.AllKeysChan should be able to use the index in some cases
3 participants