Skip to content

Instance API#271

Open
kaworu wants to merge 4 commits into
mainfrom
pr/kaworu/instance-api
Open

Instance API#271
kaworu wants to merge 4 commits into
mainfrom
pr/kaworu/instance-api

Conversation

@kaworu
Copy link
Copy Markdown
Member

@kaworu kaworu commented Mar 24, 2026

Implement a new struct API (see #21), backward-compatibility wrappers, and adapt the cmd module accordingly.

@kaworu kaworu requested a review from rolinh March 24, 2026 14:51
@kaworu kaworu self-assigned this Mar 24, 2026
@kaworu kaworu added enhancement New feature or request module/main Affects the main module module/flow Affects the flow module labels Mar 24, 2026
@kaworu
Copy link
Copy Markdown
Member Author

kaworu commented Mar 24, 2026

Still a draft for now, as I'm chasing a bug in the benchmark

% go test -bench .
goos: linux
goarch: amd64
pkg: github.com/cilium/fake/flow
cpu: AMD Ryzen 7 PRO 8840HS w/ Radeon 780M Graphics
BenchmarkFakeFlow-16    	panic: runtime error: index out of range [-1]

goroutine 79 [running]:
math/rand.(*rngSource).Uint64(...)
	/home/alex/sdk/go1.25.5/src/math/rand/rng.go:249
math/rand.(*rngSource).Int63(0x6b1888574b0426e0?)
	/home/alex/sdk/go1.25.5/src/math/rand/rng.go:234 +0x92
math/rand.(*Rand).Int63(...)
	/home/alex/sdk/go1.25.5/src/math/rand/rand.go:96
math/rand.(*Rand).Int31(...)
	/home/alex/sdk/go1.25.5/src/math/rand/rand.go:110
math/rand.(*Rand).Int31n(0xc00003a510, 0x19)
	/home/alex/sdk/go1.25.5/src/math/rand/rand.go:145 +0x53
math/rand.(*Rand).Intn(0xe?, 0x10?)
	/home/alex/sdk/go1.25.5/src/math/rand/rand.go:183 +0x25
github.com/cilium/fake.(*faker).App(...)
	/home/alex/go/src/github.com/cilium/fake/names.go:22
github.com/cilium/fake.(*faker).K8sLabels(0xc0005501e0)
	/home/alex/go/src/github.com/cilium/fake/k8s.go:13 +0xd5
github.com/cilium/fake/flow.(*flowfaker).Policies(0xc000566480)
	/home/alex/go/src/github.com/cilium/fake/flow/policy.go:18 +0xe2
github.com/cilium/fake/flow.Policies(...)
	/home/alex/go/src/github.com/cilium/fake/flow/legacy.go:65
github.com/cilium/fake/flow.(*flowfaker).NewFlow(0xc0004f0030, {0x0, 0x0, 0x0?})
	/home/alex/go/src/github.com/cilium/fake/flow/flow.go:197 +0x212
github.com/cilium/fake/flow_test.BenchmarkFakeFlow.func1()
	/home/alex/go/src/github.com/cilium/fake/flow/fake_test.go:26 +0x4b
created by github.com/cilium/fake/flow_test.BenchmarkFakeFlow in goroutine 82
	/home/alex/go/src/github.com/cilium/fake/flow/fake_test.go:23 +0x8b
exit status 2
FAIL	github.com/cilium/fake/flow	0.168s

@kaworu kaworu linked an issue Mar 24, 2026 that may be closed by this pull request
@kaworu kaworu force-pushed the pr/kaworu/instance-api branch from 4fc0e73 to 6b4fa99 Compare March 24, 2026 14:56
@kaworu kaworu force-pushed the pr/kaworu/instance-api branch from 6b4fa99 to b3e29e7 Compare March 24, 2026 15:27
Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu kaworu force-pushed the pr/kaworu/instance-api branch from b3e29e7 to e470c8c Compare March 25, 2026 19:49
@kaworu kaworu marked this pull request as ready for review March 25, 2026 19:50
@kaworu
Copy link
Copy Markdown
Member Author

kaworu commented Mar 25, 2026

Did a quick round of benchmark testing before / after the patch. Without going too deep:

  1. randv1 has improved a lot since address scaling issues #21 had been written, and the switch to a struct API doesn't improve significantly the performances over the previous package-level functions using a lockedSource from the randv1 func API.
  2. the recent randv2 change has improved performances a bit (around 8% faster)
  3. ChaCha8 and PCG show around the same numbers, suggesting the bottleneck is not the PRNG when generating fake flows.

Copy link
Copy Markdown
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

I got curious regarding this statement:

the switch to a struct API doesn't improve significantly the performances over the previous package-level functions using a lockedSource from the randv1 func API.

So I benchmarked locally with the following code for the main package:

func BenchmarkLegacyAPI(b *testing.B) {
	b.Run("Sequential", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			_ = fake.IP()
			_ = fake.MAC()
			_ = fake.Port()
			_ = fake.K8sNamespace()
		}
	})

	b.Run("Parallel", func(b *testing.B) {
		b.RunParallel(func(pb *testing.PB) {
			for pb.Next() {
				_ = fake.IP()
				_ = fake.MAC()
				_ = fake.Port()
				_ = fake.K8sNamespace()
			}
		})
	})
}

func BenchmarkInstanceAPI(b *testing.B) {
	b.Run("Sequential", func(b *testing.B) {
		f := fake.New()
		for i := 0; i < b.N; i++ {
			_ = f.IP()
			_ = f.MAC()
			_ = f.Port()
			_ = f.K8sNamespace()
		}
	})

	b.Run("Parallel", func(b *testing.B) {
		b.RunParallel(func(pb *testing.PB) {
			f := fake.New()
			for pb.Next() {
				_ = f.IP()
				_ = f.MAC()
				_ = f.Port()
				_ = f.K8sNamespace()
			}
		})
	})
}

func BenchmarkInstanceAPIShared(b *testing.B) {
	b.Run("Parallel", func(b *testing.B) {
		f := fake.New()
		b.RunParallel(func(pb *testing.PB) {
			for pb.Next() {
				_ = f.IP()
				_ = f.MAC()
				_ = f.Port()
				_ = f.K8sNamespace()
			}
		})
	})
}

And the results I get are the following:

% go test -bench .
goos: linux
goarch: amd64
pkg: github.com/cilium/fake
cpu: AMD Ryzen 7 PRO 8840HS w/ Radeon 780M Graphics 
BenchmarkLegacyAPI/Sequential-16 	4988082	      232.9 ns/op
BenchmarkLegacyAPI/Parallel-16   	3533332	      347.7 ns/op
BenchmarkInstanceAPI/Sequential-16         	5572246	      217.2 ns/op
BenchmarkInstanceAPI/Parallel-16           	23392348	       56.28 ns/op
BenchmarkInstanceAPIShared/Parallel-16     	6864799	      169.3 ns/op
PASS
ok  	github.com/cilium/fake	7.127s

So while the sequential tests don't show any significant improvements, the parallel ones show a ~7x speedup, which is very significant.

And when it comes to the flow package:

func BenchmarkLegacyFlowAPI(b *testing.B) {
	b.Run("Sequential", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			_ = flow.New()
		}
	})

	b.Run("Parallel", func(b *testing.B) {
		b.RunParallel(func(pb *testing.PB) {
			for pb.Next() {
				_ = flow.New()
			}
		})
	})
}

func BenchmarkInstanceFlowAPI(b *testing.B) {
	b.Run("Sequential", func(b *testing.B) {
		f := flow.NewFaker()
		for i := 0; i < b.N; i++ {
			_ = f.NewFlow()
		}
	})

	b.Run("Parallel", func(b *testing.B) {
		b.RunParallel(func(pb *testing.PB) {
			f := flow.NewFaker()
			for pb.Next() {
				_ = f.NewFlow()
			}
		})
	})
}

The story is very similar:

% go test -bench .
goos: linux
goarch: amd64
pkg: github.com/cilium/fake/flow
cpu: AMD Ryzen 7 PRO 8840HS w/ Radeon 780M Graphics 
BenchmarkFakeFlow-16           	 485557	     2481 ns/op
BenchmarkLegacyFlowAPI/Sequential-16         	 140707	     8594 ns/op
BenchmarkLegacyFlowAPI/Parallel-16           	 132897	     8954 ns/op
BenchmarkInstanceFlowAPI/Sequential-16       	 137576	     8414 ns/op
BenchmarkInstanceFlowAPI/Parallel-16         	 587812	     2052 ns/op
PASS
ok  	github.com/cilium/fake/flow	7.330s

So here we have roughly a 4x speedup in the parallel case.

With that said, the "legacy" API we now provide for backwards compatibility is very likely to be a significant regression for the parallel use-case due to the introduction of the global mutex which will inevitably lead to lock contention. At the very least, we should document this and encourage users to move to the instance API if they generate fake data in parallel.

Comment thread flow/verdict.go Outdated
Comment thread fake.go
Comment thread fake.go
)

// Faker is the main interface exposed to generate fake data.
type Faker interface { //nolint:interfacebloat
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the rationale to introduce an interface rather than simply returning a struct? I don't really see the benefit of doing this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is to hide the embedded rand and make the contract explicit. I'm neutral toward this though (we don't have to embed the rand stuff).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have some issues with this approach as a 13 method interface will never be implemented in practice and, even if it were implemented elsewhere, every time we add a new method, we would break the existing implementation(s). So I'd really prefer we return a concrete *Faker. However, I think we should be able to address your concern regarding hiding the embedded rang as we can just not export it and keep it private.

kaworu added 3 commits March 31, 2026 17:39
Before this patch, the fake library would only expose package defined
methods. This had the limitation of scaling poorly when used by multiple
goroutines (although both recent golang versions and rand/v2 addressed
the scaling issues), and did not abstract the PRNG very well.

This commit introduces a new faker struct, constructors, and struct
methods matching the exposed API. Each faker struct has its own instance
PRNG and should not be used concurrently.

Additionally, a package level struct is created and used by package
level defined functions for full retro-compatibility.

The following commits will introduce the same change to the flow module,
and adapt the cmd module to use the new API.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Introduce a flowfaker struct with its own PRNG instance, constructors,
and struct methods matching the existing package API.

Package-level functions are preserved in legacy.go for backwards
compatibility, delegating to a shared package-level flowfaker instance.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Use a Faker struct in each command handler and its methods rather than
the (now) legacy package-level functions.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu
Copy link
Copy Markdown
Member Author

kaworu commented Mar 31, 2026

I got curious regarding this statement:

the switch to a struct API doesn't improve significantly the performances over the previous package-level functions using a lockedSource from the randv1 func API.

So I benchmarked locally with the following code for the main package:

[…]

So here we have roughly a 4x speedup in the parallel case.

I think the benchmark you presented is testing something different than what I was referring to, sorry for the confusion.

Your test covers both "legacy" and "instance" API, both backed by the instance implementation, at the same commit. In that case, as you correctly pointed out, the global mutex introduce a non-negligible overhead that you measured.

My message mentioned the randv1 func API using a lockedSource, because I used the benchmark file introduced in this PR (sometime slightly adapted) to test different commits:

  1. bb74959 ­— the commit right before the randv2 change
  2. 9ac0640 — the commit introducing randv2 (ChaCha8)
  3. HEAD of this patch with the struct API — the proposed new API and implementation

I found that there is a slight improvement between 1. and 2., and no significant difference between 2. and 3.

@kaworu kaworu force-pushed the pr/kaworu/instance-api branch from e470c8c to 58fd3f6 Compare March 31, 2026 16:16
@kaworu kaworu requested a review from rolinh April 1, 2026 15:05
Copy link
Copy Markdown
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Your test covers both "legacy" and "instance" API, both backed by the instance implementation, at the same commit. In that case, as you correctly pointed out, the global mutex introduce a non-negligible overhead that you measured.

In practice, this is a regression for the users of the legacy API though. Can we at least document it? We should mention that these functions exist for backward compatibility only and that they are "slow" if used in a concurrent context. I'd document it both in the package and in the readme and cover the differences/use-cases for legacy and instance APIs.

Also, I think we could also commit the parallel benchmarks I submitted so that we can monitor for potential future regressions.

My message mentioned the randv1 func API using a lockedSource (...)

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request module/flow Affects the flow module module/main Affects the main module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

address scaling issues

2 participants