Performance of PhET-iO version is unacceptable

[pixelzoom]

For https://github.com/phetsims/qa/issues/921 ...

https://github.com/phetsims/calculus-grapher/issues/307 got me thinking about this. And it's possible that we already did this for https://github.com/phetsims/calculus-grapher/issues/90, but it's worth confirming.

Our entire architecture is built on reusing CurvePoint instances, then having curveChangedEmitter fire when the user completes an interaction. We should verify that CurvePoint instances are only created when setting the value of originalCurve.pointsProperty or predictCurve.pointsProperty via Studio, and not when PhET-iO is restoring state (for example via the State Wrapper).

[pixelzoom]

@veillette and I paired on this. We discovered that, in PhET-iO versions, pointsProperty is updated on every interaction, creating an entirely new set of CurvePoint instances. The result is that responsiveness of PhET-iO versions (including Standalone) is awful, even on a relatively-new MacBook Pro.

Our entire architecture is based on having one set of CurvePoints that is mutated only if the ID does so by setting pointsProperty in Studio. This is how we are able to modify so many points and remain responsive. What we (mainly I) failed to consider is that, because pointsProperty is instrumented, it's also saved/restored by PhetioEngine. It's how our CurvePoint instances are being serialized. And we have no way to differentiate between when PhetioEngine does the restore, vs the ID doing a "Set Value" in Studio.

This is probably going to require significant changes to the foundation of this sim. I think it's also likely that we'll need to delay creating a release branch, and will therefore need another QA dev-test.

@amanda-phet @kathy-phet FYI.

[samreid]

I confirmed the performance in unbuilt phet-io standalone is very poor on my Macbook Air M1. However, a huge part of the time is being spent in phetioStartEvent, so the first thing to try would be omitting the curves from the data stream:

[pixelzoom]

... so the first thing to try would be omitting the curves from the data stream:

Emitting the curves from the data stream is not possible with our current architecture -- it would result in an unsable sim.

Are you suggesting this as a (temporary) way to see if the curves are responsible for the performance problem? We're certain of that.

[pixelzoom]

This comment in Curve.ts is not quite correct, and this code is being excuted more often than we thought.

      // For interactive and derived Curves, we mutate CurvePoints. The value of pointsProperty therefore does not change
      // unless set via PhET-iO for a Curve instantiated with pointsPropertyReadOnly:false. So when the sim is changing
      // the curve (either by the user manipulating a curve, or the sim deriving a curve), this is needed to notify
      // Studio that pointsProperty has effectively changed.
      this.curveChangedEmitter.addListener( () => {
        if ( notifyListeners ) {
          this.pointsProperty.notifyListenersStatic();
        }
      } );

[pixelzoom]

@samreid and I spent some time on this.

This bit of code in Curve.ts is the performance culprit. If we elminate the call to notifyListenersStatic, the sim is very responsive.

      // For interactive and derived Curves, we mutate CurvePoints. The value of pointsProperty therefore does not change
      // unless set via PhET-iO for a Curve instantiated with pointsPropertyReadOnly:false. So when the sim is changing
      // the curve (either by the user manipulating a curve, or the sim deriving a curve), this is needed to notify
      // Studio that pointsProperty has effectively changed.
      this.curveChangedEmitter.addListener( () => {
        if ( notifyListeners ) {
          this.pointsProperty.notifyListenersStatic();
        }
      } );

I still need to investigate the bit of code that comes after that (below), but this is headed in the right direction:

      // For Curve instances created with pointsPropertyReadOnly:false, pointsProperty may be set via PhET-iO. If that
      // happens, notify listeners. Guard against pointsProperty's listener and curveChangedEmitter calling each other.
      if ( !options.pointsPropertyReadOnly ) {
        this.pointsProperty.lazyLink( ( newPoints, oldPoints ) => {
          if ( newPoints !== oldPoints ) {
            notifyListeners = false;
            this.curveChangedEmitter.emit();
            notifyListeners = true;
          }
        } );
      }

This does present a UI problem in Studio. The reason that this code is here is so that Studio's "Current Value" display will update. That's not going to happen with this approach. And while seeing the "Initial Value" and "Current Value" is not at all useful for pointsProperty, there's currently no way to opt-out of showing "Current Value" for a Property in Studio.

@samreid suggested that we address the stale "Current Value" via phetioDocumentation, something like:

phetioDocumentation: 'The discrete points that are used to approximate the curve. ' +
  'Note that \"Current Value`" is not updated for performance reasons. ' + 
  'Use "Get Value" when you need to get or view the current value'.

Other options that @samreid suggested for the stale "Current Value" were:

• Hide it automatically if the value is greater than some length. This will be problematic if something has a variable length that changes from (say) zero to 5000 characters. I'm not a fan of this option.
• Add additional metadata (PhetioObject options) to opt-out of displaying "Current Value" and/or "Initial Value" in Studio. @samreid recommended against this because it's not aligned with the longterm vision for metadata.
• Replace pointsProperty with something like PointsIO, and IOType. Add a custom view for PointsIO to PhetioElementView.ts that excludes the "Current Value" and "Initial Value". This would be similar to what @samreid did for CircuitIO in CCK. I'm not wild about this approach for 2 reasons: (1) it will be a big change to calculus-grapher, and (2) it will put code that's specific to calculus-grapher in the studio repo.

So... My recommendation is to document (via phetioDocumentation and examples.md) that "Current Value" is not updated, and live with it. @amanda-phet and @kathy-phet is that acceptable?

[kathy-phet]

@amanda-phet and @kathy-phet and @phet-brent met and this looks like a good direction to us ... with the documentation you suggested above. Thanks!

[pixelzoom]

Great thanks. I'll proceed.

[pixelzoom]

I applied the performance fix and phetioDocumentation update in the above commit. I tested in PhET brand, Studio, and the State Wrapper. Responsiveness in the PhET-iO version is now identical to PhET brand -- really nice and smooth.

The phetioDocumentation was a little more complicated than anticipated, because the Set/Get Value buttons are only available for originalCurve.pointsProperty and predictCurve.pointsProperty, and not available for the derived curves (integralCurve, derivativeCurve, secondDerivativeCurve).

For originalCurve.pointsProperty and predictCurve.pointsProperty, documentation looks like this:

For the derived curves, the last sentence that refers to the "Get Value" button is omitted:

Since this is a major change, I'd like to have multiple eyes reviewing it. @Nancy-Salpepi, @veillette, and @amanda-phet please have a look before we create the 1.0 release branch (scheduled for 3/28). Focus on Studio (including the "Test" button) and the State Wrapper, and verify that everything looks OK when you're:

• [ ] directly manipulating f(x)
• [ ] undoing operations with the Undo button
• [ ] pressing the Eraser button
• [ ] resetting the sim with the Reset All button
• [ ] setting originalCurve.pointsProperty via Studio's Get/Set Value buttons

Also let me know if you have wordsmithing suggestions for the phetioDocumentation shown above.

[veillette]

I see that it was a bit of jiujitsu to add to add the documentation.
The added phet-io documentation reads well. The meaning and justification are clear.

I tested the undo, eraser and reset all button with the state wrapper and in studio.

In the state wrapper, you can see the state of the curve in the downstream simulation and the simulation states receives update. It is still quite laggy as it receives the large JSON updates but I think that is a good compromise.

However, in studio, the curve is very responsive. I was able to see the JSON curve from the get Value and set the curve as well. Most importantly, the test simulation is very responsive.

[Nancy-Salpepi]

On MacBook Air macOS 13.2.1 and Safari 16.3:

Responsiveness was great in Studio and in the Standard PhET-iO wrapper when manipulating the curve and pressing the Eraser/Undo/Reset All buttons. Get Value/Set Value also worked nicely.

In the State wrapper, there was a noticeable lag. I compared it with dev.25 and it was actually worse. There is even a noticeable lag when switching screens. I tested keeping the Set State Rate at the default value and with Set State Rate= 0. After a few minutes, I see this message in master:

Here is a video comparing dev.25(left) to master. Sorry it is a bit long:

https://user-images.githubusercontent.com/87318828/227950863-1d3ee4eb-0aa7-4edf-a7ae-38cfbbdf535d.mp4

[veillette]

Hmm, I have a shaky understanding of it but I think it is related to

this.pointsProperty.lazyLink( () => {
      assert && assert( Tandem.PHET_IO_ENABLED, 'pointsProperty may change only in PhET-iO brand' );
      this.curveChangedEmitter.emit();
    } );

This listener lives in Curve, so therefore all Curves will emit, whereas it is only necessary for the TransformedCurve to emit since the derivatives and integral are already listening to the emit of TransformedCurve.

[veillette]

By the way, thanks @Nancy-Salpepi for the careful comparison. It is only with a careful analysis that you can really tell that the performance is not the same. I'm assigning back to @pixelzoom and myself.

[veillette]

This is really more in @pixelzoom wheelhouse, but I'll give it a go.

[samreid]

In https://github.com/phetsims/calculus-grapher/issues/312#issuecomment-1481776137 we discussed the long delay in getState/setState and agreed it is not a problem for this release. Please slack me if you wish to discuss on zoom.

[veillette]

I think we have too many emitters, such that instead of this

this.pointsProperty.lazyLink( () => {
      assert && assert( Tandem.PHET_IO_ENABLED, 'pointsProperty may change only in PhET-iO brand' );
           this.curveChangedEmitter.emit();
      } );

we should replace it by

this.pointsProperty.lazyLink( () => {
      assert && assert( Tandem.PHET_IO_ENABLED, 'pointsProperty may change only in PhET-iO brand' );
      if ( !options.pointsPropertyReadOnly ) {
        this.curveChangedEmitter.emit();
         }
    } );

such that only the curve that are not read only can emit. Recall that the TransformedCurve have pointsPropertyReadOnly false. Since the 'TransformedCurves' are the "ground truth", we do not need to emit for the 'DerivativeCurve', 'IntegralCurve', etc

If one puts a console.log in each calls, one find that the first one get called 15 times every second whereas for the second its only 8. I still dont understand why there are so many ( I would expect only two) but's that progress. Perhaps the listener should be hoisted inside TransformedCurve instead.

[pixelzoom]

I'll have a look later this afternoon.

[veillette]

Trying out to find the tandem name associated with the curve' i get

So maybe it is because there are four screens and a predictCurve and originalCurve on each screen....

[veillette]

Great @pixelzoom, I'm going to unassign myself.

[veillette]

  this.pointsProperty.lazyLink( () => {
      assert && assert( Tandem.PHET_IO_ENABLED, 'pointsProperty may change only in PhET-iO brand' );
      if ( !options.pointsPropertyReadOnly ) {
        this.curveChangedEmitter.emit();
        console.log( `pointsProperty changed ${options.tandem?.name} and ${options.tandem?.parentTandem?.parentTandem?.parentTandem?.name}` );
      }
    } );

yields

So every screen reports on the states of the original and predict curve, which explains why the performance is so poor. This explains 2x4=8.

Ok truly unassigned but I was curious about my hunch.

[pixelzoom]

@veillette for future reference, you can identifiy a PhET-iO element like this:

- console.log( `pointsProperty changed ${options.tandem?.name} and ${options.tandem?.parentTandem?.parentTandem?.parentTandem?.name}` );
+ console.log( `pointsProperty changed ${this.phetioID}` );

... which will result in console output like:

Curve.ts:138 pointsProperty changed: calculusGrapher.derivativeScreen.model.curves.originalCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.derivativeScreen.model.curves.predictCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.integralScreen.model.curves.originalCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.integralScreen.model.curves.predictCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.advancedScreen.model.curves.originalCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.advancedScreen.model.curves.predictCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.labScreen.model.curves.originalCurve
Curve.ts:138 pointsProperty changed: calculusGrapher.labScreen.model.curves.predictCurve

[pixelzoom]

Performance improvements were made in the above commit. Get/set state is now limited to originalCurve.pointsProperty and predictCurve.pointsProperty. While we're investigating further state improvements in #320, this is now ready for review:

@Nancy-Salpepi would you please take another look at this? Close if OK.

Focus on Studio (including the "Test" button) and the State Wrapper, and verify that everything looks OK when you're:

• [ ] directly manipulating f(x)
• [ ] undoing operations with the Undo button
• [ ] pressing the Eraser button
• [ ] resetting the sim with the Reset All button
• [ ] setting originalCurve.pointsProperty via Studio's Get/Set Value buttons

[Nancy-Salpepi]

@pixelzoom responsiveness is still great in Studio/Standard wrapper. I don't see a noticeable difference in State between yesterday and today. Setting is the state is still slower than dev.25.

[pixelzoom]

Thanks @Nancy-Salpepi.

There's nothing else we can do at this time to improve State Wrapper performance. We made a last-ditch effort in #320, and that didn't pan out. Since the PhET-iO team has indicated that performance of the State Wrapper is acceptable, I'm going to close this.