-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
ci(NODE-6755): add tags to benchmarks #4419
base: main
Are you sure you want to change the base?
Conversation
|
||
// Constant used to scale normalized_throughput results for ping benchmark to be roughly in the | ||
// range 0 - 9 to make it easier to work with | ||
export const NORMALIZED_PING_SCALING_CONST = 1000; |
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 know this is kind of splitting hairs, but I was hoping this scaling would happen internally to the primes benchmark because ideally we'd never touch this parameter or the primes benchmark itself once we set this up. I thought it might be most helpful for the output of our primes benchmark itself to be a unit (order of 1^0
) so that it represents what we think of as the baseline cpu performance (particularly since in bson it will be used for all the scalings). I wanted to take the mean of the current stable region for the baseline cpu performance and scale it to === 1
, then when we look at the baseline cpu performance, we can see that it's 2x or 3x or 0.5x relative to when we started tracking it, and that number would be easy to interpret. And when we factor that number from any benchmark (for the driver, just ping), that cpu-normalized benchmark would have a more direct connection to the absolute result.
assert.ok(primesBench, 'primes bench results not found!'); | ||
const pingThroughput = pingBench.metrics[0].value; | ||
const primesThroughput = primesBench.metrics[0].value; | ||
primesBench.metrics.push({ 'name': 'normalized_throughput', value: NORMALIZED_PING_SCALING_CONST * pingThroughput / primesThroughput }); |
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.
this should be just primesThroughput, no fancy math, taking my other comment into account
const finalResults = calculateCompositeBenchmarks(results); | ||
function calculateNormalizedResults(results: MetricInfo[]): MetricInfo[] { | ||
const primesBench = results.find(r => r.info.test_name === 'primes'); | ||
const pingBench = results.find(r => r.info.test_name === 'ping'); |
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.
this will be able to go away, then adjust the loop below to continue iff 'primes', and push metrics.value / primesThroughput
for ping
Description
What is changing?
tags
toBenchmarkModule
type to facilitate adding tags to benchmarks that get reported when usingperf.send
TAG
enum to driver.mts that contains the tags we use across all of our benchmarks for ease of use and to prevent accidental misspellings of tagsnormalized_throughput
measurement to all benchmarksNORMALIZED_PING_SCALING_CONST * ping_throughput_in_megabytes_per_second/primes_throughput_in_megabytes_per_second
throughput_in_megabytes_per_second/ping_throughput_in_megabytes_per_second
Is there new documentation needed for these changes?
What is the motivation for this change?
Release Highlight
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript