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

Cleanup Meta Descs #12852

Closed
wants to merge 1 commit into from
Closed

Cleanup Meta Descs #12852

wants to merge 1 commit into from

Conversation

stefanpenner
Copy link
Member

  • make it work, run a normal app
  • rebase
  • fix last 9 failing tests
  • cleanup LOLcode
  • more stringent perf analysis + fixes
  • decide how to handle setters

@stefanpenner stefanpenner changed the title WIP Cleanup Meta Descs Jan 22, 2016
export function defineProperty(obj, keyName, desc, data, meta) {
var possibleDesc, existingDesc, watching, value;
export function defineProperty(obj, keyName, desc, data/*, meta*/) {
let meta = arguments[4] || metaFor(obj);
Copy link
Member Author

Choose a reason for hiding this comment

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

a meta may not exists here yet, we should be careful to not create on if it does not need to exist.

@mmun
Copy link
Member

mmun commented Feb 7, 2016

@stefanpenner Is it possible to separate this into two PRs? a) undoing #10323 and b) getter and setter support? It looks like you've done the work for a) so we can land it asap? I'm happy to clean up the commit with your approval.

@stefanpenner
Copy link
Member Author

Ya it's possible, as I was going through this code I realized many things required work-arounds and without a more full exploration, which work-around is more appropriate is tricky. Some performance considerations became obvious as well. So my goal here is to complete the whole exercise, get a full picture of the performance work, to allow my to see where the pieces fall then work backwards.

Downside is ^ will take some time, and I have been pushed in another direction for the moment. I can try later this week to bring this to a close, or at-least split it up.

@mmun
Copy link
Member

mmun commented Feb 7, 2016

In chat, @stefanpenner suggests that we implementing descs-flattening-after-first-create before rather than using the existing "inheriting map" structure. Otherwise we will likely regress on the performance of all get and set calls.

@krisselden krisselden force-pushed the restore-meta-descs branch 12 times, most recently from ae8ad74 to 51cd45b Compare April 7, 2016 00:41
@homu
Copy link
Contributor

homu commented Apr 8, 2016

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

@krisselden krisselden force-pushed the restore-meta-descs branch from 51cd45b to 5128ae7 Compare April 8, 2016 22:28
@homu
Copy link
Contributor

homu commented Apr 14, 2016

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

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2016

@stefanpenner - thoughts on progressing this?

@stefanpenner
Copy link
Member Author

stefanpenner commented Jul 22, 2016

@rwjblue planning to resume later this summer, unless someone else jumps in.

@stefanpenner stefanpenner deleted the restore-meta-descs branch April 5, 2017 00:19
@stefanpenner stefanpenner restored the restore-meta-descs branch April 5, 2017 00:19
@stefanpenner stefanpenner deleted the restore-meta-descs branch April 5, 2017 00:19
@stefanpenner stefanpenner restored the restore-meta-descs branch April 5, 2017 00:19
@stefanpenner stefanpenner deleted the restore-meta-descs branch April 5, 2017 00:19
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.

4 participants