perf($rootScope) - removing queues from isolated scope instances

[jbedard]

d219a57: If I understand the use of scope['this'] correctly it can be replaced with a $parse keyword and avoid being set per scope instance. [EDIT: moved to https://github.com//pull/9105]

5b426f6: For scopes that don't use listeners (most?) this avoids creating the two JSON objects similar to $$watchers. [EDIT: removed from PR for now]

abdebc8 284782b: This seemed a little confusing before (imo) having the 3 queue objects on every root/isolated scope while only having one instance (per root). The way they were accessed was also a little random - sometimes it was this.$$queue, sometimes $rootScope.$$queue. The $$applyAsyncQueue was also created per root/isolated scope, only ever accessed on the $rootScope, but still had a unique instance per scope which was never used (unlike the other 2 queues which referenced the $rootScope queue instance). The ngBindSpec change was actually a buggy test throwing due to lack of a context.

All three of these just reduce the number of variables/objects initialized per scope which is pretty minor, but scopes are created quite often...

If anyone depends on scope['this'] outside of parsed expressions or depends on the moved $$ variables then all 3 changes could be considered breaking changes.

[caitp]

These are all separate issues, please open separate pull requests for each one.

First commit looks good to me, but is technically a breaking change --- please also add a test verifying that we don't shadow the this of child objects in the scope this way. I've confirmed this myself, but a test would be good.

First commit also needs a breaking change notice.

I don't have much of an opinion on the second item (haven't looked at it).

The third item is a bit problematic --- we lose the ability to inspect at runtime and during testing --- This possibly breaks $evalAsync semantics, too. Needs more in-depth review though.

Third commit also needs a breaking change notice.

[jbedard]

Could still put the 3 queues on the $rootScope to allow inspecting at runtime and testing. Or could forget about the change, but maybe just make $$applyAsyncQueue consistent with the others. Other then not exposing the queues I don't think this breaks anything though, how is $evalAsync effected?

I'll add the test and create separate PRs for each in a bit...

[caitp]

$evalAsync may be affected because (and I can't recall), I believe we traverse scopes in tree-order, so putting them all in the same list would affect the order of execution.

I'm not positive on that anymore though because this section changes too frequently.

[caitp]

Removing tests that fail because of your changes is not good, either --- find a way to make the test work so that we know we didn't break anything important, otherwise add a new test verifying your refactor

[jbedard]

There was always only one queue though, so that hasn't changed. But now it is more clear there is only one queue...

[caitp]

One does not remove tests unless they are no longer valid --- the expected behaviour has not changed, so find a way to make the tests verify the expected behaviour

[jbedard]

Sorry I was replying to your previous comment. Regarding the test - yes I'll put it back, especially if we still want to expose the queues then the test can almost remain unchanged.

[lgalfaso]

d219a57 is a small breaking change, but if it makes a difference, then I think it is good

5b426f6 looks fine... I doubt that it makes any performance difference, but I can be wrong here

I think that some business logic is lost with abdebc8. This is, right now when calling $applyAsync on a destroyed scope, the task will not be scheduled (Note: we are not 100% consistent here). If this breaking change is acceptable and the removed test is rewritten in a way that it actually tests what is needed, then the change looks good

[jbedard]

@lgalfaso I see! Because post-destroy this.$$asyncQueue is a new empty array instead of the shared one...

I'd rather this didn't have any functional change. How about adding $applyAsync to the list of methods which get noop-ed on destroy? $apply is in that list so I think $applyAsync probably should be there anyway...

[caitp]

$$asyncQueue has nothing whatsoever to do with $applyAsync, fyi

[jbedard]

Right... so I take back what I said. How has $applyAsync functionality changed then?

[caitp]

$evalAsync may have changed (but I'm not positive). $applyAsync hasn't changed

[lgalfaso]

Sorry, my mistake, I mean to say $evalAsync

[jbedard]

I moved the first commit to #9105. Still thinking about the rest, I might stash 13d1a36 and just fixup cac0273 for now.

[jbedard]

I've removed 13d1a36 for now. This PR now only has the queue cleanup commit.

The queues are back onto the $rootScope for debugging/testing and I added the test back.
I added $evalAsync and $applyAsync to the methods that get noop-ed out on $destroy and moved that block up out of the "for V8 memory leak" section since the noop-ing was already depended on by the tests (for what seems to be for reasons other then V8).

[IgorMinar]

lgtm