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

fix(Repeat): ignore mutation while processing instance changed #349

Merged
merged 1 commit into from Jun 4, 2018

Conversation

Projects
None yet
5 participants
@bigopon
Copy link
Member

commented May 19, 2018

This PR is for aurelia/framework#830

I tried to reproduce the broken behavior in test but failed.

@EisenbergEffect @jdanyow

@EisenbergEffect EisenbergEffect requested review from martingust and jdanyow May 20, 2018

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented May 20, 2018

@jdanyow @martingust This is a potentially critical fix to the repeat functionality in Aurelia. @bigopon has taken a pass at the fix but we need another pair of eyes on this, preferably someone who has worked on this code extensively before 😄 Would one/both of you be able to look at this PR? If you have any idea on how to get the tests to correctly produce the issue, that would be great also. Thanks!

@bigopon

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2018

Regardless of real source of this issue, I think it's good to have this patched. A simple demo (https://gist.run/?id=d6eef539a65f5ac55bcbeb656eb0f779) to show the differences between Repeat on raw array (without ignoreMutation https://github.com/aurelia/templating-resources/blob/master/src/repeat.js#L152-L157) and array after a value converter (thus have ignoreMutation (https://github.com/aurelia/templating-resources/blob/master/src/repeat.js#L162-L170)

@EisenbergEffect @jdanyow @fkleuver

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jun 2, 2018

I agree. It's a workaround, but that's better than a flat-out bug. It's probably a good idea to create a new issue for this (or keep the existing one open) so as to make it explicit that this still needs a better fix.

I'm currently working on a different approach altogether but it's a fairly large undertaking. It will take some more time (and I'm not even sure if it will end up working)

@EisenbergEffect EisenbergEffect merged commit b04e3df into aurelia:master Jun 4, 2018

2 checks passed

WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@bigopon bigopon deleted the bigopon:fix-repeat-mutation branch Jun 4, 2018

@bulldetektor

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

Any estimates on when we can expect a release with this fix?

@EisenbergEffect

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

Very soon. We are in the middle of a big release push across many of our libraries. This will come out in the new templating release either this weekend or next week. It will be an RC release at first.

@3cp

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@3cp

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

I think there is an opportunity of architecture improvement in aurelia-binding.

Currently all observations (property/collection/dirty/expression) queue micro tasks that directly calls subscribers (which renders updates to DOM).

This is not only inefficient (this.foo=1; this.foo=2; this.foo=3 will render <template>${foo}</template> three 3 times), but also prone to code that mutate model in micro tasks themself (which is the issue that this PR tried to solve).

I would propose that all observations queue micro tasks to register changes, not call subscribers directly.

For instance for property-observation.js

  call() {
    // ...
    // this.callSubscribers(newValue, oldValue);
    this.aureliaRenderOrSomething.registerChange(this, newValue, oldValue);
  }

The aureliaRenderOrSomething should merge multiple changes from same observable. For example, if foo init value is 0; this.foo=1; this.foo=2; this.foo=3 will only register one change, not three (newValue = 3, oldValue =0). Beware, this optimisation also merges mutations done by multiple micro tasks.

Merge for CollectionObservation is bit more troublesome but can be done.

When aureliaRenderOrSomething detects first registerd change, it queues a macro task to call subscribers (flush all changes to DOM, without fear of model mutation during rendering).

This not only improves performance, but also simplifies code (because rendering is processed with frozen model). Let me know what you think.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

This ^^^^^^
I was planning on implementing exactly that in vNext (thought about calling it ChangeQueue).
As for the merging that you're talking about, I already built something like that for ArrayObserver in vNext and it's working quite well: https://github.com/fkleuver/experiment/blob/binding-attempt-2/src/runtime/binding/observation/array-observer.ts
It's basically an incremental change tracker.
@huochunpeng If you want, I'd be more than happy to work together with you on this. There's a lot of ground to cover because it requires a fair amount of rewriting ;)

@bigopon

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

The aureliaRenderOrSomething should merge multiple changes from same observable. For example, if foo init value is 0; this.foo=1; this.foo=2; this.foo=3 will only register one change, not three (newValue = 3, oldValue =0). Beware, this optimisation also merges mutations done by multiple micro tasks.

I think this is the current behavior of Aurelia.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Correct. Observers set their status to this.queued = true when they have a change and they don't re-queue on consecutive changes if they're already queued.
Still a separate ChangeQueue seems like a good idea. I don't think DOM updates should be in the same task queue as everything else.

@3cp

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@fkleuver I am quite happy you do all the hard work :-) I got some idea but it does not necessarily mean I have enough motivation to drive through it.

@bigopon you sure? I will run some test tomorrow.

@3cp

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Right. So my understanding is falsy. Thx for clarifying.

Now I can recall where my memory went wrong.

When I worked on optimisation on computed property, I noticed if you use the same property 3 times in view template, the getter will be called 3+1=4 times for one rendering. I was expecting an optimised single getter call for rendering.

@fkleuver

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

I am quite happy you do all the hard work :-) I got some idea but it does not necessarily mean I have enough motivation to drive through it.

Would love to hear your ideas :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.