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

fix(benchmark-test): suggestions for #10975 #10982

Draft
wants to merge 2 commits into
base: ahmad-benchmark-test
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/agoric-cli/src/sdk-package-names.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export default [
"@agoric/async-flow",
"@agoric/base-zone",
"@agoric/benchmark",
"@agoric/benchmark-test",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was automatically generated from the change to the packages's package.json name:. This was one of the impediments to doing a yarn build at the root directory.

"@agoric/boot",
"@agoric/builders",
"@agoric/cache",
Expand Down
35 changes: 0 additions & 35 deletions packages/benchmark-test/benchmark.js

This file was deleted.

49 changes: 0 additions & 49 deletions packages/benchmark-test/benchmark.test.js

This file was deleted.

33 changes: 26 additions & 7 deletions packages/benchmark-test/package.json
Original file line number Diff line number Diff line change
@@ -1,9 +1,28 @@
{
"name": "benchmark",
"version": "1.0.0",
"type": "module",
"scripts": {
"test": "yarn rollup -c && eshost -h v8,xs dist/bundle.js"
},
"license": "MIT"
"name": "@agoric/benchmark-test",
Copy link
Member Author

Choose a reason for hiding this comment

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

Generated the correct entry in sdk-package-names.js

"version": "1.0.0",
"type": "module",
"scripts": {
"build": "exit 0",
"prepack": "tsc --build tsconfig.build.json",
"postpack": "git clean -f '*.d.ts*' '*.tsbuildinfo'",
"test": "yarn rollup -c && eshost -h 'V8,Moddable XS' dist/bundle.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do any of the "Pre-reqs of running yarn test" instructions from #10975 , because I was curious how far I'd get with my existing configuration. I still have not tried that, so I don't know what difference it would make. That said:

By debugging into eshost code to figure out why it was failing, it was not finding the names "v8" or "xs". The names it seemed to want instead were "V8" and "Moddable XS". I tried it and it worked, but I suspect this is not a good change. I'm just reporting it as the outcome of my experiment.

"bench": "yarn rollup -c && node --jitless --cpu-prof dist/bundle.js",
"lint": "run-s --continue-on-error lint:*",
"lint:eslint": "eslint .",
"lint:types": "tsc"
},
"author": "Agoric",
"license": "Apache-2.0",
"bugs": {
"url": "https://github.com/Agoric/agoric-sdk/issues"
},
"homepage": "https://github.com/Agoric/agoric-sdk#readme",
Comment on lines +15 to +20
Copy link
Member Author

@erights erights Feb 11, 2025

Choose a reason for hiding this comment

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

All blindly imitated from patterns found in other packages. Note the license change from "MIT" to "Apache-2.0". It is our policy to use this license by default, unless there is a compelling reason for another choice. That said, I do like "MIT" ;)

"devDependencies": {
"tsd": "^0.31.1",
"@rollup/plugin-node-resolve": "^16.0.0",
"@rollup/plugin-commonjs": "^28.0.2",
"@endo/init": "^1.1.8",
"@endo/pass-style": "^1.4.8"
}
Comment on lines +21 to +27
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed by the top level "yarn build"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll note that this package does not depend on anything in the agoric-sdk repo, and many of the things we'd like to benchmark reside into the endo repo. In general, it is fine for packages in agoric-sdk to depend on things in endo, but not vice versa. Thus, at some point before this is announced for general use (even internally), it should be moved to the endo repository. But feel free to do it at any earlier time as is convenient. IMO, you'll find it most convenient to move it asap, because the longer we develop this in the wrong place, the more unknown assumptions we'll accumulate that will make this migration difficult.

Choose a reason for hiding this comment

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

I already tried moving it to endo repo. But I was not able to do yarn, reason being

  • Difference of yarn versions in the root directory and current directory.
  • After resolving that the CLI was stuck where I had to chose the versions of all the packages named @endo/* but after selecting the version for the single package the process just quit.

Let me try again and paste more findings here.

Copy link
Member Author

Choose a reason for hiding this comment

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

You may find the endo ./scripts/create-package.sh useful. That's what I always start with to create a new endo package.

Attn @kriskowal

Choose a reason for hiding this comment

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

Thanks for the script. I used it and I was able to create the new package.

}
8 changes: 2 additions & 6 deletions packages/benchmark-test/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,12 @@ import resolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';

export default {
input: 'benchmark.test.js',
input: 'test/benchmark.test.js',
Copy link
Member Author

@erights erights Feb 11, 2025

Choose a reason for hiding this comment

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

I generally don't like substantive source files in the top level directory, but rather in subdirectories "src/", "test/", or "tools/". Putting this package's own internal tests into "test/" is natural. Putting the tool itself into "tools/" rather than "src/" is a bit strange. But "tools/" generally contains those things that are appropriate for others to use via a "devDependencies:" rather than a "dependencies:". In other words, it is to support use for testing by other modules.

Choose a reason for hiding this comment

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

Make sense. Test should be in the test/ directory.

output: {
file: 'dist/bundle.js',
format: 'iife',
name: 'bundle',
sourcemap: false,
},
plugins: [
resolve(),
commonjs(),
],
plugins: [resolve(), commonjs()],
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I made this format change manually. I suspect it was done by a top level "yarn format", but I am not certain.

};

58 changes: 58 additions & 0 deletions packages/benchmark-test/test/benchmark.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import '@endo/init';
import { passStyleOf } from '@endo/pass-style';
import { test, benchmark } from '../tools/benchmark.js';
Copy link
Member Author

@erights erights Feb 11, 2025

Choose a reason for hiding this comment

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

Github is supposed to be better at detecting file rename and motion as different from a deletion and addition for purposes of review. Oh well. In any case, I think this is the only line I changed from your original.


async function runBenchmark() {
await test('Benchmark', async t => {
await benchmark(
'Empty object',
t,
async () => {
passStyleOf(harden({}));
},
0.01,
);
});

await test('Another Benchmark', async t => {
await benchmark(
'Alphabets object',
t,
async () => {
passStyleOf(
harden({
a: 12,
b: 13,
c: 14,
d: 15,
e: 16,
f: 17,
g: 18,
h: 19,
i: 20,
j: 21,
k: 22,
l: 23,
m: 24,
n: 25,
o: 26,
p: 27,
q: 28,
r: 29,
s: 30,
t: 31,
u: 32,
v: 33,
w: 34,
x: 35,
y: 36,
z: 37,
}),
);
},
0.001,
);
});
}

void runBenchmark();
39 changes: 39 additions & 0 deletions packages/benchmark-test/tools/benchmark.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// import { performance } from 'perf_hooks';
const performance = { now: () => Date.now() };
Comment on lines +1 to +2
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change anything in this file beyond moving it to "tools/". However, I considered changing this line to

Suggested change
// import { performance } from 'perf_hooks';
const performance = { now: () => Date.now() };
// import { performance } from 'perf_hooks';
const performance = { now: () => Date.now() * 1000 };

Since your naming it "performance.now" suggests that we're using it as a low resolution replacement on those platforms on which it is absent. I did not make this change because I'm not sure what else I would then need to change.

Choose a reason for hiding this comment

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

I am of this opinion that we should start writing tests with Date.now by merging the PR. Meanwhile I can work on something that gives us time in nano seconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. But should we multiply the output of Date.now() by 1000 to better prepare for switching to the real performance.now() eventually, on platforms where that is available?

Choose a reason for hiding this comment

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

For now, I am keeping it consistent with both engines. This means that even if node has the perf_hooks I am still gonna use Date.now for both.
But, I am inclined to see the time in nano seconds because this is where we are heading. So, it will be better to not give developers any surprise. One day they wake up and update the package and see all of their tests are failing because they used the ms instead of ns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the benchmarking is in our control, why not multiply by 1000 regardless wether we use Date.now or performance.now?
If we keep it in nanoseconds, we can benefit later when we actually have nanoseconds precision without breaking anything


async function benchmark(name, t, fn, expedtedTime, iterations = 10000) {
await null;
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this line due to our lint checking rules for await safety

const start = performance.now(); // 3
for (let i = 0; i < iterations; i += 1) {
await fn();
}
const end = performance.now(); // 8
const avgTime = (end - start) / iterations;

console.log(`${name} | Average time: ${avgTime}ms`);
t.assert(
avgTime < expedtedTime,
`Expected ${avgTime} to be less than ${expedtedTime}`,
);
}

async function test(name, fn) {
await null;
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this line due to our lint checking rules for await safety

try {
console.log('Running test: ', name);
await fn({ assert, truthy });
console.log(`✅ Passed`);
} catch (err) {
console.log(`❌ Failed: ${err.message}`);
}
}

function assert(condition, message = 'Assertion failed') {
if (!condition) throw new Error(message);
}

function truthy(value, message = 'Expected a truthy value') {
if (!value) throw new Error(message);
}

export { benchmark, test };
6 changes: 6 additions & 0 deletions packages/benchmark-test/tsconfig.build.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"extends": [
"./tsconfig.json",
"../../tsconfig-build-options.json"
]
}
Comment on lines +1 to +6
Copy link
Member Author

@erights erights Feb 11, 2025

Choose a reason for hiding this comment

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

blindly imitated

10 changes: 10 additions & 0 deletions packages/benchmark-test/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// This file can contain .js-specific Typescript compiler config.
{
"extends": "../../tsconfig.json",
"compilerOptions": {},
"include": [
"tools",
"test",
"rollup.config.js"
Comment on lines +6 to +8
Copy link
Member Author

@erights erights Feb 11, 2025

Choose a reason for hiding this comment

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

I did not move "rollup.config.js" to a subdirectory, because it is directly used by "yarn test" at the top.

Aside from the contents of "includes", I blindly imitated the rest.

]
}
12 changes: 12 additions & 0 deletions packages/benchmark-test/typedoc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"extends": [
"../../typedoc.base.json"
],
"entryPoints": [
"tools/benchmark.js"
],
"validation": {
"notExported": false,
"invalidLink": false,
}
}
Comment on lines +1 to +12
Copy link
Member Author

Choose a reason for hiding this comment

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

blindly imitated

Loading
Loading