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

Don't use pointers for Hints #1

Merged
merged 4 commits into from
Oct 3, 2023
Merged

Don't use pointers for Hints #1

merged 4 commits into from
Oct 3, 2023

Conversation

bombsimon
Copy link
Owner

@bombsimon bombsimon commented Oct 3, 2023

Revert "Use pointers for `Hints` and `HintSet`"

This reverts commit fe1b071fea20b93067f9373db546dd8320307ca8.

I converted the Hints to be pointers because gocritic (hugeParam) reports that the hints are 80 bytes and heavy to copy.

When using pointers instead we become sensitive to nil data and since the Hints holds pointers to HintSets we needed to add checks to ensure no values were actually left nil. I started to wonder how much this would cost compared to just copying the hints and wrote some benchmarks. These are the results:

Hints is pointer

› go test -run XXX -bench .
goos: darwin
goarch: arm64
pkg: github.com/bombsimon/jtd-infer-go
BenchmarkInferOneRowNoMissingHints-10            1039881              1122 ns/op
BenchmarkInferThousandRowsNoMissingHints-10         1266            922018 ns/op
BenchmarkInferOneRowMissingHints-10               961521              1217 ns/op
BenchmarkInferThousandRowsMissingHints-10           1194           1009154 ns/op
PASS
ok      github.com/bombsimon/jtd-infer-go       5.832s

Hints is copied

› go test -run XXX -bench .
goos: darwin
goarch: arm64
pkg: github.com/bombsimon/jtd-infer-go
BenchmarkInferOneRowNoMissingHints-10            1210305               995.1 ns/op
BenchmarkInferThousandRowsNoMissingHints-10         1512            776240 ns/op
BenchmarkInferOneRowMissingHints-10              1200474              1008 ns/op
BenchmarkInferThousandRowsMissingHints-10           1540            784011 ns/op
PASS
ok      github.com/bombsimon/jtd-infer-go       6.926s

Given this the code is much simpler and less fragile when not using pointers.

@bombsimon bombsimon merged commit 3b0c904 into main Oct 3, 2023
4 checks passed
@bombsimon bombsimon deleted the pointer-benchmark branch October 3, 2023 19:20
@bombsimon
Copy link
Owner Author

Some more testing done with different sizes of hints.

func benchmarkWithHints(n int, b *testing.B) {
	rows := generateRows(100)
	hints := WithoutHints()

	for i := 0; i < n; i++ {
		hints.Enums = hints.Enums.Add([]string{"a", "b", "c"})
		hints.Values = hints.Values.Add([]string{"a", "b", "c"})
		hints.Discriminator = hints.Discriminator.Add([]string{"a", "b", "c"})
	}

	for n := 0; n < b.N; n++ {
		InferStrings(rows, hints)
	}
}

Hints is pointer

› go test ./ -run XXX -bench .
goos: darwin
goarch: arm64
pkg: github.com/bombsimon/jtd-infer-go
BenchmarkInferOneRowNoMissingHints-10            1045717              1129 ns/op
BenchmarkInferThousandRowsNoMissingHints-10         1278            944240 ns/op
BenchmarkInferWith0Hints-10                        12938             93904 ns/op
BenchmarkInferWith1Hints-10                        12402             94638 ns/op
BenchmarkInferWith10Hints-10                       10000            104948 ns/op
BenchmarkInferWith100Hints-10                       6236            188500 ns/op
BenchmarkInferWith1000Hints-10                      1099           1119160 ns/op
PASS
ok      github.com/bombsimon/jtd-infer-go       11.301s

Hint is copied

› go test ./ -run XXX -bench .
goos: darwin
goarch: arm64
pkg: github.com/bombsimon/jtd-infer-go
BenchmarkInferOneRowNoMissingHints-10            1171512              1008 ns/op
BenchmarkInferThousandRowsNoMissingHints-10         1528            782710 ns/op
BenchmarkInferWith0Hints-10                        15328             78575 ns/op
BenchmarkInferWith1Hints-10                        15459             78166 ns/op
BenchmarkInferWith10Hints-10                       13785             88467 ns/op
BenchmarkInferWith100Hints-10                       6664            177562 ns/op
BenchmarkInferWith1000Hints-10                      1100           1073912 ns/op
PASS
ok      github.com/bombsimon/jtd-infer-go       11.847s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant