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

Do not slice classNames and classNameBindings speculatively. #14389

Merged
merged 3 commits into from
Oct 27, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 30, 2016

The vast majority of the time these arrays are completely static on the prototype, there are relatively few instances where we actually need to slice them but we were speculatively doing this "just in case" you wanted it.

This change does not remove the ability to have custom classNames / classNameBindings per instance, but it does make it so that folks wanting to do this would need to do this.classNames.slice() first.

This avoids many extraneous array allocations, and one more layer of wasted work during initial render.

The vast majority of the time these arrays are completely static on the
prototype, there are relatively few instances where we actually need to
slice them but we were speculatively doing this "just in case" you
wanted it.

This change does not remove the ability to have custom `classNames` /
`classNameBindings` per instance, but it does make it so that folks
wanting to do this would need to do `this.classNames.slice()` first.

This avoids many extraneous array allocations, and just one more layer
of wasted work during initial render.
@chancancode
Copy link
Member

Do we think we can get away with this? I suspect it would probably be considered a breaking change since the slice was added a while ago in response to this use case. If we think it's okay to make this change I would be happy to see this go though.

However, if we can't, I have an alternative that I am working on. My plan is that we make these a getter and try to detect access them during construction (with some POST_INIT trick), and if they are accessed, we assume they are being mutated and returned a sliced version (only during init).

I plan to apply this to a few things such as per-instance tagName, etc to emit optimized component templates. If we detect that any of these features are used, we can set a flag on the class indicating the component is unoptimizable and fallback to the generic template we have today.

If that works out, we might be able to get the same performance improvements without the breaking change.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 30, 2016

@chancancode - Thank you for reviewing! Yes, I agree that this is a tad bit tricky, but I still think this is something that we can do. No capabilities are removed by this PR (the only use cases I have seen of this are already .slice() ing), this was not documented functionality (mutating during init) and the pattern used is generally something you should avoid in JS (mutating arrays on the prototype from within an instance).

We have done this sort of thing before, so there is precedent. For example, we made helpers params / hash frozen objects during the 2.9 cycle for similar optimizations.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 30, 2016

Updated to ensure that concatenatedProperties are frozen arrays in debug mode.

Mutating objects on the prototype (which includes concatenated
properties) is not good. This changes the functionality of concatenated
properties to freeze the resulting array while in debug mode, to make it
clear that mutating them without slicing first is not good.
@rwjblue rwjblue merged commit cb9959b into emberjs:master Oct 27, 2016
@rwjblue rwjblue deleted the avoid-slicing-classNames branch October 27, 2016 15:32
bgentry added a commit to bgentry/ember-test-selectors that referenced this pull request Jan 24, 2017
As noted in the Ember v2.11 release notes, concatenated properties such
as attributeBindings are frozen in debug builds. This means we cannot
push directly onto the attributeBindings array without first copying it
via slice.

Without this change, users may see errors like this when the
`attributeBindings.push()` call is made:

Uncaught TypeError: Can't add property 2, object is not extensible

References:

emberjs/ember.js#14389
emberjs/ember.js#14601
bgentry added a commit to bgentry/ember-test-selectors that referenced this pull request Jan 24, 2017
As noted in the Ember v2.11 release notes, concatenated properties such
as attributeBindings are frozen in debug builds. This means we cannot
push directly onto the attributeBindings array without first copying it
via slice.

Without this change, users may see errors like this when the
`attributeBindings.push()` call is made:

Uncaught TypeError: Can't add property 2, object is not extensible

References:

emberjs/ember.js#14389
emberjs/ember.js#14601
bgentry added a commit to bgentry/ember-test-selectors that referenced this pull request Jan 24, 2017
As noted in the Ember v2.11 release notes, concatenated properties such
as attributeBindings are frozen in debug builds. This means we cannot
push directly onto the attributeBindings array without first copying it
via slice.

Without this change, users may see errors like this when the
`attributeBindings.push()` call is made:

Uncaught TypeError: Can't add property 2, object is not extensible

References:

emberjs/ember.js#14389
emberjs/ember.js#14601
Turbo87 pushed a commit to mainmatter/ember-test-selectors that referenced this pull request Jan 25, 2017
As noted in the Ember v2.11 release notes, concatenated properties such
as attributeBindings are frozen in debug builds. This means we cannot
push directly onto the attributeBindings array without first copying it
via slice.

Without this change, users may see errors like this when the
`attributeBindings.push()` call is made:

Uncaught TypeError: Can't add property 2, object is not extensible

References:

emberjs/ember.js#14389
emberjs/ember.js#14601
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