phetsims / dot

A math library with a focus on mutable and immutable linear algebra for 2D and 3D applications.
http://scenerystack.org/
MIT License
13 stars 6 forks source link

Performance problems with using "FastArray" in Matrix3 #81

Closed jbphet closed 5 years ago

jbphet commented 5 years ago

As of this writing, Matrix3.js contains the following bit of code starting at line 26:

  var createIdentityArray = FastArray === Array ?
                            function() {
                              return [ 1, 0, 0, 0, 1, 0, 0, 0, 1 ];
                            } :
                            function() {
                              return new FastArray( identityFastArray );
                            };

As part of work that is being done to improve the performance of the energy-forms-and-changes-sim (see https://github.com/phetsims/energy-forms-and-changes/issues/99), I was seeing a lot of time spent in this function. Below is a screenshot that shows the bottom of the call stack with a bunch of long calls to createIdentityArray. The tool top shows that one of these calls is taking almost 14ms, and there are a bunch of these calls, so all together they are essentially killing the performance of the drag operations.

image

I tried changing this code not to use FastArray, and the problem seemed to go away. I showed this to @jonathanolson over zoom, and he was surprised, and basically said that he'd take it from here.

jonathanolson commented 5 years ago

Snapshot comparisons with added performance tests are showing that this should be safe, and the normal Array version in Chrome seems to be the overall fastest (shaving about 4% off of the combined sim load/fuzz from what we had before).

@jbphet can you see how this behaves in EFAC?

jbphet commented 5 years ago

This is a vast improvement, so much so that I don't even see the calls to createIdentityArray in the call tree when I profile. To compare, I picked a drag event from a RequireJS version on master and compared the duration of the call to globalToParentPoint to that in the profile shown in the image above. Before the change, this was taking 70 ms, not it's taking 0.13 ms. I'd call that a big win.

Here's a screenshot that shows this. Note that I had to be zoomed in a lot further than in the profile above to even see the method calls that I was looking for.

image

jbphet commented 5 years ago

@jonathanolson - this is resolved as far as I'm concerned, you're welcome to close it unless there is further work that you believe is warranted.