Memory pressure...

[mramato]

Almost all of our types have methods that create new instances, which leads to a ton of unnecessary throw away objects and increases memory pressure. For example, computing the sun position ends up creating several Cartesians every frame, when in reality it shouldn't need to create any (or at most 1). All of our functions that create new instances should allow for a result parameter to be specified as the last parameter which is the instance to store the result in. It should also allow for the result parameter to be the same as "this" so that you can do in-place modification.

For example:

Cartesian3.prototype.normalize()

becomes

Cartesian3.prototype.normalize(result)

If result is undefined, then we create a new instance. In either case, we return the result for easy chaining.

We should then revisit all of our existing code and make sure we are using private scratch objects and passing result parameters to eliminate all throw away objects.

I know we originally thought we might want static versions of all of our methods as well, but if we follow the above strategy, that won't be necessary. I've done some of this work on the viewer branch (the minimum necessary to make it performant) but I (or someone else) should do a full refactor everywhere after that branch comes in.

[pjcozzi]

The static versions are useful because user's don't need to allocate, for example, Cartesian3. Instead, they might have parsed JSON and just have an object literal with x, y, and z properties, or they have some other object that has x, y, and z properties and others but it still works with our math functions.

[mramato]

I understand the reasoning, and we can certainly have static versions of every method too if we think it's worth while. In my experience so far, however, I haven't needed the static versions of anything and it just seems like a lot of redundant code to have both. I think the static versions it the type of thing that sounds really useful, but in practice doesn't really add a lot of value (but adds a lot of code). I could be convinced otherwise though.

[mramato]

Another thought. If we're smart, we would implement the prototype versions in terms of the static versions to avoid duplicate code. However, this adds an extra method call to the prototype versions; which unless the compiler is doing some inlining would certainly make the prototype versions slower. Since the static versions would be faster, it would make sense to use them everywhere at that point, and then get rid of the prototype versions. Of course this is some conjecture on my part; if the compilers are fast enough to eliminate that extra method call, this point is moot.

[pjcozzi]

For Cesium users, they are very useful. I wanted them several times when dog-fooding. In fact, we decided that all public interfaces should take Cartesian3 or an object with x, y, and z properties, which is why many functions allocate a temporary Cartesian3. @kristiancalhoun made the change. This was a baby-step to static functions.

I'm not sure about the inlining. I also argued against static functions because it is double the amount of code to write, document, test, etc., but it is only for a handful of classes, so I suppose we can justify it. @shunter and I have debated this several times over the past six months or so.

[mramato]

Yeah, I've talked to @shunter a lot about it too. As I said, I have no problem having both, so here's my proposal.

1. All of our core types should have their last parameter be an optional result parameter as I defined above.
2. All of our core types should work whether the passed in object is an instance of the type, or just and object that has the same properties
3. We should have both static and prototype versions of all methods on core types.

Assuming we all agree on this, it's something I want to work on right after the viewer pull request is in.

[pjcozzi]

Supporting (2) sounds a bit painful, right? Sometimes there will be:

if (result) {
   if (result === this) {
      // ...
   }
   else {
      // ...
   }
}
else {
   // ...
}

Maybe that case is rare enough. Have you looked at other popular JavaScript vector and matrix libraries? If after looking at them, you still propose the above, I am OK with it.

[shunter]

The logic can be simplified a lot. At the start:

if (typeof result === 'undefined') 
  result = new Cartesian3();

Then, make all of the internal work within the methods functional instead of object-oriented, that is, use the static versions internally.

[mramato]

I haven't looked at the other libraries, but I'll check them out. I've already implemented my strategy in a few classes and it's not hard at all and works really well. For a simple example, see the new Spherical class in the viewer branch:

https://github.com/AnalyticalGraphicsInc/cesium/blob/viewer/Source/Core/Spherical.js

For something more complicated, I've also made some change to Quaternion, check out Quaternion.prototype.multiply. (there's no static version yet)

https://github.com/AnalyticalGraphicsInc/cesium/blob/viewer/Source/Core/Quaternion.js

I don't plan on doing any more modifications to the viewer branch, as my goal was to do the minimum needed for the branch. I'll do everything else on a new branch.

[pjcozzi]

@shunter Does that work all the time? The following fails when left or right equals result. It's a read after write hazard.

Cartesian3.prototype.cross = function(other, result) {
    if (typeof result === 'undefined') {
        result = new Cartesian3();
    }
    Cartesian3.cross(this, other, result);
    return result;
};

Cartesian3.cross = function(left, right, result) {
    result.x = left.y * right.z - left.z * right.y;
    result.y = left.z * right.x - left.x * right.z;
    result.z = left.x * right.y - left.y * right.x;
};

[mramato]

Your implementation of Cartesian3.cross is bad, it should do the property look ups first, this one works in your example. This is also probably a little faster (though negligibly so), since it eliminates multiple property look ups.

Cartesian3.cross = function(left, right, result) {
    var leftX = left.x
    var leftY = left.y
    var leftZ = left.z
    var rightX = right.x
    var rightY = right.y
    var rightZ = right.z

    result.x = leftY * rightZ - leftZ * rightY;
    result.y = leftZ * rightX - leftX * rightZ;
    result.z = leftX * rightY - leftY * rightX;
};

[pjcozzi]

Actually, my Cartesian3.cross implementation was by design. Maybe the copies are faster. I can't rationalize about JavaScript performance. In C++, the function would be inlined along with other functions and all those copies would result in lots of register pressure which decreases the amount of parallelism gained by out of order and super scalar execution. In JavaScript? No idea what code is generated. Is it enough to matter? Probably not. I'm sold.

[shunter]

Cartesian3.prototype.cross = function(other, result) {
    return cross(this, other, result);
};

var cross = Cartesian3.cross = function(left, right, result) {
    if (typeof result === 'undefined') {
        result = new Cartesian3();
    }

    var leftX = left.x;
    var leftY = left.y;
    var leftZ = left.z;
    var rightX = right.x;
    var rightY = right.y;
    var rightZ = right.z;

    result.x = leftY * rightZ - leftZ * rightY;
    result.y = leftZ * rightX - leftX * rightZ;
    result.z = leftX * rightY - leftY * rightX;

    return result;
};

[mramato]

And by "bad" I meant incorrect for its purpose, I didn't mean to call your code "bad".

[mramato]

Looks like me and @shunter implemented the same exact functions independently of each other. I'm going to name this "proof by duplication".

[shunter]

@mramato I based mine on yours, just to get it as concise I can. Sorry to invalidate your proof.

[pjcozzi]

Long story short, yes, I'm in full support of this, but, as I originally stated, you have to agree it is quite a bit more than our current code:

Cartesian3.prototype.cross = function(other) {
    return new Cartesian3(
            this.y * other.z - this.z * other.y,
            this.z * other.x - this.x * other.z,
            this.x * other.y - this.y * other.x);
};

[mramato]

@pjcozzi I definitely agree with you and I'm glad we've reached a consensus overall. I blame Javascript :)

@shunter Too bad. I was being lazy, so thanks for putting the whole thing together.

[pjcozzi]

Might be worth looking at these too - http://stepheneb.github.com/webgl-matrix-benchmarks/matrix_benchmark.html

[mramato]

Awesome, thanks. I had previously checked out some of these library and all of the fast ones followed patterns similar to what I laid out above, so I think we're on the right track. Looks like this site is available on github here: https://github.com/stepheneb/webgl-matrix-benchmarks It would be great for someone to add Cesium to the mix, this way we know how were stand (and what's possible).

[mramato]

I'm closing this issue. While memory pressure is still a problem in some areas of the code-base, as a team we are much more aware of them and fix them as we come across them. Core itself is in pretty good shape. There's no need to have an overly general issue like this.