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

Add nix and upgrade node #67

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Add nix and upgrade node #67

merged 3 commits into from
Nov 9, 2023

Conversation

airhorns
Copy link
Contributor

@airhorns airhorns commented Nov 8, 2023

So we're all using the same version when comparing benchmarks. Regrettably, there's a huge performance regression on newer nodes! I am going to work on that next, but at least with a nix setup we're comparing apples to apples.

❯ node --version
v21.1.0
❯ p x bench/create-many-different-roots.ts

> @gadgetinc/[email protected] x /Users/airhorns/Code/mobx-quick-tree
> ts-node --transpile-only "bench/create-many-different-roots.ts"

instantiating a large root x 3,047 ops/sec ±1.32% (99 runs sampled)
instantiating a small root x 11,136,120 ops/sec ±0.47% (100 runs sampled)
instantiating a diverse root x 449,239 ops/sec ±0.16% (98 runs sampled)
❯ node --version
v20.9.0
❯ p x bench/create-many-different-roots.ts

> @gadgetinc/[email protected] x /Users/airhorns/Code/mobx-quick-tree
> ts-node --transpile-only "bench/create-many-different-roots.ts"

instantiating a large root x 3,243 ops/sec ±0.56% (98 runs sampled)
instantiating a small root x 11,026,548 ops/sec ±0.14% (98 runs sampled)
instantiating a diverse root x 461,517 ops/sec ±0.15% (100 runs sampled)
❯ node --version
v18.18.2
❯ p x bench/create-many-different-roots.ts
> @gadgetinc/[email protected] x /Users/airhorns/Code/mobx-quick-tree
> ts-node --transpile-only "bench/create-many-different-roots.ts"

instantiating a large root x 4,876 ops/sec ±0.56% (98 runs sampled)
instantiating a small root x 9,065,427 ops/sec ±0.89% (93 runs sampled)
instantiating a diverse root x 638,657 ops/sec ±1.81% (97 runs sampled)

@airhorns airhorns force-pushed the upgrade-node branch 2 times, most recently from b6ae2d5 to 411a446 Compare November 8, 2023 15:40
@airhorns airhorns marked this pull request as ready for review November 8, 2023 21:57
@airhorns airhorns changed the title upgrade node Add nix and upgrade node Nov 8, 2023
@airhorns airhorns requested a review from thegedge November 8, 2023 22:01
Copy link
Contributor

@thegedge thegedge left a comment

Choose a reason for hiding this comment

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

👍

Until we fix the regression, how do you feel about adding this to package.json:

  "engines" : { 
    "node" : "<20.0.0"
  }

@airhorns
Copy link
Contributor Author

airhorns commented Nov 9, 2023

Enh, I feel like it really does run on all of them and we know it is an issue, we'll have to fix it soon so I think we just shouldn't cut a release until its fixed or we give up?

@airhorns airhorns merged commit 8e7dd8c into main Nov 9, 2023
4 checks passed
@airhorns airhorns deleted the upgrade-node branch November 9, 2023 16:11
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