branch: createArrayProxy

[pixelzoom]

This is a branch for experimenting with createArrayProxy.js, a potential replacement for ObservableArray. See phetsims/axon#330.

[pixelzoom]

In the above commits, I converted to createArrayProxy.

[pixelzoom]

I did memory test -- brand=phet as in #232, brand=phet-io as in #233. Heap sizes were similar, and no memory leaks were identified.

[pixelzoom]

Hmm... We have a problem with "Start Over" and "Reset All" performance.

Testing on my MacBookPro16,1 + macOS 10.15.6 + Chrome 85.0.4183.121 :

• Before converting to createArrayProxy, "Start Over" took ~90 ms.
• After converting to createArrayProxy, "Start Over" is taking > 1200ms, which is unacceptable according to improve responsiveness of "Start Over" button #140

Before I dive in... @samreid any thoughts on what could be causing this?

Steps to reproduce:

1. Check out master or 'createArrayProxy' branch of natural-selection, depending on which you want to test.
2. Run the sim with ?showTimes&secondsPerGeneration=1
3. Go to Lab screen
4. Press "Add a Mate" button
5. Wait for bunnies to take over the world, then close dialog
6. Press "Start Over" button and note the time displayed at upper-left corner

[samreid]

On my Mac/Chrome, with this URL (no assertions):

http://localhost/vanilla/natural-selection/natural-selection_en.html?brand=phet&showTimes&secondsPerGeneration=1&screens=2

I see 80ms in master, and 1071ms in the branch (for Start Over). After this patch:

Index: main/tandem/js/PhetioGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/tandem/js/PhetioGroup.js	(revision 2f8159ca6f9a3758a6ffaef9f9071ee5512aa0c8)
+++ main/tandem/js/PhetioGroup.js	(date 1601353233469)
@@ -199,7 +199,7 @@
     }, options );
 
     while ( this._array.length > 0 ) {
-      this.disposeElement( this._array[ this._array.length - 1 ], options.fromStateSetting );
+      this.disposeElement( this._array[ 0 ], options.fromStateSetting );
     }
 
     if ( options.resetIndex ) {
Index: main/natural-selection/js/common/model/BunnyCollection.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/natural-selection/js/common/model/BunnyCollection.js	(revision eb577bbef918ace3341eaa120f8997d755c3584e)
+++ main/natural-selection/js/common/model/BunnyCollection.js	(date 1601353158855)
@@ -177,9 +177,15 @@
     // When a bunny is disposed, remove it from the appropriate array. removeListener is not necessary.
     bunnyGroup.elementDisposedEmitter.addListener( bunny => {
       assert && assert( bunny instanceof Bunny, 'invalid bunny' );
-      this.liveBunnies.includes( bunny ) && this.liveBunnies.remove( bunny );
-      this.deadBunnies.includes( bunny ) && this.deadBunnies.remove( bunny );
-      this.recessiveMutants.includes( bunny ) && this.recessiveMutants.remove( bunny );
+
+      const liveIndex = this.liveBunnies.indexOf( bunny );
+      liveIndex >= 0 && this.liveBunnies.splice( liveIndex, 1 );
+
+      const deadIndex = this.deadBunnies.indexOf( bunny );
+      deadIndex >= 0 && this.deadBunnies.splice( deadIndex, 1 );
+
+      const recessiveIndex = this.recessiveMutants.indexOf( bunny );
+      recessiveIndex >= 0 && this.recessiveMutants.splice( recessiveIndex, 1 );
     } );
 
     // @private fields needed by methods

I'm seeing 87ms in the branch.

[samreid]

I committed the change in PhetioGroup. @zepumph can you please review it? It changes PhetioGroup removal from First In/Last Out to First In/First Out so that, if you go in order, indexOf calls won't have to search the whole array.

Also leaving assigned to @pixelzoom to apply the performance improvements in BunnyCollection.js.

[zepumph]

That looks really nice. Good work!

[pixelzoom]

The improvement to BunnyCollection is a lot of boilerplate. I investigated using arrayRemove.js, but it seems like that functionality should be rolled into createArrayProxy.js.

My recommendation is to plan on keeping ArrayProxy.remove, and change its implementation like this:

-  remove: function( element ) { this.includes( element ) && arrayRemove( this, element );},
+ remove: function( element, required = true ) {
+   const index = this.indexOf( element );
+   required && assert && assert( index >= 0, 'item not found in Array' );
+   this.splice( index, 1 );
+ }

I'm not wild about the required parameter, and I think my preference would be for two variations of remove:

-  remove: function( element ) { this.includes( element ) && arrayRemove( this, element );},
+ remove: function( element ) {
+   const index = this.indexOf( element );
+   assert && assert( index >= 0, 'item not found in Array' );
+   this.splice( index, 1 );
+ }
+
+ removeIfIncludes: function( element ) {
+   const index = this.indexOf( element );
+   if ( index >= 0 ) {
+     this.splice( index, 1 );
+   }
+ }

@samreid thoughts?

[samreid]

It would probably be fine to have remove and removeIfIncludes. I considered trying to match the Array API as much as possible (to make it easy to port ArrayProxy => Array), but maybe that's not too important. But for the sake of discussion, how about:

arrayRemove(myArray,element);
arrayRemoveIfIncludes(myArray,element);

[pixelzoom]

@samreid which of these do you prefer?

(A) Client is responsible for getting and checking index, then doing splice. Lots of boilerplate.

     bunnyGroup.elementDisposedEmitter.addListener( bunny => {
       assert && assert( bunny instanceof Bunny, 'invalid bunny' );

      const liveIndex = this.liveBunnies.indexOf( bunny );
      liveIndex >= 0 && this.liveBunnies.splice( liveIndex, 1 );

      const deadIndex = this.deadBunnies.indexOf( bunny );
      deadIndex >= 0 && this.deadBunnies.splice( deadIndex, 1 );

      const recessiveIndex = this.recessiveMutants.indexOf( bunny );
      recessiveIndex >= 0 && this.recessiveMutants.splice( recessiveIndex, 1 );
     } );

(B) Above boilerplate factored out into standalone functions. Not object-oriented, requires additional imports.

import arrayRemove from '../../../../phet-core/js/arrayRemove.js';
import arrayRemoveIfIncludes from '../../../../phet-core/js/arrayRemoveIfIncludes.js';

    bunnyGroup.elementDisposedEmitter.addListener( bunny => {
      assert && assert( bunny instanceof Bunny, 'invalid bunny' );
      arrayRemoveIfIncludes( this.liveBunnies, bunny );
      arrayRemoveIfIncludes( this.deadBunnies, bunny );
      arrayRemoveIfIncludes( this.recessiveMutants, bunny );
    } );

(C) ArrayProxyDef extended to provide a better "remove" API. New methods are not compatible with back-porting to Array.

    bunnyGroup.elementDisposedEmitter.addListener( bunny => {
      assert && assert( bunny instanceof Bunny, 'invalid bunny' );
      this.liveBunnies.removeIfIncludes( bunny );
      this.deadBunnies.removeIfIncludes( bunny );
      this.recessiveMutants.removeIfIncludes( bunny );
    } );

[samreid]

I recommend we start with (C) for now, but if we eventually need to move back and forth between ArrayProxy -> Array in the future, we could consider (B).

[pixelzoom]

Great, because I have a strong preference for (C). I'll modify createArrayProxy.js accordingly and proceed with NS changes.

[pixelzoom]

Once I got further into this, I bailed on any changes to the "remove" API for ArrayProxyDef. I unfortunately stuck with approach (A) (boilerplate) for now.

In the above commit, I replaced uses of ArrayProxyDef remove throughput BunnyCollection. And for bunnyGroup.elementDisposedEmitter listener, I did a further optimization -- it's not necessary to look at all 3 arrays when a bunny is disposed.

Unbuilt testing with ?showTimes&secondsPerGeneration=1 (MacBookPro16,1 + macOS 10.15.6 + Chrome 85.0.4183.121) shows that "Start Over" performance is now ~90 ms. Very acceptable, and what it was before the conversion to createArrayProxy.

I also verified that memory footprint has not changed, using the procedure in #232.

[pixelzoom]

Oh rats... I did d325a40 in master. Stand by while I fix this.

[pixelzoom]

This branch stopped working with master dependencies because of changes to IO Types. So I manually merged what I could into master, and I'm giving up on this branch. Everything seems to be working OK in master, so I'll close this issue and delete this branch.

[pixelzoom]

For https://github.com/phetsims/special-ops/issues/198, this branch needs to be deleted again. Reopening and self assigning.

[pixelzoom]

Re-deleted using GitHub UI. Closing.