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

GODRIVER-3455 Make BSON benchmarks more representative of real use cases. #1974

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthewdale
Copy link
Collaborator

GODRIVER-3455

Summary

  • Make BenchmarkMarshal and BenchmarkUnmarshal BSON benchmarks parallel.
  • Add more test cases to BenchmarkMarshal and BenchmarkUnmarshal.
  • Make it easier to run encoding-specific benchmarks (e.g. BSON, ExtJSON, etc) by restructuring the subtests to make the encoding the top-level subtest.

Background & Motivation

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Mar 5, 2025
Copy link
Contributor

API Change Report

No changes found!

}{
{
desc: "simple struct",
desc: "simple to struct",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "simple struct" reads better:

BenchmarkUnmarshal/simple_struct

@@ -182,9 +182,11 @@ func readExtJSONFile(filename string) map[string]interface{} {
}

func BenchmarkMarshal(b *testing.B) {
b.ReportAllocs()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will report anything for the test cases, suggest moving b.ReportAllocs() to the inner-most subtest:

b.Run(tc.desc, func(b *testing.B) {
	b.ReportAllocs()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			_, err := Marshal(tc.value)
			if err != nil {
				b.Errorf("error marshalling BSON: %s", err)
			}
		}
	})
})

Same for BenchmarkUnmarshal

@@ -194,6 +196,23 @@ func BenchmarkMarshal(b *testing.B) {
desc: "nested struct",
value: nestedInstance,
},
{
desc: "simple D",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case doesn't seem like it will be any different than encodetestInstance. But if we keep it, suggest defining it globally and adding it to BenchmarkUnmarshal as well.

}

func BenchmarkUnmarshal(b *testing.B) {
b.ReportAllocs()

cases := []struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider an alternative pattern for the test cases which will help avoid having to write out similar cases repeatedly. This also allows us to pre-compute the file reads.

deepBSON := readExtJSONFile("deep_bson.json.gz")
flatBSON := readExtJSONFile("flat_bson.json.gz")
fullBSON := readExtJSONFile("full_bson.json.gz")

inputs := []struct {
	name  string
	value any
}{
	{"simple", encodetestInstance},
	{"nested", nestedInstance},
	{"deep_bson.json.gz", deepBSON},
	{"flat_bson.json.gz", flatBSON},
	{"full_bson.json.gz", fullBSON},
}

destinations := []struct {
	name string
	dst  func() any
}{
	{"to struct", func() any { return &encodetest{} }},
	{"to map", func() any { return &map[string]any{} }},
	{"to D", func() any { return &D{} }},
}

var cases []struct {
	desc  string
	value any
	dst   func() any
}

for _, input := range inputs {
	for _, dest := range destinations {
		cases = append(cases, struct {
			desc  string
			value any
			dst   func() any
		}{
			desc:  input.name + " " + dest.name,
			value: input.value,
			dst:   dest.dst,
		})
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low Low Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants