Skip to content

Update all dependencies + fix ensureRegistered bug #84

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

Merged
merged 7 commits into from
May 13, 2024
Merged

Conversation

thegedge
Copy link
Contributor

@thegedge thegedge commented May 9, 2024

I packed a bunch of stuff into one PR, but split it up into separate commits:

  • Bumped most dependencies
    • Not bumping deep-freeze-es6, which jest is unhappy with for any version >= 2.
    • Not bumping eslint, because v9 isn't backwards compatible. Best to wait a few months
      for it to settle and the plugin ecosystem to support v9.
    • Fixed any linting issues due to bumping prettier/TS
  • We were always checking the type argument passed to ensureRegistered for registration, and not each part of the prototype chain.
  • Increases initCount in our benchmarks so that we don't get any unfortunate issues where a benchmark has some JITed and non-JITed samples. Not sure if the value is high enough to ensure things will be JIT-ed, but I figure it's still an improvement.

TODO

  • Test out this branch in the Gadget repo (but opening for review early)
  • Follow up with a separate PR to bump to 0.7.0

@thegedge thegedge changed the title Update all dependencies Update all dependencies + fix ensureRegistered bug May 9, 2024
Copy link

codspeed-hq bot commented May 9, 2024

CodSpeed Performance Report

Merging #84 will not alter performance

Comparing bump-mqt (3b65711) with main (5f3020f)

🎉 Hooray! codspeed-node just leveled up to 3.1.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 8 untouched benchmarks

@thegedge
Copy link
Contributor Author

thegedge commented May 9, 2024

Oof, that's a rough regression. Lemme see if it's an easy fix, or if we don't bump.

[EDIT]
Not observing the same regression locally, except maybe on the "diverse root" benchmark (but it's not as bad as above)

[EDIT 2]
Looks like it could just be super high variance, which could be related to the JIT. I increased the amount of warmup runs to hopefully deal with such an issue. It may also be related to the bump in codspeed-node, which says that the latest version may potentially come with a regression.

@thegedge thegedge force-pushed the bump-mqt branch 4 times, most recently from d500123 to 62fe068 Compare May 9, 2024 17:31
@thegedge thegedge requested a review from airhorns May 9, 2024 17:40
@thegedge thegedge marked this pull request as ready for review May 9, 2024 17:40
@thegedge
Copy link
Contributor Author

thegedge commented May 9, 2024

Debugging some issue with the fast instantiator:

CleanShot 2024-05-09 at 13 48 34@2x

Coming from InstantiatorBuilder.build

thegedge and others added 6 commits May 9, 2024 14:46
Except two:

1. deep-freeze-es6, which jest is unhappy with for any version >= 2.
1. eslint, which broke a lot of things in v9.
This values is the number of times to run a benchmark before recording values.
It defaults to 1, but we want to have it be a high enough number to be more
likely to warm up the JIT so that benchmarks will likely be a bit fairer across
branches.

I'm not sure what a good value is, so this is very "finger in the wind".
@thegedge
Copy link
Contributor Author

thegedge commented May 9, 2024

☝️ rebasing to test the new benchmarks against mobx-state-tree directly, just to make sure any bumps to MST or mobx itself doesn't regress too much.

@thegedge
Copy link
Contributor Author

thegedge commented May 9, 2024

CleanShot 2024-05-09 at 15 00 13@2x

I don't think those should have changed, so I'm going to amend the last commit and have it run again (edit: ✅)

@thegedge
Copy link
Contributor Author

We had to patch out this change from the MST release in Gadget core, but that's because we do a ton of work with patches directly. All fixable, but that allows us to make progress more quickly. I think it's worth keeping these up to date though, so I'll leave the version bumps in!

@thegedge thegedge merged commit ebb9e0f into main May 13, 2024
@thegedge thegedge deleted the bump-mqt branch May 13, 2024 17:42
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