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.

Sign up for GitHub

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

feat: improve the latency and throughput formatting in the result table #233

Merged
merged 9 commits into from
Jan 29, 2025
Merged

feat: improve the latency and throughput formatting in the result table #233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
775d0d1
feat: improve the latency and throughput formatting
pallosp Jan 28, 2025
392af32
feat: improve the latency and throughput formatting
pallosp Jan 28, 2025
39a9c70
Update src/bench.ts
jerome-benoit Jan 29, 2025
0dc3e94
Update src/bench.ts
jerome-benoit Jan 29, 2025
102bb80
Update test/index.test.ts
jerome-benoit Jan 29, 2025
507f5d2
Update test/index.test.ts
jerome-benoit Jan 29, 2025
6b2b6fc
Update test/index.test.ts
jerome-benoit Jan 29, 2025
c16af02
Update test/index.test.ts
jerome-benoit Jan 29, 2025
59a62ae
Merge branch 'main' into main
jerome-benoit Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/bench.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
import { createBenchEvent } from './event'
import { Task } from './task'
import {
formatNumber,
invariant,
type JSRuntime,
mToNs,
Expand Down Expand Up @@ -254,12 +255,12 @@ export class Bench extends EventTarget {
}
: (convert?.(task) ?? {
'Task name': task.name,
'Latency average (ns)': `${mToNs(task.result.latency.mean).toFixed(2)} \xb1 ${task.result.latency.rme.toFixed(2)}%`,
'Latency average (ns)': `${formatNumber(mToNs(task.result.latency.mean), 5, 2)} \xb1 ${task.result.latency.rme.toFixed(2)}%`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can also use the formaNumber helper for the standard deviation.

Displaying number only in the column would be better, but I do not think it's possible because of string only characters usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For percentages less than 1000% the helper returns the same string as toFixed(2). I slightly prefer keeping toFixed(2) because of its simpler semantics.

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
'Latency median (ns)': `${mToNs(task.result.latency.p50!).toFixed(2)}${Number.parseFloat(mToNs(task.result.latency.mad!).toFixed(2)) > 0 ? ` \xb1 ${mToNs(task.result.latency.mad!).toFixed(2)}` : ''}`,
Copy link
Collaborator

@jerome-benoit jerome-benoit Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Please keep the threshold condition here : when the median absolute deviation is upper zero, it's a sign of statistical significance issue during the benchmark and a valuable information.

You can use the same helper to display it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether I get correctly what the absolute deviation is supposed to measure.

So far I've only seen it in the table when the number of samples was even. If the median for even number of samples is problematic, shouldn't the task be executed one more time instead of reporting a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's totally unrelated to the number of samples. It's a robust statistical indicator of the statistical significance of the median value. And the only one for median. Like the standard deviation (or the absolute mean deviation) is for the average.

Adding a threshold to it is only way to reduce the information to display.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. After some debugging I understood what's going on. There is a bug in absoluteDeviation that makes mad exactly zero as opposed to a small positive number in most cases. I filed #234.

'Latency median (ns)': +formatNumber(mToNs(task.result.latency.p50!), 5, 2),
'Throughput average (ops/s)': `${task.result.throughput.mean.toFixed(0)} \xb1 ${task.result.throughput.rme.toFixed(2)}%`,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
'Throughput median (ops/s)': `${task.result.throughput.p50!.toFixed(0)}${Number.parseInt(task.result.throughput.mad!.toFixed(0), 10) > 0 ? ` \xb1 ${task.result.throughput.mad!.toFixed(0)}` : ''}`,
'Throughput median (ops/s)': Math.round(task.result.throughput.p50!),
Samples: task.result.latency.samples.length,
})
/* eslint-enable perfectionist/sort-objects */
Expand Down
23 changes: 23 additions & 0 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,29 @@ export const nToMs = (ns: number) => ns / 1e6
*/
export const mToNs = (ms: number) => ms * 1e6

/**
* @param x number to format
* @param targetDigits number of digits in the output to aim for
* @param maxFractionDigits hard limit for the number of digits after the decimal dot
* @returns formatted number
*/
export const formatNumber = (x: number, targetDigits: number, maxFractionDigits: number): string => {
// Round large numbers to integers, but not to multiples of 10.
// The actual number of significant digits may be more than `targetDigits`.
if (Math.abs(x) >= 10 ** targetDigits) {
return x.toFixed()
}

// Round small numbers to have `maxFractionDigits` digits after the decimal dot.
// The actual number of significant digits may be less than `targetDigits`.
if (Math.abs(x) < 10 ** (targetDigits - maxFractionDigits)) {
return x.toFixed(maxFractionDigits)
}

// Round medium magnitude numbers to have exactly `targetDigits` significant digits.
return x.toPrecision(targetDigits)
}

let hrtimeBigint: () => bigint
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
if (typeof (globalThis as any).process?.hrtime?.bigint === 'function') {
Expand Down
23 changes: 23 additions & 0 deletions test/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { expect, test } from 'vitest'

import { formatNumber } from '../src/utils'

test('formatting integers', () => {
expect(formatNumber(123456, 5, 2)).toBe('123456')
expect(formatNumber(12345, 5, 2)).toBe('12345')
expect(formatNumber(1234, 5, 2)).toBe('1234.0')
expect(formatNumber(123, 5, 2)).toBe('123.00')
expect(formatNumber(12, 5, 2)).toBe('12.00')
expect(formatNumber(1, 5, 2)).toBe('1.00')
expect(formatNumber(0, 5, 2)).toBe('0.00')
expect(formatNumber(-1, 5, 2)).toBe('-1.00')
})

test('formatting floats', () => {
expect(formatNumber(123456.789, 5, 2)).toBe('123457')
expect(formatNumber(12345.6789, 5, 2)).toBe('12346')
expect(formatNumber(1234.56789, 5, 2)).toBe('1234.6')
expect(formatNumber(123.456789, 5, 2)).toBe('123.46')
expect(formatNumber(12.3456789, 5, 2)).toBe('12.35')
expect(formatNumber(-12.3456789, 5, 2)).toBe('-12.35')
})
Loading