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

[BUGFIX release] Fix inlining of superWrapper. #13655

Merged
merged 1 commit into from
Jun 12, 2016
Merged

Conversation

krisselden
Copy link
Contributor

This fixes the inlining of superWrapper. The bug that copying the arguments was addressing has been fixed for long enough that more users are affected by fix than the fix benefits. This also fixes a deopt in the megamorphic load of toString, since we store metadata on functions we have many shapes of functions.

I also retested call vs apply branching in modern browsers and did not see any difference between the two.

@stefanpenner
Copy link
Member

bugfix release or beta?

@stefanpenner
Copy link
Member

This is functionally equivalent, and @krisselden has done research across a large number of mobile devices. It appears the devices this affected the most, are now in a minority, and the others are adversely affected by this.

@rwjblue I think this could be [Bugfix release] but I would settle for [Bugfix beta] if you feel it is appropriate.

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2016

I am happy with either here. We already have to do a 2.6.1 so [BUGFIX release] seems fine...

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2016

@krisselden - Can you remove the apply export from ember-metal/utils?

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2016

FWIW - I think that apply here was possibly the single largest issue with stepping through Ember app code...

@stefanpenner
Copy link
Member

FWIW - I think that apply here was possible the single largest issue with stepping through Ember app code...

have you met the run-loop ? :trollface: 🏃

This fixes the inlining of superWrapper.  The bug that copying the arguments was addressing has been fixed for long enough that more users are affected by fix than the fix benefits.  This also fixes a deopt in the megamorphic load of toString, since we store metadata on functions we have many shapes of functions.
@rwjblue rwjblue changed the title Fix inlining of superWrapper. [BUGFIX release] Fix inlining of superWrapper. Jun 12, 2016
@rwjblue rwjblue merged commit 6467153 into master Jun 12, 2016
@rwjblue rwjblue deleted the fix-super-inline branch June 12, 2016 19:29
@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2016

Thanks @krisselden!

@knownasilya
Copy link
Contributor

So beautiful! Thanks @krisselden and company!

rwjblue pushed a commit that referenced this pull request Sep 3, 2016
* Update tests to ensure that the global path must not be `undefined`
* Fix issue with `Ember._Renderer` export
* Remove `Ember.rewatch` export remnants (it was removed in #9323)
* Remove `Ember.apply` export remnants (it was removed in #13655)
* Remove `Ember.tryCatchFinally` export remnants (it was removed in #11547)
toddjordan pushed a commit to toddjordan/ember.js that referenced this pull request Sep 9, 2016
* Update tests to ensure that the global path must not be `undefined`
* Fix issue with `Ember._Renderer` export
* Remove `Ember.rewatch` export remnants (it was removed in emberjs#9323)
* Remove `Ember.apply` export remnants (it was removed in emberjs#13655)
* Remove `Ember.tryCatchFinally` export remnants (it was removed in emberjs#11547)
webark pushed a commit to webark/ember.js that referenced this pull request Oct 6, 2016
* Update tests to ensure that the global path must not be `undefined`
* Fix issue with `Ember._Renderer` export
* Remove `Ember.rewatch` export remnants (it was removed in emberjs#9323)
* Remove `Ember.apply` export remnants (it was removed in emberjs#13655)
* Remove `Ember.tryCatchFinally` export remnants (it was removed in emberjs#11547)
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