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

ctxlock: use safe implementation of keyify #1459

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

Conversation

BradLugo
Copy link
Contributor

@BradLugo BradLugo commented Jan 3, 2025

I noticed while trying to implement some examples of libindex that .Index() method will panic on an empty claircore.Manifest. I tracked this down to keyify panicing when it receives an empty string as input. I didn't really try to figure out the exact bug here because reflect.StringHeader is marked as deprecated anyway, so I tried the following function:

func keyify(key string) []byte {
	h := fnv.New64a()
	p := unsafe.StringData(key)
	b := unsafe.Slice((*byte)(p), len(key))
	h.Write(b)
	return h.Sum(nil)
}

That seemed to fix the problem, but then I was curious if this caused a performance hit, so I ran BenchmarkUncontended between that implementation and the previous implementation. The results looked similiar. Since the safe implementation was there and ready for benchmarking, I figured I'd test it as well. Here are the results from the tests:

safe benchmark
❯ go test -bench BenchmarkUncontended ./pkg/ctxlock -benchtime 1m
goos: darwin
goarch: arm64
pkg: github.com/quay/claircore/pkg/ctxlock
BenchmarkUncontended-10           742704             89153 ns/op            1256 B/op         17 allocs/op
--- BENCH: BenchmarkUncontended-10
    bench_test.go:10: using embedded database
    adapter.go:32: info Dialing PostgreSQL server host=localhost
    adapter.go:32: info Exec sql=CREATE ROLE rolef6286f908286622b LOGIN PASSWORD 'rolef6286f908286622b'; args=[] time=15.2385ms commandTag=CREATE ROLE pid=89492
    adapter.go:32: info Exec commandTag=CREATE DATABASE pid=89492 sql=CREATE DATABASE db8ebf054880a36f1c WITH OWNER rolef6286f908286622b ENCODING 'UTF8'; args=[] time=46.004875ms
    adapter.go:32: info closed connection pid=89492
    adapter.go:32: info Dialing PostgreSQL server host=localhost
    adapter.go:32: info Exec sql=CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; args=[] time=1.931208ms commandTag=CREATE EXTENSION pid=89493
    adapter.go:32: info closed connection pid=89493
    db.go:78: config: {Host:localhost Database:db8ebf054880a36f1c User:rolef6286f908286622b Password:rolef6286f908286622b Port:39363}
    testing.go:98: {"level":"debug","zlog.testname":"BenchmarkUncontended","component":"internal/ctxlock/Locker.reconnect","gen":"1","caller":"ctxlock.go:153","message":"set up"}
        ... [output truncated]
PASS
ok      github.com/quay/claircore/pkg/ctxlock   70.941s

Results:
BenchmarkUncontended-10 742704 89153 ns/op 1256 B/op 17 allocs/op

old_unsafe benchmark
❯ go test -bench BenchmarkUncontended ./pkg/ctxlock -benchtime 1m -tags old_safe
goos: darwin
goarch: arm64
pkg: github.com/quay/claircore/pkg/ctxlock
BenchmarkUncontended-10           818978             87649 ns/op            1256 B/op         17 allocs/op
--- BENCH: BenchmarkUncontended-10
    bench_test.go:10: using embedded database
    adapter.go:32: info Dialing PostgreSQL server host=localhost
    adapter.go:32: info Exec time=15.076875ms commandTag=CREATE ROLE pid=93451 sql=CREATE ROLE rolee5605a7b4266a8ae LOGIN PASSWORD 'rolee5605a7b4266a8ae'; args=[]
    adapter.go:32: info Exec pid=93451 sql=CREATE DATABASE dba83bd9aa5e678d8c WITH OWNER rolee5605a7b4266a8ae ENCODING 'UTF8'; args=[] time=41.451375ms commandTag=CREATE DATABASE
    adapter.go:32: info closed connection pid=93451
    adapter.go:32: info Dialing PostgreSQL server host=localhost
    adapter.go:32: info Exec sql=CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; args=[] time=1.554459ms commandTag=CREATE EXTENSION pid=93452
    adapter.go:32: info closed connection pid=93452
    db.go:78: config: {Host:localhost Database:dba83bd9aa5e678d8c User:rolee5605a7b4266a8ae Password:rolee5605a7b4266a8ae Port:33326}
    testing.go:98: {"level":"debug","component":"internal/ctxlock/Locker.reconnect","zlog.testname":"BenchmarkUncontended","gen":"1","caller":"ctxlock.go:153","message":"set up"}
        ... [output truncated]
PASS
ok      github.com/quay/claircore/pkg/ctxlock   76.170s

Results:
BenchmarkUncontended-10 818978 87649 ns/op 1256 B/op 17 allocs/op

new_unsafe benchmark
❯ go test -bench BenchmarkUncontended ./pkg/ctxlock -benchtime 1m -tags new_unsafe
goos: darwin
goarch: arm64
pkg: github.com/quay/claircore/pkg/ctxlock
BenchmarkUncontended-10           776305             87822 ns/op            1256 B/op         17 allocs/op
--- BENCH: BenchmarkUncontended-10
    bench_test.go:10: using embedded database
    adapter.go:32: info Dialing PostgreSQL server host=localhost
    adapter.go:32: info Exec commandTag=CREATE ROLE pid=97744 sql=CREATE ROLE role1400dd44238cf4d0 LOGIN PASSWORD 'role1400dd44238cf4d0'; args=[] time=15.369417ms
    adapter.go:32: info Exec pid=97744 sql=CREATE DATABASE db5b1fbd8ee021771 WITH OWNER role1400dd44238cf4d0 ENCODING 'UTF8'; args=[] time=43.988333ms commandTag=CREATE DATABASE
    adapter.go:32: info closed connection pid=97744
    adapter.go:32: info Dialing PostgreSQL server host=localhost
    adapter.go:32: info Exec sql=CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; args=[] time=1.493459ms commandTag=CREATE EXTENSION pid=97745
    adapter.go:32: info closed connection pid=97745
    db.go:78: config: {Host:localhost Database:db5b1fbd8ee021771 User:role1400dd44238cf4d0 Password:role1400dd44238cf4d0 Port:37605}
    testing.go:98: {"level":"debug","zlog.testname":"BenchmarkUncontended","component":"internal/ctxlock/Locker.reconnect","gen":"1","caller":"ctxlock.go:153","message":"set up"}
        ... [output truncated]
PASS
ok      github.com/quay/claircore/pkg/ctxlock   72.663s

Results:
BenchmarkUncontended-10 776305 87822 ns/op 1256 B/op 17 allocs/op

It seems that all implementations have similar performance. Therefore, I propose we remove all unsafe implementations.

I've attached a patch with the changes required to run the benchmarks described above. I encourage reviewers to run them as well, especially folks who have non-darwin systems.

benchmarks.patch

@BradLugo BradLugo requested a review from a team as a code owner January 3, 2025 00:22
@BradLugo BradLugo requested review from crozzy and removed request for a team January 3, 2025 00:22
@RTann RTann requested a review from hdonnay January 3, 2025 00:45
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

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

LGTM

The benchmarks for the unsafe implementation of keyify don't show a
significant improvement over the safe implementation. These changes
also fix a bug where keyify panics when an empty string input is
received.

Signed-off-by: Brad Lugo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants