googlearchive / observe-js

A library for observing Arrays, Objects and PathValues
1.35k stars 118 forks source link

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

Open jmesserly opened 9 years ago

jmesserly commented 9 years ago

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 commented 9 years ago

I went ahead and rebased, so this CL can be tested without https://github.com/Polymer/observe-js/pull/73

ajklein commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

Hmmm, thinking on this more, it seems like we could do this check always and not just for the root object (assuming https://github.com/Polymer/observe-js/pull/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 commented 9 years ago

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 commented 9 years ago

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

arv commented 9 years ago

LGTM

rafaelw commented 9 years ago

lgtm w/ comment above.

rafaelw commented 9 years ago

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 commented 9 years ago

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