Details

Reviewers

tomek
• abosh
• jon
varun

Commits

rCOMMaac3407a4577: [services] Tests - Add performance tests base

Summary

Depends on D4579

Adding scripts and initial test files for the performance tests.

Test Plan

cd services
yarn run-performance-tests blob
yarn run-performance-tests backup

For now, the desired behavior for them is to fail with a message "unimplemented".

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Jul 20 2022, 4:14 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJul 20 2022, 4:14 AM

• karol edited the summary of this revision. (Show Details)Jul 20 2022, 4:23 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, • abosh.

• karol added a parent revision: D4579: [services] Tests - Refactor integration tests.

Harbormaster completed remote builds in B10662: Diff 14679.Jul 20 2022, 4:26 AM

• karol requested review of this revision.Jul 20 2022, 4:26 AM

• karol added a child revision: D4581: [services] Tests - Add performance tests base for Blob.Jul 20 2022, 4:29 AM

@abosh is sick right now and can't review, but @jon is a good bet since he is good at both Rust and bash

• jon added inline comments.Jul 20 2022, 2:54 PM

services/scripts/run_performance_tests.sh
6 ↗	(On Diff #14679)	Do we need this? we seem to only set it [14:46:20] jon@jon-desktop /home/jon/comm/comm (jonringer/tunnelbroker) $ rg COMM_SERVICES_DEV_MODE services/scripts/run_unit_tests.sh 6:export COMM_SERVICES_DEV_MODE=1 services/scripts/run_integration_tests.sh 6:export COMM_SERVICES_DEV_MODE=1
11 ↗	(On Diff #14679)	Generally "status" logging isn't outputed to stdout
23 ↗	(On Diff #14679)	should go to stderr
25 ↗	(On Diff #14679)	the semi-colon isn't needed if there's also a newline. Is this an established convention?
44 ↗	(On Diff #14679)	I don't see this file in master or the previous diff. If it's meant to be conditionally sourced, then we should do something like

tomek requested changes to this revision.Jul 21 2022, 4:19 AM

tomek added inline comments.

services/scripts/run_performance_tests.sh
6 ↗	(On Diff #14679)	Shouldn't we use `SANDBOX`?
13 ↗	(On Diff #14679)	For performance testing, shouldn't we use `bench`? https://doc.rust-lang.org/cargo/commands/cargo-bench.html

This revision now requires changes to proceed.Jul 21 2022, 4:19 AM

• karol added inline comments.Jul 21 2022, 6:42 AM

services/scripts/run_performance_tests.sh
6 ↗	(On Diff #14679)	Both of these variables can be removed I believe
11 ↗	(On Diff #14679)	I just ran this and it is output to the std out. What do you mean?
13 ↗	(On Diff #14679)	I'm not sure about this for two reasons: It runs the `main` file and runs functions annotated with `#[bench]`. I'd rather have these tests living next to the integration tests because sharing the code between them is easier that way. It is unstable and available on nightly builds. Maybe I missed something, but I think we're better with the performance tests as they are.
23 ↗	(On Diff #14679)	ok
25 ↗	(On Diff #14679)	Just trying to put semicolons everywhere but I do not mind.
44 ↗	(On Diff #14679)	it's here: https://github.com/CommE2E/comm/blob/71e1f1ceda6e64f709457dae969fe4551f917eb3/services/.env

address feedback

Harbormaster completed remote builds in B10736: Diff 14775.Jul 21 2022, 6:56 AM

• jon added inline comments.Jul 21 2022, 1:46 PM

services/scripts/run_performance_tests.sh
11 ↗	(On Diff #14679)	generally stdout is for output that you would want to capture with `$(<command>)`, here you're just tracing the progress of the command, which is usually dumped to something like stderr. It's not really a big issue. Just a personal preference.
44 ↗	(On Diff #14679)	having relative paths dependent on where a file gets executed from is a bad anti-pattern. For example, if were I were to execute this from the project root, it would fail. We should probably determine the file location, then construct paths from that known location. An example: # should work on linux + mac SCRIPT_DIR=$(cd "$(dirname "$0")"; pwd -P) source "$SCRIPT_DIR"/../.env then this will work if it's called from root, `services/` or `services/scripts/`

@jon it would be easier if you used the machine state provided by the phabricator. If you have serious doubts about the code from the diff, please use "request changes". It's easier because it get in my queue and if I am to make any changes, I can just update the diff instead of using "plan changes". Thanks ;)

services/scripts/run_performance_tests.sh
11 ↗	(On Diff #14679)	here you're just tracing the progress of the command, which is usually dumped to something like stderr. That sounds odd. I don't mind changing this but I don't see the logic here. stderr is for errors, right? Printing out status like this does not seem like an error/exceptional situation. Where does this statement come from? How'd you come up with that?
44 ↗	(On Diff #14679)	This script is not meant to be run directly, we should use npm scripts. But since we can do this from nested directories, your point's valid. I'll use the line you provided, thanks.

fix source

Harbormaster completed remote builds in B10756: Diff 14797.Jul 22 2022, 12:54 AM

tomek accepted this revision.Jul 22 2022, 2:12 AM

tomek added 1 blocking reviewer(s): • jon.

tomek added inline comments.

services/scripts/run_performance_tests.sh
13 ↗	(On Diff #14679)	Ok, that makes sense. But it is still a good idea to read about performance testing in Rust, especially on the optimization flags that should be added and other good practices (running with the right profile). Also, having an option to mark performance tests with attribute sounds like a more reliable strategy than creating a file naming convention - but that might be complicated if we don't want to use the tool.

• karol added a reviewer: varun.Jul 26 2022, 3:10 AM

@jon you are blocking here, could you please either accept or request changes? Thanks.

services/scripts/run_performance_tests.sh
13 ↗	(On Diff #14679)	I agree, I'm going to take a look. Although, there's one thing that I think we should keep in mind: we're not looking at the "standard" performance tests for Rust - we're not benchmarking the rust functions themselves, but we're testing the grpc in fact. So when doing the research, most of the results are for the "rust performance tests" while what we really need is "grpc performance tests written in rust". Am I thinking correctly?

rust code looks good to me

Main concern was the implicit reliance on PWD, I'm fine with the other changes

This revision is now accepted and ready to land.Jul 26 2022, 12:14 PM

• jon added inline comments.Jul 26 2022, 12:18 PM

services/scripts/run_performance_tests.sh
44 ↗	(On Diff #14679)	The script will inherit the CWD from the calling script. Still best practice to not rely on implicit assumptions. But you applied the changes, so all is good