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

Modifying unpublished properties on a polymer element causes a lot of observe-js work #844

Closed
PolymerCommunityBot opened this issue Nov 4, 2014 · 6 comments

Comments

@PolymerCommunityBot
Copy link

Issue by kevinpschaaf
Thursday Sep 25, 2014 at 02:31 GMT
Originally opened as https://github.com/Polymer/polymer-dev/issues/101


Unexpected cost: Modifying any property on a polymer element (particularly an unobserved property) causes observe-js to do a ton of unexpected/extra work.

This was stumbled upon in the sample in the following gist, which was attempting to measure how often the browser sends scroll events while scrolling. observe-test-a.html appends text to this.deltas (instance property of list-test element) every scroll event, recording the time between scroll frames. Note this.deltas is not used by any bindings or otherwise explicitly published/observed. In observe-test-b.html, deltas is moved to a variable not on the element, and the observe-js work per event goes away, reducing the time between scroll events substantially (click the menu icon to see the deltas text in an overlay).

https://gist.github.com/kevinpschaaf/b12d38796be25a0ce5fe

@PolymerCommunityBot
Copy link
Author

Comment by jmesserly
Thursday Sep 25, 2014 at 03:10 GMT


Excellent reproduction!

So I think this is caused by an interaction between <template repeat> and polymer-expressions. PolymerExpressions implements scoping by chaining the prototype: https://github.com/Polymer/polymer-expressions/blob/master/src/polymer-expressions.js#L610, which in this case is the list-test element. That means every binding inside the template repeat could in theory depend on the element's properties (it doesn't, but the system doesn't know that).

This is going to be somewhat tricky, but I think we can come up with a solution somewhere across the observe-js/TemplateBinding/PolymerExpression layers. In principle, we should (somehow) be able to divide observed state into "inside the repeat" and "outside the repeat" and ensure we only create a single observer for each bit of "outside the repeat" state.

It would be harder to replicate this issue with raw TemplateBinding as the "default binding syntax" doesn't look into parent scopes. But one way to do it would be a shared prototype among data items you're repeating over. Then mutate something on that shared prototype. Not sure how common that would be, but it seems possibly less pressing than the current issue.

@PolymerCommunityBot
Copy link
Author

Comment by jmesserly
Thursday Sep 25, 2014 at 04:55 GMT


So one quick idea: there's an optimization to skip most of the work if the property doesn't match the root object: googlearchive/observe-js@b229ed5

... however it doesn't consider the prototype chain.

This wouldn't really fix the problem (you'd still get O(N) pings for a big repeat), but it would bail out faster so it might help in practice.

@PolymerCommunityBot
Copy link
Author

Comment by jmesserly
Thursday Sep 25, 2014 at 18:30 GMT


this CL seems to fix the problem for me: googlearchive/observe-js#74

@PolymerCommunityBot
Copy link
Author

Comment by jmesserly
Thursday Sep 25, 2014 at 18:41 GMT


after that change, we only call callback a small number of times, meaning the items aren't observing the root element anymore

@PolymerCommunityBot
Copy link
Author

Comment by jmesserly
Thursday Sep 25, 2014 at 18:53 GMT


btw, that fix wouldn't help cases like using a filter or a property from list-test in each item. For that we need something smarter. But maybe it helps in some common cases.

@tjsavage tjsavage added the 0.5 label May 21, 2015
@sorvell sorvell removed the p1 label Jun 25, 2015
@tjsavage
Copy link
Contributor

Closing this issue due to age and the release of version 1 of Polymer - please feel free to re-open if this is incorrect.

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

No branches or pull requests

3 participants