-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
During normalization, use property lookup instead of hasOwnProp checks #4313
During normalization, use property lookup instead of hasOwnProp checks #4313
Conversation
This most likely will, at the time I made the recommendation I had an active benchmark (in ember-data not just a micro one) but I have since mis-placed it. I can try finding it, or hopefully i'll have some time soon to revisit it, so we can double check this improvement. |
I tried to get this (old) benchmark working to see if it could be used to test this pr https://github.com/emberjs/data/tree/igor-benchmarks. After spending about an hour looking at it I was still unable to get it to run successfully. |
@chadhietala @asakusuma testing this in one your ED heavy apps might reveal something, specifically the call times of the methods in question. |
@stefanpenner you wanting to confirm a perf boost, or wanting to make sure this doesn't break something? @msranade is doing some perf testing right now. |
both, and especially the selfTime of the call-sites that have been updated. such as |
@minasmart or @asakusuma did you get a chance to pref test this pr? |
@bmac I'm confident in saying that this will be positive for performance. Latest v8 starts to optimize hOP, but this form is still faster. Most importantly all those older android devices will also benefit. |
Thanks @stefanpenner. @sly7-7 do you have time to rebase this pr? |
This pr has been committed as 03b34e2 |
@bmac I'm slowly recovering from a big burn out. I'm still not able to develop anything 😞 |
To follow #4311 and according to #2289 (comment), the server returning an undefined value is just invalid, I think we can now replace all those slowly hasOwnProp checks during the normalization process.
Seems like this would improve perfs :)
This will need #2289 to be merged before (the same tests where failing).
I will rebase after that
kindly ping to @stefanpenner @igorT @wecc @bmac