Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

improve performance for template repeat over model objects with shared prototype #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmesserly
Copy link
Contributor

This fixes the case presented in: https://github.com/Polymer/polymer-dev/issues/101

What is happening there is we have a bunch of paths inside a <template repeat> such as {{model.path}}. With polymer-expressions, the model for each repeated item has the same __proto__, which is the containing element, and we don't want every property set there to ping every item in the list. This seems to be the simplest thing that prevents the bug.

(Note: if the property is deleted from rootObj, we should get a change record from rootObj which causes us to iterate all objects again, and at that point, will observe the prototype because it is no longer an ownProperty of rootObj. That won't happen for polymer-expression scopes, but could happen in general.)

@jmesserly
Copy link
Contributor Author

I went ahead and rebased, so this CL can be tested without #73

@ajklein
Copy link
Contributor

ajklein commented Sep 25, 2014

@jmesserly, do you have an intuition about how often this will avoid the prototype walk? At first glance, it looks to me like it should be super-common, but then it comes to mind that published properties are only present on the prototype...

I ask because I've been investigating the overhead involved in prototype observation, in general.

@jmesserly
Copy link
Contributor Author

yeah, it wouldn't help for published props :(. The only case I know of that it fixes is the scopes created by PolymerExpressions+template repeat="{{model in data}}". In that case "model." is an own prop of each scope created for each repeated item. But at least in that example it reduces work per scroll to O(1) from O(N) where N==items in list.

On the other hand, if you used a filter or just a data property that came from the parent scope, you're out of luck :|

edit: clarity

@jmesserly
Copy link
Contributor Author

Hmmm, thinking on this more, it seems like we could do this check always and not just for the root object (assuming #73 is included). In other words, if we found the property on a particular object, we'll observe that object, so why walk up to the prototype at all?

@sorvell
Copy link

sorvell commented Sep 25, 2014

If this turns out to be a significant optimization, we should be able to take advantage of it for Polymer's published properties. The same optimization could apply since published property values are stored at a private location on the element instance, but we'd need more fanciness to know we can use this code path.

@jmesserly
Copy link
Contributor Author

This seemed to me like a helpful optimization, but waiting for an LGTM from someone before landing :)

// If we find the property on the root object, we can skip observing
// the prototype, which is expensive. This helps a common use case:
// when the user has <template repeat> over items that all share a
// prototype. See https://github.com/Polymer/polymer-dev/issues/101
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to handle the case where the own property gets deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, I should at least have a test for that & mention it the comment. It should work because whenever it gets a change record (in callback below), it will rescan everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think that will work since we'll run through this code path in that case again and object will no longer have the own-property -- but a test would be ideal.

@arv
Copy link
Contributor

arv commented Oct 7, 2014

LGTM

@rafaelw
Copy link
Contributor

rafaelw commented Oct 9, 2014

lgtm w/ comment above.

@rafaelw
Copy link
Contributor

rafaelw commented Oct 9, 2014

actually, can you wait to merge until i push my construction benchmarks. i want to make sure the call to hasOwnProperty doesn't regress construction.

@jmesserly
Copy link
Contributor Author

benchmarking sounds great to me. I could run it through https://github.com/Polymer/TemplateBinding/blob/master/benchmark/codereview-diff.html too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants