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

Generate a monomorphic version of .createReadOnly and .instantiate for each class model #74

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

airhorns
Copy link
Contributor

@airhorns airhorns commented Nov 13, 2023

The VScode Deopt explorer showed that .createReadOnly inside the class model register function was getting deoptimized because it closes over too-wide a variety of klass variable references. We're closing over it so it is correct, but to v8, the callsite is still the same I guess, so it has to deoptimize to pull the right value for the closure! Same for .is and .instantiate -- these dynamically produced functions make a huge difference if they are re-eval'd for each class instead of defined dynamically.

CleanShot 2023-11-12 at 22 57 15

Removing this deoptimization recovered a lot of the node 18 => node 21 performance gap for me! Not all of it. It also makes node 18 faster, but not by nearly as much.

On main on node 18

instantiating a small root x 3,105,456 ops/sec ±0.15% (100 runs sampled)
instantiating a large root x 6,289 ops/sec ±0.68% (99 runs sampled)
instantiating a large union x 803,839 ops/sec ±1.33% (98 runs sampled)
instantiating a diverse root x 881,149 ops/sec ±1.35% (99 runs sampled)

On main on node 21

instantiating a small root x 1,942,360 ops/sec ±0.75% (94 runs sampled)
instantiating a large root x 3,725 ops/sec ±1.61% (93 runs sampled)
instantiating a large union x 468,432 ops/sec ±1.57% (95 runs sampled)
instantiating a diverse root x 604,151 ops/sec ±0.19% (96 runs sampled)

On branch on node 18

instantiating a small root x 3,047,781 ops/sec ±0.30% (95 runs sampled)
instantiating a large root x 6,842 ops/sec ±2.48% (96 runs sampled)
instantiating a large union x 1,085,365 ops/sec ±1.45% (99 runs sampled)
instantiating a diverse root x 983,455 ops/sec ±1.39% (100 runs sampled)

On branch on node 21

instantiating a small root x 1,904,508 ops/sec ±0.52% (99 runs sampled)
instantiating a large root x 5,247 ops/sec ±0.29% (98 runs sampled)
instantiating a large union x 523,979 ops/sec ±1.31% (95 runs sampled)
instantiating a diverse root x 1,040,574 ops/sec ±1.07% (101 runs sampled)

@airhorns airhorns force-pushed the faster-create-read-only branch from 5f37e08 to 1c75760 Compare November 13, 2023 04:03
@airhorns airhorns changed the title Generate a monomorphic version .createReadOnly for each class model Generate a monomorphic version of .createReadOnly and .instantiate for each class model Nov 13, 2023
@airhorns airhorns force-pushed the faster-create-read-only branch from 1c75760 to 1c75b10 Compare November 13, 2023 04:16
@airhorns airhorns marked this pull request as ready for review November 13, 2023 04:21
@airhorns airhorns requested a review from thegedge November 13, 2023 04:22
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.

🚀

…is for each class model

The VScode Deopt explorer showed that `.createReadOnly` inside the class model `register` function was getting deoptimized because it closes over too-wide a variety of `klass` variable references. We're closing over it so it is correct, but to v8, the callsite is still the same I guess, so it has to deoptimize to pull the right value for the closure! Same for .is and .instantiate -- these dynamically produced functions make a huge difference if they are re-eval'd for each class instead of defined dynamically.

Removing this deoptimization almost 2x'd to the large root benchmark for me!
@airhorns airhorns force-pushed the faster-create-read-only branch from 1c75b10 to 87b5ceb Compare November 13, 2023 15:14
@airhorns airhorns merged commit 87b5ceb into main Nov 13, 2023
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