Simple but effective performance improvements #622

Merged
merged 9 commits into from Apr 9, 2013

Conversation

Projects
None yet
2 participants
@mramato
Member

mramato commented Apr 6, 2013

Me and @shunter found some areas of the code that were causing major slow-downs and memory churn. There is still plenty of room for improvement, but these targeted changes help out a lot.

  1. Interpolation proved to be a huge part of memory churn, so this change adds result parameters for them and updated the one function using them to use result parameters. (NOTE: Hermite is still a problem but is not often used and needs a larger refactor that we'll do another time).
  2. DynamicProperties with constant, infinite values were doing way more work than necessary and now do almost no work. (fixes #411).
  3. There was another widget layout issue in Animation.js that was missed with #617
  4. There was a bug in VertextBufferFacade were it was recreating buffers every frame. This was because of a re-named variable that was never updated.
  5. Minor tweak to BoundingSphere where it should have been using a scratch parameter but wasn't.
  6. Improve billboard performance and memory churn. Rather than push billboards onto billboardsToUpdate and then throwing away the entire array every frame, this maintains a long-term billboardsToUpdate array and simply tracks the current index. This reduced GC churn but also turns the push into an index assignment, which is faster.
  7. DynamicObject.isAvailable had an obvious duplicate computation in it, which I removed.
  8. Apps that use mouse over picking would see performance issues when using a large number of billboards. This was the result of computeNewBuffersUsage being called during pick, which would result in all usages being set back to STATIC_DRAW and the VertexArrayFacade being recreated every time the mouse moved. This changes it so we don't call computeNewBuffersUsage on pick passes.
  9. Optimize usage of FrustomCommandList in the same way we do for billboards in 6, above.

To compare performance, I used Chrome's profiling tools using FAA.czml (available in the website Gallery directory). I found 1800x was a good speed for causing performance problems. Garbage collection goes down significantly, as does the amount of time spent in the above areas of the code.

mramato added some commits Apr 5, 2013

Unit tests for interpolation result parameters.
Also add a scratch variable for BoundingSphere.fromPoints, which was previously missing.
Improve billboard performance and memory churn.
Rather than push billboards onto billboardsToUpdate and then throwing away the entire array every frame, this maintains a long-term billboardsToUpdate array and simply tracks the current index.  This reduced GC churn but also turns the push into an index assignment, which is faster.
Update billboard buffer usage heuristics.
Apps that use mouse over picking would see performance issues when using a large number of billboards.  This was the result of computeNewBuffersUsage being called during pick, which would result in all usages being set back to STATIC_DRAW and the VertexArrayFacade being recreated every time the mouse moved.  This changes it so we don't call computeNewBuffersUsage on pick passes.

Also, we no longer reset usage properties ever pass, which could result in ping-ponging, instead, we only reset them when we are recreating the vertex for other reasons, such as billbarods being added/removed.
Don't zero out frustomCommandList every frame.
In Scene.js we were binning different commands into FrustomCommandLists
and then setting the array length to 0 every frame.  Instead, we now use
an index variable to track where we are in the commands and simply set that
to zero, reusing the previous array.
@mramato

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 8, 2013

Member

Can someone please review this? We're demoing Cesium heavily at the National Space Symposium and I would really like to show our best on the website.

Member

mramato commented Apr 8, 2013

Can someone please review this? We're demoing Cesium heavily at the National Space Symposium and I would really like to show our best on the website.

@@ -171,7 +172,7 @@ define([
maxBoxPt.y = yMax.y;
maxBoxPt.z = zMax.z;
- var naiveCenter = Cartesian3.multiplyByScalar(Cartesian3.add(minBoxPt, maxBoxPt, fromPointsScratch), 0.5);
+ var naiveCenter = Cartesian3.multiplyByScalar(Cartesian3.add(minBoxPt, maxBoxPt, fromPointsScratch), 0.5, fromPointsNaiveCenterScratch);

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

You could re-use fromPointsScratch scratch in place of fromPointsNaiveCenterScratch, no?

@kring

kring Apr 9, 2013

Member

You could re-use fromPointsScratch scratch in place of fromPointsNaiveCenterScratch, no?

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

That was my first inclination, but looking at the code I wasn't sure it would work. But I looked again and you may be right. I'll give it a try.

@mramato

mramato Apr 9, 2013

Member

That was my first inclination, but looking at the code I wasn't sure it would work. But I looked again and you may be right. I'll give it a try.

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

My original guess was correct. fromPointsNaiveCenterScratch is necessary because fromPointsScratch is used in the for loop below this code; the loop modifies fromPointsScratch but needs to reuse the same value for naiveCenter.

@mramato

mramato Apr 9, 2013

Member

My original guess was correct. fromPointsNaiveCenterScratch is necessary because fromPointsScratch is used in the for loop below this code; the loop modifies fromPointsScratch but needs to reuse the same value for naiveCenter.

+ var len;
+ var index;
+ var length = xTable.length;
+ var coefficients = new Array(yStride);

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

It would be good to eliminate this allocation, too, but somewhat tricky. Probably outside the scope of this pull request.

@kring

kring Apr 9, 2013

Member

It would be good to eliminate this allocation, too, but somewhat tricky. Probably outside the scope of this pull request.

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

Yeah, I mentioned this in the main pull request description. Hermite could use a full refactoring to improve allocations. Definitely outside the scope of this pull.

@mramato

mramato Apr 9, 2013

Member

Yeah, I mentioned this in the main pull request description. Hermite could use a full refactoring to improve allocations. Definitely outside the scope of this pull.

@@ -64,11 +64,14 @@ define(function() {
* @memberof LagrangePolynomialApproximation
*
*/
- LagrangePolynomialApproximation.interpolateOrderZero = function(x, xTable, yTable, yStride) {
+ LagrangePolynomialApproximation.interpolateOrderZero = function(x, xTable, yTable, yStride, result) {

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

I noticed while looking at this that the doc for this function is pretty wrong, as a result of evolution. It talks about a "specified degree" even though there isn't one. Up to you if you want to consider that part of this pull request.

@kring

kring Apr 9, 2013

Member

I noticed while looking at this that the doc for this function is pretty wrong, as a result of evolution. It talks about a "specified degree" even though there isn't one. Up to you if you want to consider that part of this pull request.

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

I might as well while I'm here.

@mramato

mramato Apr 9, 2013

Member

I might as well while I'm here.

Source/DynamicScene/DynamicProperty.js
@@ -228,13 +234,13 @@ define([
// Interpolate!
var x = times[lastIndex].getSecondsDifference(time);
- var interpolationResult = intervalData.interpolationAlgorithm.interpolateOrderZero(x, xTable, yTable, doublesPerInterpolationValue);
+ interpolationScrach = intervalData.interpolationAlgorithm.interpolateOrderZero(x, xTable, yTable, doublesPerInterpolationValue, interpolationScrach);

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

Scrach -> Scratch

@kring

kring Apr 9, 2013

Member

Scrach -> Scratch

Source/Scene/BillboardCollection.js
}
}
- for (var k = 0; k < NUMBER_OF_PROPERTIES; ++k) {
- properties[k] = 0;
+ //If the number of total billboards every shrinks considerably

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

every -> ever

@kring

kring Apr 9, 2013

Member

every -> ever

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

Also, don't we usually put a space after the //?

@kring

kring Apr 9, 2013

Member

Also, don't we usually put a space after the //?

@kring

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

Tests pass, CZML and Billboard Sandcastle apps look good. Just those typos left and then I'll merge this.

Member

kring commented Apr 9, 2013

Tests pass, CZML and Billboard Sandcastle apps look good. Just those typos left and then I'll merge this.

@mramato

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

All changes are in. The doc was pretty bad on all of the interpolators, so I simplified all three. That's not my strongest area, so hopefully I didn't screw anything up. Feel free to tweak it before merging (or come yell and me and I'll change it again).

Member

mramato commented Apr 9, 2013

All changes are in. The doc was pretty bad on all of the interpolators, so I simplified all three. That's not my strongest area, so hopefully I didn't screw anything up. Feel free to tweak it before merging (or come yell and me and I'll change it again).

Specs/Core/EllipsoidTangentPlaneSpec.js
@@ -139,7 +139,7 @@ defineSuite([
it('constructor throws if origin is at the center of the ellipsoid', function() {
expect(function() {
- return new EllipsoidTangentPlane(Cartesian.ZERO, Ellipsoid.WGS84);
+ return new EllipsoidTangentPlane(Cartesian2.ZERO, Ellipsoid.WGS84);

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

This test is now failing on my system.

@kring

kring Apr 9, 2013

Member

This test is now failing on my system.

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

I'm surprised it wasn't failing before. Should be Cartesian3.

@mramato

mramato Apr 9, 2013

Member

I'm surprised it wasn't failing before. Should be Cartesian3.

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

I guess any exception makes expect(...).toThrow() happy, including dereferencing undefined.

@kring

kring Apr 9, 2013

Member

I guess any exception makes expect(...).toThrow() happy, including dereferencing undefined.

@kring

This comment has been minimized.

Show comment Hide comment
@kring

kring Apr 9, 2013

Member

Updated doc looks good. Thanks.

Member

kring commented Apr 9, 2013

Updated doc looks good. Thanks.

@mramato

This comment has been minimized.

Show comment Hide comment
@mramato

mramato Apr 9, 2013

Member

Okay, this is ready.

Member

mramato commented Apr 9, 2013

Okay, this is ready.

kring added a commit that referenced this pull request Apr 9, 2013

Merge pull request #622 from AnalyticalGraphicsInc/performanceTweaks
Simple but effective performance improvements

@kring kring merged commit 11b75e6 into master Apr 9, 2013

@kring kring deleted the performanceTweaks branch Apr 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment