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

Add pure read, write and read-write benchmarks #2

Merged
merged 2 commits into from
Feb 22, 2019
Merged

Conversation

mangalaman93
Copy link
Contributor

@mangalaman93 mangalaman93 commented Feb 15, 2019

Old PR: #1

Comparing following libraries -

The benchmarks are inspired from github repo
github.com/ben-manes/caffeine and file
caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/GetPutBenchmark.java


This change is Reviewable

@mangalaman93 mangalaman93 self-assigned this Feb 15, 2019
cache_bench_test.go Outdated Show resolved Hide resolved
cache_bench_test.go Outdated Show resolved Hide resolved
cache_bench_test.go Outdated Show resolved Hide resolved
cache_bench_test.go Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Feb 15, 2019

CLA assistant check
All committers have signed the CLA.

@mangalaman93 mangalaman93 force-pushed the benchmark branch 3 times, most recently from 74d3de4 to 231a102 Compare February 16, 2019 06:34
Comparing following libraries -
  * bigcache github.com/allegro/bigcache
  * freecache github.com/coocood/freecache
  * sync Map https://golang.org/pkg/sync/#Map

The benchmarks are inspired from github repo
github.com/ben-manes/caffeine and file
caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/GetPutBenchmark.java
Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @golangcibot and @manishrjain)


cache_bench_test.go, line 119 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of cache.Set is not checked (from errcheck)

Done


cache_bench_test.go, line 121 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of cache.Reset is not checked (from errcheck)

Done.


cache_bench_test.go, line 148 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of cache.Set is not checked (from errcheck)

Done.


cache_bench_test.go, line 199 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of cache.Set is not checked (from errcheck)

Done.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @golangcibot, @mangalaman93, and @manishrjain)


cache.go, line 2 at r1 (raw file):

/*
 * Copyright 2015-2018 Dgraph Labs, Inc. and Contributors
  1. The year that this code was written.

cache_bench_test.go, line 2 at r1 (raw file):

/*
 * Copyright 2015-2018 Dgraph Labs, Inc. and Contributors

2019 here and elsewhere.


cache_bench_test.go, line 63 at r1 (raw file):

	// scrambled zipfian to ensure same keys are not together
	z := generator.NewScrambledZipfian(0, maxKey, generator.ZipfianConstant)

Why not rand.Zipf? It would avoid a dep on TiDB!

Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @golangcibot, @mangalaman93, and @manishrjain)


cache.go, line 2 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…
  1. The year that this code was written.

Done.


cache_bench_test.go, line 2 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

2019 here and elsewhere.

Done.


cache_bench_test.go, line 63 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why not rand.Zipf? It would avoid a dep on TiDB!

I thought about it. But I am not confident in using rand.Zipf. Two reasons -

  • It takes two additional parameters s,v. I am not sure how they are used in order to generate the distribution and what their effect is on distribution
  • Not confident that it won't cluster same keys together (whether it is scrambled)

I could probably figure this out, though, doesn't sound worth spending time on it. The random number generator that I am using is very similar to the one that Ben used.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @golangcibot and @mangalaman93)


cache_bench_test.go, line 63 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I thought about it. But I am not confident in using rand.Zipf. Two reasons -

  • It takes two additional parameters s,v. I am not sure how they are used in order to generate the distribution and what their effect is on distribution
  • Not confident that it won't cluster same keys together (whether it is scrambled)

I could probably figure this out, though, doesn't sound worth spending time on it. The random number generator that I am using is very similar to the one that Ben used.

OK. We won't want to have the cache depend on TiDB. So, maybe it makes sense to put all these in a separate repository, as discussed today.


go.mod, line 1 at r2 (raw file):

module github.com/dgraph-io/caffeine

Let's not worry about all this for now, while we're still in flux about the deps. So, remove the go.mod and go.sum.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: remove the go.mod and go.sum, so we don't finalize on deps yet.

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @golangcibot and @mangalaman93)

@mangalaman93 mangalaman93 merged commit 0d8658c into master Feb 22, 2019
@mangalaman93 mangalaman93 deleted the benchmark branch February 22, 2019 11:49
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.

4 participants