fix(benchmark-test): suggestions for #10975 #10982
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
9380d0b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f0cb53d.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-suggestion-ahmad-bench.agoric-sdk.pages.dev |
| @@ -5,6 +5,7 @@ export default [ | |||
| "@agoric/async-flow", | |||
| "@agoric/base-zone", | |||
| "@agoric/benchmark", | |||
| "@agoric/benchmark-test", | |||
There was a problem hiding this comment.
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.
| "test": "yarn rollup -c && eshost -h v8,xs dist/bundle.js" | ||
| }, | ||
| "license": "MIT" | ||
| "name": "@agoric/benchmark-test", |
There was a problem hiding this comment.
Generated the correct entry in sdk-package-names.js
packages/benchmark-test/package.json
Outdated
| "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", | ||
| "lint": "run-s --continue-on-error lint:*", | ||
| "lint:eslint": "eslint .", | ||
| "lint:types": "tsc" | ||
| }, |
There was a problem hiding this comment.
For all these but tests, I just blindly imitated the entries from other packages.
| "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", |
There was a problem hiding this comment.
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.
| "author": "Agoric", | ||
| "license": "Apache-2.0", | ||
| "bugs": { | ||
| "url": "https://github.com/Agoric/agoric-sdk/issues" | ||
| }, | ||
| "homepage": "https://github.com/Agoric/agoric-sdk#readme", |
There was a problem hiding this comment.
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" | ||
| } |
There was a problem hiding this comment.
Needed by the top level "yarn build"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for the script. I used it and I was able to create the new package.
| resolve(), | ||
| commonjs(), | ||
| ], | ||
| plugins: [resolve(), commonjs()], |
There was a problem hiding this comment.
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.
| @@ -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', | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Make sense. Test should be in the test/ directory.
| @@ -0,0 +1,58 @@ | |||
| import '@endo/init'; | |||
| import { passStyleOf } from '@endo/pass-style'; | |||
| import { test, benchmark } from '../tools/benchmark.js'; | |||
There was a problem hiding this comment.
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.
| // import { performance } from 'perf_hooks'; | ||
| const performance = { now: () => Date.now() }; |
There was a problem hiding this comment.
I did not change anything in this file beyond moving it to "tools/". However, I considered changing this line to
| // 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| { | ||
| "extends": [ | ||
| "./tsconfig.json", | ||
| "../../tsconfig-build-options.json" | ||
| ] | ||
| } |
| "tools", | ||
| "test", | ||
| "rollup.config.js" |
There was a problem hiding this comment.
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.
| { | ||
| "extends": [ | ||
| "../../typedoc.base.json" | ||
| ], | ||
| "entryPoints": [ | ||
| "tools/benchmark.js" | ||
| ], | ||
| "validation": { | ||
| "notExported": false, | ||
| "invalidLink": false, | ||
| } | ||
| } |
| @@ -2020,6 +2020,11 @@ | |||
| resolved "https://registry.yarnpkg.com/@jridgewell/sourcemap-codec/-/sourcemap-codec-1.4.15.tgz#d7c6e6755c78567a951e04ab52ef0fd26de59f32" | |||
| integrity sha512-eF2rxCRulEKXHTRiDrDy6erMYWqNw4LPdQ8UQA4huuxaQsVeRPFl2oM8oDGxMFhJUWZf9McpLtJasDDZb/Bpeg== | |||
|
|
|||
| "@jridgewell/sourcemap-codec@^1.5.0": | |||
There was a problem hiding this comment.
All the changes to yarn.lock were automatically generated. I have not done any sanity checking on these.
|
Because this PR is merely suggestions for #10975 , I intent to leave it in Draft. @muhammadahmadasifbhatti @gibson042 , I'm listing you both as reviewers to bring it to your attention. I do not suggest you do any serious review of this PR apart from #10975 itself. |
|
I am getting a CI error in trying to run under XS, even though it works locally for me. I do not understand enough to debug this. But it may well be related to my experimenting with not following your "Pre-reqs of running yarn test" instructions. I leave this remaining problem with you two. |
| const performance = { now: () => Date.now() }; | ||
|
|
||
| async function benchmark(name, t, fn, expedtedTime, iterations = 10000) { | ||
| await null; |
There was a problem hiding this comment.
I added this line due to our lint checking rules for await safety
| } | ||
|
|
||
| async function test(name, fn) { | ||
| await null; |
There was a problem hiding this comment.
I added this line due to our lint checking rules for await safety
|
These tools enabled endojs/endo#2716 ! |
|
Hey @erights I have created the new PR in the |
Closes: #XXXX Refs: Agoric/agoric-sdk#10975 Agoric/agoric-sdk#10982 ## Description Using the benchmark tool from #10975 by @muhammadahmadasifbhatti as modified by #10982 to produce flamegraphs (interactive in vscode) I iterated on that benchmark test which already had very specific simple examples exercising `passStyleOf`. Together with the snippet at https://github.com/Agoric/agoric-sdk/blob/acbb5da3c5a52bab8db319fd696aed70146f9a89/.github/actions/restore-node/action.yml#L156 (which @gibson042 brought to my attention) ```sh $ scripts/get-packed-versions.sh ~/endo | scripts/resolve-versions.sh $ yarn install && yarn build $ cd packages/benchmark-test $ yarn bench ``` and then clicking on the latest *.cpuprofile file, assuming you've already installed the https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-js-profile-flame vscode extension. I was able to iterate and tinker on possible improvements, to see what made how much of a difference. This PR has the improvements to `passStyleOf` that I and my reviewers have came up with so far. @gibson042 suggested the main technique used: to avoid redundant checking by breaking up `assertValid` so we could still running the checks of `canBeValid` twice. ### Security Considerations I claim that this PR is a pure refactoring. If true, none. ***Reviewers***, please review with a skeptical eye. `passStyleOf` is security critical, so any observable difference might lead to an opportunity for an adversary. ### Scaling Considerations Even though I iterated on a specialized benchmark exercising `passStyleOf`, I believe I only made changes that would also be an improvement for the typical cases. ### Documentation Considerations If this is a pure refactor, none. ### Testing Considerations If this is a pure refactor, none. In fact, this PR did not need to change any tests, providing some weak evidence that it is indeed a pure refactor. ### Compatibility Considerations If this is a pure refactor, none. ### Upgrade Considerations If this is a pure refactor, none.
closes: #XXXX
refs: #10975 endojs/endo#2716
Description
Suggested changed to #10975, to be treated as if these are comments directly on the #10975 PR. Suggested changes to be explained by inline comments on this PR.
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
Once this new feature settles down, we will need to document it. But that will be awhile.
Testing Considerations
The point. To be able to run benchmarks as tests
Upgrade Considerations
none