[BUGFIX release] Simplify mixin application #19082
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krisselden
reviewed
Aug 7, 2020
| import('./decorator').Decorator, | ||
| import('./decorator').ComputedDescriptor | boolean | ||
| > = new WeakMap(); | ||
| const DECORATOR_DESCRIPTOR_MAP: WeakMap<Function, unknown | boolean> = new WeakMap(); |
There was a problem hiding this comment.
unknown | boolean is the same as unknown
Simplifies mixin application in a number of ways: - Ensures that we only access `meta` once for a given object when applying mixins - Ensures that we only `peekDescriptors` once for a given descriptor - Minimizes the number of if/else branches and defaulting in general - Breaks apart `defineProperty` so that we can do less work per definition when mixing in mixins, since we know more about what possible operations will occur. - Removes extra brand checks from `defineProperty` (Array.isArray) so we don't penalize every defineProperty for doing that. - Only revalidate observers once per mixin application. - Only brand check `didDefineProperty` once per mixin application. - Replace `for..in` loops with `Object.keys` since we only care about own properties. - Only loop once in `extractAccessors`. - Combine observer and listener meta into a single object so we only need to do one lookup. In most cases, this is undefined in modern applications, so no extra memory cost. - Simplify CP descriptor property lookups (remove duplicate functions).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplifies mixin application in a number of ways:
metaonce for a given object whenapplying mixins
peekDescriptorsonce for a given descriptordefinePropertyso that we can do less work perdefinition when mixing in mixins, since we know more about what
possible operations will occur.
defineProperty(Array.isArray) sowe don't penalize every defineProperty for doing that.
didDefinePropertyonce per mixin application.for..inloops withObject.keyssince we only care aboutown properties.
extractAccessors.need to do one lookup. In most cases, this is undefined in modern
applications, so no extra memory cost.
Benchmark results:
artifact-60.pdf