Rational for iterator changes

[brandonLi8]

@jonathanolson I see several commits that were made that changed the iterator usage in collision lab, specifically changing forEach to for (let i = 0.... loops (17961bd42deb2bb25a7d0198212a4a9c2ec87884, c40dae95ddc7cb670b646f2f213bc4b82c59674a, 6c83cc605276e21a7cd0d9c99a7f06185075ce5f, fb2ff4c1d5cc09d5219e41ca5d86687542f78eb9).

I would like some justification to why these changes were made. In terms of performance, for loop performance is browser dependent (see https://github.com/phetsims/chipper/issues/966) with chrome heavily optimizing forEach. Although it was decided that iterator usages was developer discretion, I think forEach loops are easier to read and is more consistent within the PhET code base.

[jonathanolson]

I think forEach loops are easier to read and is more consistent within the PhET code base.

100% completely agree, so for situations where I haven't heard performance (and specifically garbage collection like pause patterns) are an issue, I highly prefer the more readable version.

Instead of quoting past results that have shown things, I wanted to re-run some tests to ensure browsers haven't optimized things like this (which could change things).

First some setup code for hacking testing:

var arr = _.range( 0, 5 )
var value = 0;

Then a for-loop version:

// For loop (in order, checking length every iteration, so maybe not the fastest)
var start = Date.now();
for ( let i = 0; i < 100000000; i++ ) {
  for ( let j = 0; j < value.length; j++ ) {
    value += v;
  }
}
console.log( Date.now() - start );

and a forEach version:

// forEach (with a closure)
var start = Date.now();
for ( let i = 0; i < 100000000; i++ ) {
  arr.forEach( v => { value += v } );
}
console.log( Date.now() - start );

In Chrome (just picking the Scenery playground as something with no background noise running), I got the timing:

• for-loop: ~40-55ms
• forEach: ~1300-4500ms

The usual case here seems like the for loop is around 100 times faster (which in practice isn't going to be the constraint, and is NOT the primary reason I optimized things here).

However the memory issues (garbage collection patterns, which I heard from @ariel-phet seemed laggy in the simulation) can be affected heavily by these types of closures. I switched the test to run every frame with a smaller number of iterations:

var arr = _.range( 0, 5 )
var value = 0;
(function f() {
  requestAnimationFrame( f );

  for ( let i = 0; i < 1000000; i++ ) { for ( let j = 0; j < value.length; j++ ) { value += v } }
})();

and

var arr = _.range( 0, 5 )
var value = 0;
(function f() {
  requestAnimationFrame( f );

  for ( let i = 0; i < 1000000; i++ ) { arr.forEach( v => { value += v } ) }
})();

The forEach is creating closures internally, which is adding a TON of work for the garbage collector. It is running about 50 minor GCs a frame (again, not realistic), but basically every single time the forEach is called, it's adding a new object AND the collector is recording that and having to clean it up:

Whereas the for-loop version is not creating any objects for the GC, in addition to being significantly faster. In this snapshot, it seems two unrelated GCs happened (but otherwise there is a flat line of memory, since nothing is being allocated):

I'd encourage you to potentially re-run the testing (or tell me if I'm doing something wrong in my testing). It backs up my previous results with "creating closures creates garbage that still isn't getting optimized out, and in this contrived example is significantly faster". Because there were reports of garbage collection issues causing undesirable performance, I think it makes sense in this case to be stripping these garbage creators out.

For reference, I highly prefer starting with easy-to-read/maintain, which is exactly what you had (and it was great!), but unfortunately sometimes things need to be turned into a much uglier form in order to improve performance.

[brandonLi8]

Thanks for laying this out @jonathanolson. I was just looking for some rational of the changes and I was unaware that garbage collection was a problem for the sim.

[brandonLi8]

Closing.