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

feat: adds the strict resolver as an option #837

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

gabrielcsapo
Copy link

@gabrielcsapo gabrielcsapo commented Dec 19, 2022

Summary

Originally from https://github.com/stefanpenner/ember-strict-resolver we have been using this in production for a few years! It is faster and less prone to errors as it doesn't allow configuration.

It would be great to support this as a first class resolver.

Details

When debugging how often we call into the resolver to access values from the registry, on an initial page load of linkedin feed we access the registry ~11,800 times.

When running linked.com feed using the ember-strict resolver we see in the experiment over ~300ms faster The differences are:

boot phase estimated difference -188ms [-195ms to -181ms]
transition phase estimated difference -42ms [-57ms to -27ms]
render phase estimated difference -23ms [-37ms to -9ms]

5fHNH_thW0KtzX6AR8t5hwkAOoyIOUzfOJxJO9OiOXpJBFDSwGFQg0R65TWVmmrG1KBzJARzn8u6f4WdXAIG12IkV0nKy5PyXEzEHrvyL22UXA6W5W3NZOYPKlb1R9B76Ve7ekFYu2_GVGvN-SS_gr0GNjSC5IfpOALIgShqw2K27qKsCrNseU0LmM_y

Shoutout to @stefanpenner for making this library and making it all possible!

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

First-pass review on just the README changes; I'll get to the actual implementation bit (which I assume is just inlined directly from the existing package, so I don't expect to want changes there!) later today or tomorrow!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 49 to 56
_For additional improvements when fully using the ember-strict-resolver monkey patching the registry to no longer cache and simply returning the values passed like the following can be produce extra performance._

```js
// disable the normalization cache as we no longer normalize, the cache has become a bottle neck.
Ember.Registry.prototype.normalize = function (i) {
return i;
};
```
Copy link
Contributor

Choose a reason for hiding this comment

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

However, I have a broader comment, which is: if we think this is worth doing, we should build a public API for it. It's one thing to do this as an experiment in our app (a successful one, I might add!) but we should not recommend others do it until there is a public API, or else we're setting people up for pain.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
gabrielcsapo and others added 8 commits December 19, 2022 09:03
Co-authored-by: Chris Krycho <[email protected]>
Co-authored-by: Chris Krycho <[email protected]>
Co-authored-by: Chris Krycho <[email protected]>
Co-authored-by: Chris Krycho <[email protected]>
Co-authored-by: Chris Krycho <[email protected]>
Co-authored-by: Chris Krycho <[email protected]>
Co-authored-by: Chris Krycho <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants