Performance improvement for exports.getType()

[Tavriets]

Hy there,

While investigating performance issues in my application I found a place in knockout mapping plugin which could be improved slightly from both performance and memory allocation perspectives. It is exports.getType(). In fact it is quick enough but since this function called on the bottom of the mapping recursion call stack, the number of invocations can reach tens of thousands which makes a difference. My suggestion is to replace existing code:

exports.getType = function(x) {
    if ((x) && (typeof (x) === "object")) {
        if (x.constructor == (new Date).constructor) return "date";
        if (Object.prototype.toString.call(x) === "[object Array]") return "array";
    }
    return typeof x;
}

with something like:

exports.getType = function(x) {
    if (x && typeof (x) === "object") {
        if (x.constructor == Array) return "array";
        if (x.constructor == Date) return "date";
    }
    return typeof (x);
}

I didn't spot any difference in returning values in both functions (tried IE6-9, FF 18, Chrome 24, Opera12.01, Safari 5.1.7). Besides simplified code it also perform less object allocations and accordingly causes less garbage collections and thus makes better whole page performance (especially significant in single page apps).

I have created basic performance test for that: http://jsperf.com/gettype-test. It emulates almost the same number of invocations for 2 to and from mappings of my view model. Under that stress test the difference is 10-35% in different browsers. Perhaps, the suggested code could be improved further though.

Cheers, Tavriets

[sagacity]

Thanks, I committed it: acd5ea49b4c062949de25143162f031ff98c87da.

[illinar]

This change has introduced an unwelcome side-effect. The test constructor === Array fails in the situation when array was created in an iframe. See the relevant discussion on stackoverflow and in-depth details here.

The situation where it becomes a problem is knockoutjs + SignalR + Internet Explorer. SignalR (or similar technology) resorts to an iframe-based method to receive the data. If you supply the data as-is to ko.mapping.fromJS(...) you would miss array mapping entirely. The mapping still works since array is parsed as a composite object with properties 0, 1, 2, etc., but key/create/update options no longer apply.

Regards, Oleg

[sagacity]

Hmm, that's unfortunate. I don't think there would be any objections if this change is reverted?

[jhandby]

It'd be great if someone could revert this change. I've just had to back out of using knockout.mapping in a project at the last minute because foreach wasn't working in IE. (I'm using exactly the combination that illinar describes: knockoutjs + SignalR + Internet Explorer.) It took me a while to track this down!