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: don't require --allow-net #158

Merged
merged 7 commits into from
Aug 5, 2023
Merged

Conversation

NfNitLoop
Copy link
Contributor

This relates to conversation in #31

It looks like the module actually loads faster with the embedded wasm file, despite it being larger and requiring base64-decoding.

Before 0697afd:

〉deno task bench
Task bench deno bench -A
cpu: AMD Ryzen 7 3700X 8-Core Processor
runtime: deno 1.35.3 (x86_64-pc-windows-msvc)

file:///C:/Users/Cody/code/dax/benchmarks/mod.bench.ts
benchmark           time (avg)             (min … max)       p75       p99      p995
------------------------------------------------------ -----------------------------
loadWasmModule      83.62 ms/iter   (81.72 ms … 86.68 ms)  83.93 ms  86.68 ms  86.68 ms

After 0697afd:

benchmark           time (avg)             (min … max)       p75       p99      p995
------------------------------------------------------ -----------------------------
loadWasmModule      80.41 ms/iter   (79.28 ms … 83.79 ms)  80.47 ms  83.79 ms  83.79 ms

This also removes the need to --allow-write or --allow-net:

#!/usr/bin/env -S deno run --allow-env --allow-read
// ⬆️ New 🎉

// old:
// #!/usr/bin/env -S deno run --allow-env --allow-read --allow-write  --allow-net

import $ from "https://raw.githubusercontent.com/NfNitLoop/dax/0697afdbee91a866288214327fad04cc63a57b67/mod.ts"

await $`pwd`.printCommand()

@NfNitLoop
Copy link
Contributor Author

Not all tests pass on Windows, but that seems to have been the case for me before my changes as well.

@dsherret dsherret changed the title Don't require --allow-net fix: don't require --allow-net Aug 5, 2023
Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! I verified the benchmarks you provided and it is indeed faster, which is interesting.

@dsherret dsherret merged commit b9397a5 into dsherret:main Aug 5, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants