search

emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

2.9.0-beta.3: Inexplicable subtree modifications #14332

Closed Turbo87 closed 8 years ago

Turbo87 commented 8 years ago

Reproduction: https://ember-twiddle.com/1b7c9053d07a483a0b70ef25f4afb1bd

Open the "Elements" tab of the Chrome DevTools, and look for the <div> surrounding the xxx token. With v2.9.0-beta.3 (and the other v2.9.0 betas) a subtree modification is triggered every second, while it works fine for v2.8.0.

Unfortunately I was not able to isolate the issue any further than this yet. Note that it seems to work fine if everything is inside the application controller/template/route, it only fails for subroutes like index or anything else.

Turbo87 commented 8 years ago

/cc @rwjblue @chancancode

chancancode commented 8 years ago

@Turbo87 I will fix this, but can you help me understand the issues that it is causing for your app, so I know how to determine if I have addressed the issue? i.e. is this causing noticeable performance problems, or other undesirable side-effects, etc?

Turbo87 commented 8 years ago

I haven't noticed any significant performance issues, but I haven't really measured either. I just noticed it in the DevTools and when I attached a subtree modification breakpoint I saw that it was constantly adding and removing a #comment node. I was a bit worried about it because it happened right in the <div> where I have a rather large D3 graph.

Turbo87 commented 8 years ago

I've been able to slim the reproduction down a little bit more: https://gist.github.com/Turbo87/1b7c9053d07a483a0b70ef25f4afb1bd/revisions

Turbo87 commented 8 years ago

awesome, thanks everyone!

chancancode commented 8 years ago

@Turbo87 I think this fixed it for the most important case where we unnecessarily reiterate. For the cases where we actually need to reiterate, we still unconditionally insert and remove the marker comment: https://github.com/tildeio/glimmer/blob/master/packages/glimmer-runtime/lib/vm/update.ts#L318-L319.

In that case, I doubt that this would cause any performance issues relative to the work we already have to do to reiterate. However, it's also relatively easy to make that lazy. The point of the marker comment is to mark the "end" of the list, so when we need to move or insert something "at the end", we have a stable marker for insertBefore, which means that we don't actually need it until we know we need to insert/move anything.

Again, I am personally unconvinced that it would make any difference, but if you or someone else wants to experiment with making that lazy, we can measure the before/after and see if it's worth the little bit of extra complexity. (If you or someone else noticed actual problems associated with this we can definitely fix it.)

Turbo87 commented 8 years ago

I'm totally fine with adjusting the marker comment once the each source changes. I was just confused that it was doing that even when the array was stable, but looking at the commit linked above that should be fixed now. I'll test the next beta and let you know in case the bug still exists.