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 various performance improvements #65

Merged
merged 8 commits into from
Nov 8, 2023
Merged

Add various performance improvements #65

merged 8 commits into from
Nov 8, 2023

Conversation

thegedge
Copy link
Contributor

@thegedge thegedge commented Nov 7, 2023

Each commit shows the specifics, but to summarize:

Don't use for .. of in some hot paths

https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array

Nowadays, the performance of both for-of and forEach is on par with the old-fashioned for loop.

But I don't observe this at all (that blog post is from 2017).

More direct instantiation in our fast instantiator

  • types.frozen can be directly assigned. There's technically a typeof check for functions, but I'm okay ignoring that for read-only roots (we can typecheck it out too, if desired)
  • If an array's child type is directly assignable, the input snapshot will be exactly the format we need it.

Simplify setting parent/env

We had some helpers to set the special parent/env values. These actually had to do a bunch of extra work when the caller had deeper knowledge about the objects being provided to these functions. I removed these functions in favour of all the different instantiators doing the necessary work.

One issue that arose from this change was that these properties are not always tagged as non-enumerable. This is okay forthings like Object.keys and Object.entries, which focus on the "own" string keys, but can now be enumerated by getOwnPropertySymbols and getOwnPropertyDescriptors. Jest appears to do something deeper for toEqual checks and these were getting pulled in. To circumvent this, I made array snapshots be proper arrays, not quick arrays.

✅ Speed
✅ Verified this doesn't break the main Gadget repo
✅ Tests passing

Results

# Before
instantiating a small root x 1,698,169 ops/sec ±0.64% (98 runs sampled)
instantiating a large root x 4,168 ops/sec ±2.22% (95 runs sampled)
instantiating a large union x 326,144 ops/sec ±1.17% (95 runs sampled)

# After
instantiating a small root x 3,230,733 ops/sec ±0.28% (97 runs sampled)
instantiating a large root x 4,697 ops/sec ±1.57% (96 runs sampled)
instantiating a large union x 357,445 ops/sec ±1.36% (96 runs sampled)

https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array
states that they should be equivalent these days, but my own experience is that
they perform much better than `for ... of` and `Object.entries` alternatives. A
simple microbenchmark shows `for .. of` to be ~20% slower, which also agrees
with the results below:

```
* Before
instantiating a small root x 1,640,220 ops/sec ±0.32% (96 runs sampled)
instantiating a large root x 4,154 ops/sec ±2.11% (96 runs sampled)
instantiating a large union x 327,827 ops/sec ±0.21% (98 runs sampled)

* After
instantiating a small root x 1,860,914 ops/sec ±0.13% (97 runs sampled)
instantiating a large root x 4,364 ops/sec ±1.43% (99 runs sampled)
instantiating a large union x 344,748 ops/sec ±0.17% (98 runs sampled)
```
@thegedge thegedge changed the title Add various perofrmance improvements Add various performance improvements Nov 7, 2023
@airhorns airhorns mentioned this pull request Nov 8, 2023
@thegedge thegedge marked this pull request as ready for review November 8, 2023 03:19
Copy link
Contributor

@airhorns airhorns left a comment

Choose a reason for hiding this comment

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

Presuming Gadget is green, :shipit:

@@ -2,8 +2,8 @@ import { types } from "../src";

test("can create an array of simple types", () => {
const arrayType = types.array(types.string);
expect(arrayType.createReadOnly()).toEqual([]);
expect(arrayType.createReadOnly(["a", "b"])).toEqual(["a", "b"]);
expect(arrayType.createReadOnly().toJSON()).toEqual([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, they're needed unfortunately :(

    expect(received).toEqual(expected) // deep equality

    Compared values serialize to the same structure.
    Printing internal object structure without calling `toJSON` instead.

    - Expected  - 1
    + Received  + 1

    - Array []
    + QuickArray []

Different errors, but it's what we did in map.spec.ts too. Jest's toEqual matcher is unfortunately too picky in this instance :(

Fortunately I only had to do something like this in one place in the gadget repo. I was worried it would have been way more.

@thegedge
Copy link
Contributor Author

thegedge commented Nov 8, 2023

@airhorns Gonna merge this one, and then let's get your PR in #66. I'll cut a new release after.

@thegedge thegedge merged commit 278d96a into main Nov 8, 2023
4 checks passed
@thegedge thegedge deleted the perf-improvements branch November 8, 2023 15:32
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