-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 beta] Set models array length in link-to. #12546
Conversation
LGTM |
@@ -769,7 +769,7 @@ let LinkComponent = EmberComponent.extend({ | |||
this.set('queryParams', queryParams); | |||
|
|||
// 4. Any remaining indices (excepting `targetRouteName` at 0) are `models`. | |||
let models = []; | |||
let models = new Array(params.length - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if params are empty, we may want to skip allocating and later traversing at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, agreed
Once that last issue is fixed, can you prefix with |
It would be nice if provided perfs had demonstrable improvements in ember. An example or benchmark illustrating this improvement would be nice. |
@stefanpenner agreed, often micro benchmarks like this have no noticeable impact on a macro benchmark due to the improved code being a small percentage of the macro case. I'm fine with merging but am curious if this has any impact on a benchmark of list with link-to components. |
We know the array length in advance so we should set it. Proof: http://jsperf.com/literal-vs-new-23/9
3b880e9
to
7141e35
Compare
In emberperf it is about 2% faster across multiple runs, presumably by just pulling it out into a function which can be optimized separately, but that doesn't exercise the models use case where we should actually get our performance win. Tests pass now, tagged correctly. Ember Version: link-to modifications
Ember Version: Canary
|
Thanks @nathanhammond :-) |
[BUGFIX beta] Set models array length in link-to.
We know the array length in advance so we should set it.
Proof:
http://jsperf.com/array-push-new-literal