Skip to content

Conversation

@rwjblue
Copy link
Member

@rwjblue rwjblue commented Jul 2, 2016

Shaping the prototype is not enough, we should ensure the instance properties are set in the constructor.

@rwjblue rwjblue force-pushed the ensure-shaping-for-destroyed-checks branch from 85a5e91 to 38eaf84 Compare July 2, 2016 05:19
Shaping the prototype is not enough, we should ensure the instance
properties are set in the constructor.
@rwjblue rwjblue force-pushed the ensure-shaping-for-destroyed-checks branch from 38eaf84 to f85dca2 Compare July 2, 2016 05:52
@mixonic
Copy link
Member

mixonic commented Aug 21, 2016

@rwjblue is this something we should be benchmarking to show improvement? Or are you otherwise just waiting on review from a perf team member (I expect @krisselden or @stefanpenner )?

@rwjblue
Copy link
Member Author

rwjblue commented Aug 21, 2016

Yeah, this is still a bit WIP (I'll update it as such).

@rwjblue rwjblue changed the title Ensure correct shaping for isDestroy[ing|ed] on CoreObject. [WIP] Ensure correct shaping for isDestroy[ing|ed] on CoreObject. Aug 21, 2016
@stefanpenner
Copy link
Member

@rwjblue is this something we should be benchmarking to show improvement?

This is most likely to small a change to notice much.

  • today i do not believe those properties are nonEnumerable once set
  • setting them via the nonEnumerable path is most likely going to be slower during construction. Is it possible for us to just set them as a property or ideally, move them to meta?

@homu
Copy link
Contributor

homu commented Sep 5, 2016

☔ The latest upstream changes (presumably #14215) made this pull request unmergeable. Please resolve the merge conflicts.

@krisselden
Copy link
Contributor

@stefanpenner the states should be moved to meta, and we should have accessors on the prototype that defer to meta for backwards compatibility.

I agree that this PR is a step backwards and that it is dubious to test non enumerability since they are enumerable once set currently.

@stefanpenner
Copy link
Member

@stefanpenner the states should be moved to meta, and we should have accessors on the prototype that defer to meta for backwards compatibility.

I agree that this PR is a step backwards and that it is dubious to test non enumerability since they are enumerable once set currently.

yes

@rwjblue
Copy link
Member Author

rwjblue commented Sep 27, 2016

Completely replaced by #14359

@rwjblue rwjblue closed this Sep 27, 2016
@rwjblue rwjblue deleted the ensure-shaping-for-destroyed-checks branch September 27, 2016 21:58
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.

5 participants