Faster computeds

[SteveSanderson]

Similar to #1840, this pull request improves the performance of ko.computed initialisation. It reduces the time taken to construct computed instances by about 50%, and perhaps more importantly, reduces the memory overhead to about half of what it was before.

Like #1840, the approach is mainly to factor out all the closure-captured function instances and put them on the prototype instead.

Review notes

This change touches (or will touch) nearly every line in ko.dependentObservable.js, because it involves factoring out most of the logic inside ko.computed. That makes it a bit hard to review - sorry about that - couldn't avoid it! To make it a bit easier to review, I've broken it down into more commits than would be normal. But let me know if you suspect I might have unintentionally changed any behavior. Specs are all still passing.

Compatibility

Like #1840, there should be no compatibility difference, except for scenarios where code does equality comparisons on methods (e.g., myComputed1.peek === myComputed2.peek will always be true now). If anyone objects to this, please let us know! (I think we would do it anyway though)

Not yet complete!

There's still quite a big, confusing mass of setup code relating to dispose handlers. I would like to clean this up further (both for perf, but mainly for readability) but am out of time for today. If anyone else wants to jump in here you'd be welcome :)

[brianmhunt]

Do we want to expose state as part of the public API?

If not, perhaps this should be exposed via a Symbol e.g. ko.computedStateSymbol = Symbol('computed_state').

[SteveSanderson]

Good point. It will be private-named by the minifier anyway, but I agree it would be better still to use a Symbol, equivalent to how we do for ko.observable.

[brianmhunt]

FWIW, it looks good to me. If you merge it with the gulp branch (PR #1360) then you should be able to run the tests on a testrunner across multiple platforms, just to be sure. (The tests will run on all platforms on commit with Travis-CI, or locally with gulp test:sauce --PLATFORM where platform is one of the groups in the config e.g. firefox, chrome, etc.). Let me know and I can do the merge too.

[mbest]

I'll take a detailed look at this when a get a chance (probably in a day or two).

[brianmhunt]

I've merged the tests into and run them on a few platforms.

✅ It looks like these performance patches have introduced no new errors.

There are however some outstanding errors that have been around for a bit:

PhantomJS 1.9.8 when used with jQuery

This looks to come from #1773

PhantomJS 1.9.8 (Linux 0.0.0) Parse HTML fragment should parse correctly FAILED

Expected [ ] to equal [ '' ].

PhantomJS 1.9.8 (Linux 0.0.0) Parse HTML fragment should parse world correctly FAILED

Expected [ ] to equal [ 'world' ].

PhantomJS 1.9.8 (Linux 0.0.0) Parse HTML fragment should parse foo correctly FAILED

Expected [ ] to equal [ 'foo' ].

Firefox

• Firefox 17-22 fail unit test #1367 still exhibits

IE 7 and 8

• has a number of issues that exhibited before this patch

[mbest]

I decided to do a bit of clean-up first and then tackle refactoring the disposal code. Overall, I'm really happy we're able to make this kind of improvement in Knockout.

[brianmhunt]

Incidentally, if we will be exposing the state via a Symbol, we may want to invest in a trivial construction function e.g. ko.utils.newSymbol(name) that mimics this:

knockout/src/subscribables/observable.js

Lines 1 to 2 in ac5f21b

	var shouldUseSymbol = !DEBUG && typeof Symbol === 'function';
	var latestValueSymbol = shouldUseSymbol ? Symbol('_latestValue') : '_latestValue';

i.e.

var shouldUseSymbol = !DEBUG && typeof Symbol === 'function';
ko.utils.newSymbol = function (name) {
  return shouldUseSymbol ? Symbol(name) : name;
}

[SteveSanderson]

@brianmhunt That's exactly what I was thinking, and in fact I just implemented that simultaneously with your post :) Pushed as 8c1e65d

[SteveSanderson]

The get part of getSymbolOrString may be misleading ... Perhaps create or new would be a better prefix than get

Totally agree! Great point. Renamed to createSymbolOrString.

I'm not so sure about the debugSymbols thing though - I would have thought that would make things less convenient during debugging, rather than more, as the symbols won't normally show up in the browser debug tools. Having it as a string-named property seems more convenient in debug mode, wouldn't you think? Am I missing something?

[brianmhunt]

Having it as a string-named property seems more convenient in debug mode, wouldn't you think? Am I missing something?

You're absolutely right about the ease-of-reference.

I was thinking ko.debugSymbols would be exposed in both debug and production builds, making some cases easier (possible) to debug where otherwise it would be virtually impossible.

In the debug builds though, I agree it would be better for the symbols to be strings — for the debugging tools.

– on ko.debugSymbols, I'm easy, and leave it to you. 😄

[SteveSanderson]

I've just pushed commit b5d5d6, which reduces ko.computed's evaluation overhead by about 40% on Chrome, and about 20-30% on Firefox. This is the final thing I had in mind to optimise, unless anyone has suggestions for any particularly simple big wins.

How I've achieved this is mainly by factoring out the evaluateImmediate into a series of static sub-functions. This reduces the number of anonymous function instances we create on each evaluation (to zero, unless I'm missing something) but more significantly it separates most of the evaluation logic from the nested try-finally blocks. I guess this allows browsers to run the code in a more aggressively optimised way (I've read in various places that try blocks make V8 stop using many of its optimisations for that function).

Some of these refactorings are a bit, well, unnatural. I tried only to restructure things in ways that either (a) improved readability or (b) make substantial measurable improvements to speed. Arguably it's good to split up evaluateImmediate like this anyway, as it was kind of huge, but the places I have split it up are based on measured perf impact more than anything else.

If anyone objects to this commit (b5d5d6) please speak up!

[SteveSanderson]

I was thinking ko.debugSymbols would be exposed in both debug and production builds

I'd strongly prefer to avoid exposing this in production builds. It's already possible to reach them when debugging a production build by using Object.getOwnSymbols(). If we add further APIs to make it even more easy to reach, it increases the likelihood that people will use them not just for debugging but also for messing with internal state, turning all kinds of previously-private mechanisms into quasi-public APIs :) Are you OK with that?

[brianmhunt]

messing with internal state, turning all kinds of previously-private mechanisms into quasi-public APIs :) Are you OK with that?

Totally.

[SteveSanderson]

BTW thanks @mbest for your tidy-up, and for figuring out all the disposal stuff!

This branch is ready to be merged as far as I'm concerned, and I'm hoping we can ship 3.4.0 very soon :)

[SteveSanderson]

Do these functions (computedDisposeDependencyCallback and computedBeginDependencyDetectionCallback) need to be anonymous?

They don't need to be anonymous, and I didn't really have any particular reason for defining them that way. I've pushed a further commit to give them proper names.

As for all the others (e.g., all the properties on computedFn) - that's a lot of functions and although I have no objection to putting names on them all, I'd prefer to open the floor to any other comments before going through and changing them all. I guess possible counter-arguments might be that it means duplicating the names (e.g., someFunction: function someFunction() { ... }, which is another (albeit trivial) thing to maintain, and they won't show up in production builds anyway. But I really don't feel strongly about this. The change may well be useful to some people.

[brianmhunt]

👍 Excellent @SteveSanderson & @mbest

I've merged faster-computeds into #1360 and the test results will be here: https://travis-ci.org/brianmhunt/knockout

[rniemeyer]

Nice work all 👍

[brianmhunt]

As for all the others (e.g., all the properties on computedFn) - that's a lot of functions and although I have no objection to putting names on them all, I'd prefer to open the floor to any other comments before going through and changing them all.

@SteveSanderson Yes, sorry – I didn't mean to suggest we do any wholesale changes to the naming conventions outside the scope of the issue at hand, just to keep it in mind for future. However for the two functions identified, and you've since updated, it seems like the naming convention may be valuable.

[AdamWillden]

Just wanted to chime in and report the fantastic speed increase I can see in my app testing with these improvements. It is certainly noticeable. Great work and thank you 👍 roll on 3.4.0

[mbest]

I made some minor adjustments to evaluateImmediate code. I think everything looks good now.

[mbest]

I reverted most of 7f77e8c because I realized that the compiler would merge the functions that way, eliminating the benefit of splitting it up in the first place.

[brianmhunt]

I have given the latest version a whirl and I can report massive speed improvements across the board, particularly with deferUpdates on. I am seeing reductions of processes that took 3-4 seconds down to under 250ms. That's pretty incredible. 🚀

[SteveSanderson]

Fantastic - it's merged then. Thanks @mbest for the code improvements and to everyone else for helping validate the design and checking the perf effects!