-
Notifications
You must be signed in to change notification settings - Fork 60
Add performance tests #152
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
Conversation
0c7d836
to
6c411b6
Compare
80498a2
to
453e249
Compare
6dc5ba6
to
9da1984
Compare
9da1984
to
723873e
Compare
Added calls of b.ResetTimer() to all Benchmark tests before running bench loop. That call clears all counters of allocs, timers etc. The preparation before bench loop affected perf results. Part of #122
723873e
to
a93024a
Compare
a93024a
to
cf95b4b
Compare
cf95b4b
to
961ff1b
Compare
961ff1b
to
86f904d
Compare
Makefile
Outdated
| tee ${BENCH_FILE} | ||
benchstat ${BENCH_FILE} | ||
|
||
${BENCH_REFERENCE_REPO}: |
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.
BENCH_REFERENCE_REPO
- I would rename it toGO_TARANTOOL_URL
for clarityif .. then .. fi
is bulky, I would replace it with operator for test condition and command itself- you do not need to change dir just for making a
git clone
Cloning the repository into ${BENCH_PATH} for generating benchmark reference file
- target just clones a repository, we don't know here what for we do it. I would remove "for generating benchmark reference file"
Please consider my changes:
diff --git a/Makefile b/Makefile
index ca1fb95..40ff068 100644
--- a/Makefile
+++ b/Makefile
@@ -9,8 +9,9 @@ TEST_PATH ?= ${PROJECT_DIR}/...
BENCH_FILE := ${BENCH_PATH}/bench.txt
REFERENCE_FILE := ${BENCH_PATH}/reference.txt
BENCH_FILES := ${REFERENCE_FILE} ${BENCH_FILE}
-BENCH_REFERENCE_REPO := ${BENCH_PATH}/go-tarantool
BENCH_OPTIONS := -bench=. -run=^Benchmark -benchmem -benchtime=${DURATION} -count=${COUNT}
+GO_TARANTOOL_URL := https://github.com/tarantool/go-tarantool
+GO_TARANTOOL_DIR := ${BENCH_PATH}/go-tarantool
.PHONY: clean
clean:
@@ -83,11 +84,11 @@ ${BENCH_FILE} bench:
| tee ${BENCH_FILE}
benchstat ${BENCH_FILE}
-${BENCH_REFERENCE_REPO}:
- @echo "Cloning the repository into ${BENCH_PATH} for generating benchmark reference file"
- cd ${BENCH_PATH} && if ! [ -e go-tarantool ]; then git clone https://github.com/tarantool/go-tarantool.git; fi
+${GO_TARANTOOL_DIR}:
+ @echo "Cloning the repository into ${GO_TARANTOOL_DIR}"
+ [ ! -e ${GO_TARANTOOL_DIR} ] && git clone ${GO_TARANTOOL_URL} ${GO_TARANTOOL_DIR}
-${REFERENCE_FILE}: ${BENCH_REFERENCE_REPO}
+${REFERENCE_FILE}: ${GO_TARANTOOL_DIR}
@echo "Running benchmark tests from master for using results in bench-diff target"
cd ${BENCH_PATH}/go-tarantool && git pull && go test ${BENCH_PATH}/go-tarantool/... ${BENCH_OPTIONS} 2>&1 \
| tee ${REFERENCE_FILE}
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.
Fixed. Thank you for review!
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 for changes!
LGTM after processing my comment about target for cloning repository.
e0326f5
to
ed4d55e
Compare
I played around the benchmarking toolkit and have some feedback. DocumentationI would add a sections regarding benchmarking. I mean, let's add a header for the section to easily spot it visually. I would start from a quickstart ( I would also add some recommendations how to achieve stable results. I briefly written my suggestions below.
Also I would note that $ export PATH="/home/${USER}/go/bin:${PATH}" UsabilityI met several problems.
Benchmarking resultsBefore I disbled TurboBoost I received stable degradation for on But when I disabled TurboBoost, I received 1-2% noise, which looks suitable for comparisons. There are rare spikes, so we anyway should run the comparison several times to ensure that results are good. It is okay for the manual tool. My resolution is that the tooling is suitable for manual comparisons. Future thoughtsIt would be nice to look around unit-test-like benchmarks / microbenchmarks, which would test parts of the connector without actual interation with tarantool. I think they will be more sensitive. I would not make it a separate task, but would experiment around in the scope of some relevant work. |
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.
The changes are okay for me in general.
I propose to look on my feedback (summarized above), fix simple problems, leave warnings in CONTRIBUTING.md on hard problems and land it to master
. Feel free to proceed without further reviews.
9a1f104
to
4f77e34
Compare
Added benchmarks of large Select and Replace. Added a new target in Makefile for running benchmark tests. Added a new space in config.lua for large Select tests. Added a new target in Makefile for measuring performance degradation between current changes and master. Added a new line in gitignore for ignoring artifacts from bench target. Added a new step for running benchmark tests in ci. Added description to the CONTRIBUTING.md for how to run benchmark tests. Closes #122
4f77e34
to
0d2c868
Compare
Added benchmarks of large Select and Replace.
Added a new target in Makefile for running benchmark tests.
Added a new step in ci for running benchmark.
Added description to the README for how to run benchmark tests.
Added calls of b.ResetTimer() to all Benchmark tests before the bench loop.
That call clears all counters of allocs, timers etc. in internal counter.
The preparation before bench loop affected perf results.
Fixes #122