-
Notifications
You must be signed in to change notification settings - Fork 367
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
Benchmark ListEffectivePolicies #638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -82,10 +104,10 @@ func userWithPolicies(t *testing.T, s auth.Service, policies []*model.Policy) st | |||
|
|||
func TestDBAuthService_ListPaged(t *testing.T) { | |||
const chars = "abcdefghijklmnopqrstuvwxyz" | |||
db, _ := testutil.GetDB(t, databaseURI) | |||
defer db.Close() | |||
adb, _ := testutil.GetDB(t, databaseURI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adb
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db package name :) (a database) - not creative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhhh.... db
....
auth/service_test.go
Outdated
serviceWithCache := auth.NewDBAuthService(adb, crypt.NewSecretStore(someSecret), authparams.ServiceCache{ | ||
Enabled: true, | ||
Size: 1024, | ||
TTL: 20 * time.Second, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure you're allowed to do this. AFAIU go bench
runs increasing b.N
until the benchmark has run for "long enough" (--benchtime
). So you're not allowed to change the time an iteration takes based on N. So... if it tries too large an N, and runs for 17.2 seconds, it will automatically run somewhat slower.
See e.g. https://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-go, "traps for young players".
Either pick a really short TTL (on the order of 500ms+-50ms) and measure the effects of expiry, or pick a really long TTL (on the order of 1d) and measure the effects of a cache without expiry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm passing the benchmark object to
benchmarkListEffectivePolicies
and reset the timer - so the above code is just benchmark setup code, not part of the measure.
Using b.N for callingListEffectivePolicies
.
Missing where I miss-used the N. if I keep using the same parameters for the cache system.
I can move the service creation out of the Run call to make sure it is not part of the bench - but I am sure call reset should solve this one. - I'll add another test to try to simulate the expiry - wanted the good numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test with cache with low ttl value
Codecov Report
@@ Coverage Diff @@
## master #638 +/- ##
==========================================
- Coverage 41.10% 41.08% -0.03%
==========================================
Files 129 129
Lines 9975 9975
==========================================
- Hits 4100 4098 -2
- Misses 5337 5341 +4
+ Partials 538 536 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
Could you just run it and copy-paste results (and hardware, e.g. cat /proc/cpuinfo
) so we see what it does?
@@ -82,10 +104,10 @@ func userWithPolicies(t *testing.T, s auth.Service, policies []*model.Policy) st | |||
|
|||
func TestDBAuthService_ListPaged(t *testing.T) { | |||
const chars = "abcdefghijklmnopqrstuvwxyz" | |||
db, _ := testutil.GetDB(t, databaseURI) | |||
defer db.Close() | |||
adb, _ := testutil.GetDB(t, databaseURI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhhh.... db
....
No description provided.