Should new AxonArray({number}) fail?

[samreid]

From https://github.com/phetsims/axon/issues/308

When invoking

const deletedElements = Array.prototype.splice.apply( this, arguments );

from splice, somehow this ends up invoking the subtype constructor with a number argument. That is, it calls the AxonArray constructor with a {number}.

Thus the constructor has to be documented like so:

  /**
   * @param {Object|number} [options] - Support construction via splice(), which invokes the sub-constructor
   */
  constructor( options ) {

Do we need a way to guard against simulations calling new AxonArray(3)? Right now that isn't caught. We cannot make that a shorthand for new AxonArray({length:3}) because that would have different semantics for the subtype invocation.

Other options:

• Add a code review item or lint rule preventing new AxonArray({number})
• Push/pop a counter in methods like splice that call AxonArray.constructor. Throw an assertion error if AxonArray({number}) is called with counter===0

[pixelzoom]

Why are you calling Array.prototype.splice.apply( this, arguments ) instead of super.splice( arguments ) ?

[pixelzoom]

Support construction via splice(), which invokes the sub-constructor

I'm unable to verify that AxonArray's constructor is called when splice is called. What am I doing wrong?

I added a console.log here, in AxonArray constructor:

    if ( typeof options === 'number' ) {
      console.log( `calling constructor with ${options}` );
      super( options );
      return;
    }

I don't see that log message when exercising AxonArray splice:

> var a = new phet.axon.AxonArray( { elements: [ 1, 2, 3 ] } ) 
undefined
> a.splice( 0, 1 )
AxonArray [1]

[samreid]

I tried the same code in natural-selection-main and I did see the console.log statement. Want to schedule a zoom call?

[samreid]

I also tried a debugger and ran the axon unit tests, and it was triggered.

[pixelzoom]

I do see the console.log now. My browser must've been caching (which has been happening chronically since updating to Chrome 84).

[pixelzoom]

We definitely don't want to be calling new AxonArray( {number} ) in sim code. But I'm not sure what the best solution is for preventing that.

What is the answer to my question in https://github.com/phetsims/axon/issues/311#issuecomment-665177597?

Why are you calling Array.prototype.splice.apply( this, arguments ) instead of super.splice( arguments ) ?

[samreid]

Why are you calling Array.prototype.splice.apply( this, arguments ) instead of super.splice( arguments ) ?

We cannot use super.splice(arguments) because it passes through the arguments object directly as a single argument (like call), but we need them to be spread out. I think const deletedElements = super.splice(...arguments); would have the same functional behavior, but that probably requires more CPU/heap compared to apply. Therefore Array.prototype.splice.apply seems preferable.

Do you want to bring it up for developer meeting about how to avoid new AxonArray( {number} ) in sim code?

[pixelzoom]

Thanks for clarifying.

I still don't know what's best for preventing calls to new AxonArray( {number} ). Of the 2 options you proposed in https://github.com/phetsims/axon/issues/311#issue-666642149, the lint rule seems easiest. But you should get other opinions.

[samreid]

For now, I have forbidden new AxonArray({number}) via a lint rule.

[samreid]

Public Service Announcement about AxonArray and not calling it with {number}.

[pixelzoom]

The PSA is actually that no one should be using AxonArray yet in production code, right?

[samreid]

It looks like there are other issues for developer meeting about the game plan for AxonArray, so this public service announcement will be a brief one about not calling it with {number}.

[pixelzoom]

Over in https://github.com/phetsims/natural-selection/issues/178, @samreid converted natural-selection to use AxonArray. In subclass BunnyArray extends AxonArray, he added this same bit of code that we've been discussing here:

    // Support construction via Array.prototype.splice.apply(), etc., which invoke the sub-constructor
    if ( typeof options === 'number' ) {
      super( options );
      return;
    }

Hopefully we can find a better solution, because having to duplicate this in all subclasses is very undesirable.

[samreid]

At today's meeting, @zepumph recommended overriding methods that return Array so that we can assert that the argument is not a number, and that we can return Array instances instead of the subtype?

@zepumph: Add unit tests that call every array function. To be defensive about whether a browser starts calling the sub-constructor.

It sounds promising, I'll go for it!

Also a reminder to make sure things like splice don't call constructors (should be slice instead).

[samreid]

Even splice is calling the sub-constructor!

[samreid]

Here is a self-contained example of the same thing:

  class MyArray extends Array {
    constructor() {
      super();
      console.log( 'constructor called' );
    }
  }

  console.log( 'creating myarray' );
  const x = new MyArray();
  console.log( 'calling splice' );
  const y = x.splice( 0, 0 );

creating myarray constructor called calling splice constructor called

Why is the browser calling the sub-constructor during splice? Does this happen for other methods?

UPDATE: It is even called for map (which makes sense) and filter which does not.

  class MyArray extends Array {
    constructor() {
      super();
      console.log( 'constructor called' );
    }
  }

  console.log( 'creating myarray' );
  const x = new MyArray();
  console.log( 'calling splice' );
  const y = x.splice( 0, 0 );
  console.log( 'calling map' );
  const z = x.map( a => a );
  console.log( 'calling filter' );
  const ws = x.filter( a => true );

creating myarray
constructor called
calling splice
constructor called
calling map
constructor called
calling filter
constructor called

UPDATE: Splice and filter both return arrays, so it makes sense that Array tries to return the same type as the original Array.

[samreid]

Doesn't call the sub-constructor

array.copyWithin() array.entries() array.every() array.fill() array.find() array.findIndex() array.forEach() array.includes() array.indexOf() array.join() array.keys() array.lastIndexOf() array.pop() array.push() array.reduce() array.reduceRight() array.reverse() array.shift() array.some() array.sort() array.toLocaleString() array.toString() array.unshift() array.values()

Calls the sub-constructor

array.concat() array.filter() array.flat() array.flatMap() array.map() array.slice() array.splice()

[samreid]

I tested in Firefox and Safari and found the behavior was the same as in Chrome (same methods call the sub-constructor). This is really puzzling to me, especially splice which does not return a new array and is supposed to mutate the array in-place. What if some other browser also invokes the sub-constructor for another method? Can we efficiently + correctly reimplement each of these methods? We could correctly implement them by calling Array.from(this).method(...), but that is inefficient in CPU and heap. Shouldn't we rely on the browser implementation as much as possible?

It seems risky to choose a path that relies on a private implementation detail of these methods. And I cannot find a specification that describes how Array is supposed to behave with respect to calling subtype constructor. What if one Array method calls the sub-constructor, but not with a number argument?

Summarizing Options: (a) Override methods that call the subtype constructor. Can we test that we got them all? Can we do this efficiently and accurately? What if some browser invokes the subtype constructor for all methods and we end up reimplementing the entire Array API? (b) Allow some methods to call the subtype constructor. Can we identify these cases, and have them behave like a plain array instead of an AxonArray? At the meeting we decided we didn't want to pursue this, but that was before we knew that things like splice call the sub-constructor. (c) Investigate ways to swap out the prototype before calling these suspicious methods, so it won't call the subtype constructor? Maybe @jonathanolson would have some good ideas in this area.

Extrapolating from our dev meeting consensus, it seems like we should continue pursuing (a).

[samreid]

I added these methods as a correct-but-inefficient placeholder:

  // @public
  concat( a ) { return Array.prototype.concat.apply( Array.from( this ), arguments ); }

  // @public
  filter( a ) { Array.prototype.filter.apply( Array.from( this ), arguments ); }

  // @public
  flat() {return Array.prototype.flat.apply( Array.from( this ), arguments );}

  // @public
  flatMap() {return Array.prototype.flatMap.apply( Array.from( this ), arguments );}

  // @public
  map() {return Array.prototype.map.apply( Array.from( this ), arguments );}

  // @public
  slice() {return Array.prototype.slice.apply( Array.from( this ), arguments );}

However, splice must make notifications since it can add and remove elements. The same trick won't work there:

  // @public
  splice() {
    const deletedElements = Array.prototype.splice.apply( Array.from(this), arguments );

    // Gracefully support values created by axonArray.slice(), etc.
    if ( this.lengthProperty ) {
      this.lengthProperty.value = this.length;
      for ( let i = 2; i < arguments.length; i++ ) {
        this.elementAddedEmitter.emit( arguments[ i ] );
      }
      deletedElements.forEach( deletedElement => this.elementRemovedEmitter.emit( deletedElement ) );
    }
    return deletedElements;
  }

...because splicing on a copy fails to remove elements from the primary copy. I can't think of a way around this without invoking the subconstructor or changing the API (like throwing an error during splice).

I'd like to brainstorm with @jonathanolson if there are other alternatives here.

[samreid]

Here's an option that runs a switcheroo and passes all unit tests

  // @public
  splice() {

    const prototype = Object.getPrototypeOf( this );
    Object.setPrototypeOf( this, Array.prototype );
    const deletedElements = Array.prototype.splice.apply( this, arguments );
    Object.setPrototypeOf( this, prototype );

    // Gracefully support values created by axonArray.slice(), etc.
    if ( this.lengthProperty ) {
      this.lengthProperty.value = this.length;
      for ( let i = 2; i < arguments.length; i++ ) {
        this.elementAddedEmitter.emit( arguments[ i ] );
      }
      deletedElements.forEach( deletedElement => this.elementRemovedEmitter.emit( deletedElement ) );
    }
    return deletedElements;
  }

However, there is an exciting warning on MDN that says this code is deoptimized and possibly sketchy:

[samreid]

After seeing all of these tradeoffs, I am reconsidering recommending that we detect when the AxonArray sub-constructor is invoked, and allow it to peacefully exit, indicating that it is to be treated like a normal Array.

The advantages of this proposal are: (a) implementations will be provided by the built-in Array type, and hence will be efficient and correct (b) we will not need to trigger any deoptimizations

The disadvantages of this proposal: (a) We will need code to detect when the sub-constructor is invoked (more robust than checking numeric argument), and this will be in AxonArray and any of its subtypes (b) We will need a way to indicate when an AxonArray has been constructed in this way, and client usages will have to treat it accordingly. For instance, instanceof AxonArray checks wouldn't be enough, it would need to be instanceof AxonArray && array.hasAxonArrayFeatures

On the other hand, maybe we shouldn't be deterred by performance warnings until we observe that it is problematic in practice? But many AxonArray instances will need to call splice since it is the only way to remove elements. So maybe it means every AxonArray would be deoptimized?

UPDATE: Goals for AxonArray included:

• Reuse browser implementation as much as possible
• High performance (CPU and heap/garbage)

So even if performance tests right now indicate that setPrototypeOf is not a problem, that doesn't guarantee that it will remain that way in all future browser implementations. I feel like allowing the AxonArray subconstructor to be called is more like "taking matters into our own hands" and we can deal with the fallout in the simulation implementation side.

[pixelzoom]

Summarizing Options: ...

(c) Bail on AxonArray altogether. Mention to the JavaScript working group that they really shouldn't have skipped that Object-Oriented Programming 101 class.

[pixelzoom]

... similarly in https://github.com/phetsims/axon/issues/312#issuecomment-679317030, I said:

... If it were me, I'd bail completely on extending Array. When there are this many weird problems, composition is definitely favored over inheritance.

[jonathanolson]

After doing a bit more diving into this, I'm trying to understand the use case for AxonArray over ObservableArray/Array. The doc says:

AxonArray adds the ability to observe when elements are added or removed from an Array. This was created as an alternative to ObservableArray with the distinguishing change that this extends Array and hence uses the native Array API.

but, what is the advantage? I don't feel like I can suggest options without understanding the purpose.

The PSA is actually that no one should be using AxonArray yet in production code, right?

I'm code-reviewing code that is using AxonArray, so this is coming up as part of https://github.com/phetsims/collision-lab/issues/153.

The advantages of this proposal are: (a) implementations will be provided by the built-in Array type, and hence will be efficient and correct (b) we will not need to trigger any deoptimizations

Subtyping Array potentially includes performance issues, right? Has performance been tested? I also see a list of things that isn't supported.

[pixelzoom]

@brandonLi8 @jonathanolson I really don't think that AxonArray should be used in collision-lab. It has too many problems, and any advantages that it might have had over ObservableArray feel like they are evaporating.

[brandonLi8]

@samreid one proposal could be to provide the Symbol.species static getter.

With this:

  static get [Symbol.species]() { return Array; }

filter, map, etc. would return Array and we shouldn't need to override the default browser implementation. Thoughts?

[samreid]

static get [Symbol.species]() { return Array; } is exactly what we needed, thanks @brandonLi8! I committed it above. This allowed us to remove all of the workarounds related to calling the sub-constructor.

[pixelzoom]

JavaScript continues to amaze me (and not in a good way).

[samreid]

I'm trying to understand the use case for AxonArray over ObservableArray/Array

• The main idea is that by subtyping Array we automatically get a uniform API that is consistent with the built-in Array API.
• Recent work was spurred by https://github.com/phetsims/axon/issues/283#issuecomment-654994805 which says:

I'd also like to point out that forEach isn't the only method that exposes the underlying array which the user could modify. [...]

That's the main reason why I avoid using those "convenience" methods of ObservableArray. I think it's much clearer to call getArray() or getArrayCopy(), then use the Array API directly.

[samreid]

Thanks to @brandonLi8, the problem regarding "Should new AxonArray({number}) fail?" has been addressed, and this issue is ready to close. However, there have been more general questions and comments by @pixelzoom and @jonathanolson which should be resolved or moved elsewhere first.

[brandonLi8]

I prefer sticking to AxonArray in collision lab.

To add onto what @samreid said in https://github.com/phetsims/axon/issues/311#issuecomment-679429777...

I believe (but not fully sure without performance tests) that AxonArray is actually more performant than ObservableArray because it is able to utilize native methods from Array which are optimized. Regardless, collision lab doesn't utilize any large AxonArrays/ObservableArrays.

I also believe it is more natural to have AxonArray to be an actual array instead of wrapping an array via composition (which ObservableArray does). From the CRC:

• [ ] Prefer the most basic/restrictive type expression when defining APIs. For example, if a client only needs to know that a parameter is {Node}, don’t describe the parameter as {Rectangle}.

I think this applies to my usages of AxonArray in Collision Lab. In some APIs, it is not necessary to know that the passed-in array is an AxonArray or an ObservableArray, so I can define my API in terms of any Array. In other API's, I need to register a listener (or something specific to AxonArray), in which I define my API to take in the AxonArray.

[jonathanolson]

I’ll look into this more tomorrow, including performance. The documentation also states that this is NOT API compatible with Array, since there are unsupported methods/getters/setters, so it seems unsafe in general to pass an AxonArray in place of an Array, no?

If those are the goals, I’m curious if Proxy has been tested (as it probably would have worse performance unfortunately), since it presumably wouldn’t suffer from any API compatibility drawbacks.

[pixelzoom]

Looks OK to me, unassigning.

[jonathanolson]

1. If we're passing subtyped arrays to things that expect vanilla arrays, it's a potential that code would need to get deoptimized to handle another type of array. My testing shows no major difference in Chrome/Safari, but a potential difference in Firefox (which could slow down our code in general, beyond the scope of what AxonArray is used for). This could drown out performance gains of the actual arrays, but may not also. Firefox showed a 19.5% +/- 2% REDUCTION in speed for code that has handled an AxonArray as an Array once.
2. The core behavior of AxonArray seems 10-20% faster (although AxonArray isn't doing duplicate checks or conditionals that may add some other overhead).
3. AxonArray doesn't seem API compatible for a lot of our needs. The core differences also trigger things like lodash's .remove, .pull, etc. to fail to notify, e.g.:

var arr = new axon.AxonArray();
arr.addItemAddedListener( item => console.log( `add ${item}` ) );
arr.addItemRemovedListener( item => console.log( `remove ${item}` ) );
arr.push( 1, 2, 3 ); // notifies
_.remove( arr, x => x === 2 ); // does not notify
_.pull( arr, 3 ); // does not notify
arr.length = 0; // does not notify (as expected)

4. We're adding in enumerable properties, is this an issue? Object.keys( arr ) includes ["elementAddedEmitter", "elementRemovedEmitter", "lengthProperty", "axonArrayPhetioObject", "phetioElementType"]
5. We don't need prototype references here? AxonArray.prototype.push.apply( this, options.elements ) can be this.push( ...options.elements )? Array.prototype.something can be super.something, no?

So overall: this looks like it can be a cleaner ObservableArray with restrictions! However it seems unsafe to treat it as a general Array, both because of API reasons, AND because of the deoptimizations we can trigger in that case.

@samreid, @brandonLi8, @pixelzoom thoughts?

[jonathanolson]

Benchmarking committed above, should be testable with scenery/tests/benchmarking.html (there is other benchmarking code in that general location)

[samreid]

Results on my mac/chrome:

Creation#Array x 80,851,468 ops/sec ±2.68% (58 runs sampled)
Creation#ObservableArray x 26,569 ops/sec ±6.21% (63 runs sampled)
Creation#AxonArray x 27,056 ops/sec ±1.70% (63 runs sampled)
AssortedActions#Array x 718 ops/sec ±1.60% (61 runs sampled)
AssortedActions#ObservableArray x 71.20 ops/sec ±1.08% (53 runs sampled)
AssortedActions#AxonArray x 85.57 ops/sec ±0.85% (55 runs sampled)
Deopt#pure x 718 ops/sec ±0.36% (49 runs sampled)
Deopt#impure x 700 ops/sec ±2.78% (48 runs sampled)

Results on my Mac/Firefox:

Creation#Array x 10,962,687 ops/sec ±3.73% (55 runs sampled)
Creation#ObservableArray x 8,700 ops/sec ±6.42% (53 runs sampled)
Creation#AxonArray x 9,436 ops/sec ±2.47% (58 runs sampled)
AssortedActions#Array x 2,711 ops/sec ±2.26% (48 runs sampled)
AssortedActions#ObservableArray x 12.02 ops/sec ±2.02% (33 runs sampled)
AssortedActions#AxonArray x 15.80 ops/sec ±1.19% (29 runs sampled)
Deopt#pure x 2,274 ops/sec ±11.31% (17 runs sampled)
Deopt#impure x 1,978 ops/sec ±0.44% (14 runs sampled)

[samreid]

_.remove( arr, x => x === 2 ); // does not notify

Lodash unfortunately calls Array.prototype.splice(array) instead of array.slice.

[samreid]

5. We don't need prototype references here? AxonArray.prototype.push.apply( this, options.elements ) can be this.push( ...options.elements )? Array.prototype.something can be super.something, no?

I was concerned that our Babel step would do something suboptimal for this.push(...options.elements). For instance, babel targeting es2015 converts

    // A
    AxonArray.prototype.push.apply( this, options.elements );

    // B
    this.push(...options.elements);

to

    // A
    AxonArray.prototype.push.apply(_assertThisInitialized(_this), options.elements);

    // B
    _this.push.apply(_this, _toConsumableArray(options.elements));

That being said, this.push(...options.elements) does read nicer, and maybe prototype.push.apply is a premature optimization?

[samreid]

Should we prototype a Proxy-based solution that aims for 100% safety?

[jonathanolson]

Should we prototype a Proxy-based solution that aims for 100% safety?

If that doesn't cause deoptimizations or performance loss, that would be great! I fear it's likely to cause performance loss, but could be worth a try. Probably worth some checks with performance.

[samreid]

I'm interested in prototyping that. I'll self assign, but also leave marked for developer meeting in case we have time to touch base and get a shared sense of what is acceptable on the performance/safety tradeoff spectrum.

[samreid]

Can you recommend how to proceed with Array + Proxy and subclassing? For instance, we have BunnyArray extends ObservableArray and adds methods and attributes. But I don’t see a clear way to do that with Proxy, which cannot be subclassed like class MyClass extends Proxy. The "return a different object from the constructor" pattern appears to be used commonly as an alternative, but then I don't see how subclassing would work for that. Or should we say that subclassing would not be possible if we use Proxy?

UPDATE: here's my patch for when I return to this:

```diff Index: js/axon-tests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/axon-tests.js (revision d96cb126b896fa92103943367c9f154918efe627) +++ js/axon-tests.js (date 1598633811373) @@ -7,6 +7,7 @@ */ import qunitStart from '../../chipper/js/sim-tests/qunitStart.js'; +import './createArrayProxyTests.js'; import './AxonArrayTests.js'; import './BooleanPropertyTests.js'; import './DerivedPropertyTests.js'; Index: js/createArrayProxyTests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/createArrayProxyTests.js (date 1598633811371) +++ js/createArrayProxyTests.js (date 1598633811371) @@ -0,0 +1,30 @@ +// Copyright 2020, University of Colorado Boulder + +/** + * QUnit tests for ObservableArray + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +import createArrayProxy from './createArrayProxy.js'; + +QUnit.module( 'ArrayProxy' ); + +QUnit.test( 'Hello', assert => { + + assert.ok( 'first test' ); + + const arrayProxy = createArrayProxy(); + arrayProxy.push( 'hello' ); + console.log( arrayProxy[ 0 ] ); + + console.log( 'START: setting length = 0' ); + arrayProxy.length = 0; + console.log( 'END: setting length = 0' ); + + console.log( 'START: getting length' ); + console.log( 'voila: ' + arrayProxy.length ); + console.log( 'END: getting length' ); + + arrayProxy.slice( 0 ); +} ); \ No newline at end of file Index: js/createArrayProxy.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/createArrayProxy.js (date 1598633811381) +++ js/createArrayProxy.js (date 1598633811381) @@ -0,0 +1,42 @@ +// Copyright 2020, University of Colorado Boulder + +/** + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +import axon from './axon.js'; + +// class cannot extend Proxy directly. How important is subclassing for this? BunnyArray makes it sound important +// https://stackoverflow.com/questions/37714787/can-i-extend-proxy-with-an-es2015-class + +const createArrayProxy = () => { + + const originalArray = []; + + // other method calls done from the first intercepted method are NOT intercepted + // https://gist.github.com/XoseLluis/94e071c659cbbb3728db + return new Proxy( originalArray, { + + // target is the object being proxied, receiver is the proxy + get: function( target, key, receiver ) { + + console.log( 'key: ' + key ); + // I only want to intercept method calls, not property access + const value = target[ key ]; + if ( typeof value !== 'function' ) { + console.log( 'intercept property access for ' + key ); + return value; + } + else { + return () => { + console.log( 'intercepting call to ' + key + ' in array' + target ); + return value.apply( target, arguments ); + }; + } + } + } ); +}; + +axon.register( 'createArrayProxy', createArrayProxy ); +export default createArrayProxy; \ No newline at end of file ```

[samreid]

We would like to experiment with Proxy to have full support for array.length=... and array[3]=differentObject. However, that will probably have the following disadvantages:

1. lower performance
2. more difficulty in code navigation (e.g. navigating to an AxonArrayProxy method)
3. difficulty extending AxonArrayProxy, like BunnyArray extends AxonArrayProxy

For developer meeting: are any of these dealbreakers? Does anyone know a way around (3)? - more detail on that in previous comment.

[zepumph]

We discussed this at developer meeting today, and feel like Proxy is worth investigation, as nothing above feels like a total "deal breaker", although it seems like a bummer to not be able to use inheritance.

We are most excited about 100% (or close to) api coverage of native Array. This will help convert arrays to this new type.

[samreid]

I'll pursue the Proxy-based approach in new issue #330.

[samreid]

On hold pending results in #330

[samreid]

330 is working well and does not suffer from this problem, closing.