detect Property loops

[pixelzoom]

A Property loop occurs when a Property's set method is entered before a previous call to set exits. PhET-iO makes it necessary to deal with these loops because it results in intermediate/redundant data in the message stream. See for example phetsims/hookes-law#52.

PhetioObject previously was responsible for detecting Property loops, but that was removed in phetsims/tandem#57. And I believe it was @samreid who suggested that this responsibility does not belong in PhET-iO; it belongs in axon.

In phetsims/tandem#57 (comment), I suggested a way to add responsibility (conditionally) to Property, reproduced below. And I found this to be invaluable in troubleshooting phetsims/hookes-law#52. I'd like to see this added to Property.

      // @private
      _notifyListeners: function( oldValue ) {

        // We must short circuit based on tandem here as a guard against the toStateObject calls
        this.tandem.isSuppliedAndEnabled() && this.startEvent( 'model', 'changed', {
          oldValue: this.phetioType.elementType.toStateObject( oldValue ),
          newValue: this.phetioType.elementType.toStateObject( this.get() ),
          units: this.phetioType && this.phetioType.units
        }, this.changeEventOptions );

        // notify listeners, optionally detect loops where this Property is set again before this completes.
        assert && assert( !this.notifying || !phet.chipper.queryParameters.detectPropertyLoops, 
          'Property loop detected, value=' + this.get() + ', oldValue=' + oldValue );
        this.notifying = true;
        this.changedEmitter.emit2( this.get(), oldValue );
        this.notifying = false;

        this.tandem.isSuppliedAndEnabled() && this.endEvent();
      },

[jbphet]

Seems like a good idea to me. I've accidentally set up property loops a number of times, and they often ended up locking up the browser tab due to recursion. The proposal above would have caught the problems earlier and more elegantly.

[zepumph]

This was talked about int he meeting today, but only briefly. I am wondering if we could have a property option that dictates whether or not you want to allow Property.set() re-entrance. I would prefer that to a global that you toggle for everything in a sim at once. I think ideally we would want everything to default to not being re-entrant, such that you have to specify allowReentrantSetting: true for instances that you want to opt in. Since there are many places in the code where that would break with a default of false, perhaps we should go with something else, or use the query parameter until those places have been chipped away.

[samreid]

I think we should add {reentrant: false} as the default for Property.js, with an assertion error when a loop is detected. We would quickly "chip-away" and add reentrant: true for all the errors that immediately pop up.

[pixelzoom]

So...

Add this to options:

  // {boolean}
  // true: reentrant calls to `set` fail an assertion
  // false: reentrant calls to `set`are allowed
  detectRentry: true

Change assertion in _notifyListeners to:

 assert && assert( !this.notifying || !options.detectRentry, 
  'reentry detected, value=' + this.get() + ', oldValue=' + oldValue );

... and deal with assertion failures as they occur in CT (which is sure to be immediately).

Objections?

(EDIT: assertion message changed.)

[samreid]

Sounds good to me, but I'd recommend a key name more like reentrant with a default to false. That seems clearer because we would iterate through marking the ones that we determine are re-entrant.

[pixelzoom]

reentrant is fine with me.

[pixelzoom]

Unless someone else needs this feature for debugging, it might be advisable to defer until I return from vacation.

EDIT by @samreid: devisable => advisable

[jbphet]

I have no urgent need for it at this time.

[samreid]

It seems it would be good to have several developers "on hand" when we flip the switch for this, so responsible developers can catch their own reentrant Properties.

[jessegreenberg]

Discussed at 6/21/18 -

We will add #179 (comment) in a branch, then work on it when developers are available all at once. This will probably be done during core hours. We will do it in a branch so that CT continues to run and we can continue to work in common code in case this can't be fixed quickly. I will create a reminder to do this on July 9th since that is when devs will be back in town.

[pixelzoom]

I will create a reminder to do this on July 9th since that is when devs will be back in town.

On 7/9, @jessegreenberg reminded us about this issue on Slack.

Unless someone else needs it, I would lobby for doing this work after EqEx is in RC, and after I return from vacation, so that I can address my sims. EqEx RC will start Wed 7/18 at the latest. I’ll be on vacation 7/19-8/5.

[jessegreenberg]

More summarized discussion from slack:

Since we will do this on a branch in axon, devs don't all need to be available to work on this at once. We will wait to work on this issue until priority goes up when it is required in Hooke's Law or somewhere else, and sometime after 8/5.

[pixelzoom]

I'm back to working on Hooke's Law PhET-iO, and could really use this to troubleshoot phetsims/hookes-law#52. So I'm going to resume work on this.

[pixelzoom]

Support for this pushed in the above commit. New option reentrant determines whether reentrant calls to set are allowed. It’s default is currently true, but we should (re)discuss the value/impact of making this false.

In the meantime, I'm going to explicitly set reentrant: false in hooks-law to verify that I've resolved phetsims/hookes-law#52.

[pixelzoom]

@zepumph has been randomly selected to review my work.

[zepumph]

That looks good to me. At first I was surprised that it would be an assertion rather than just a noop, but I think that is smart. Back to you.

[pixelzoom]

After using this to identify cycles (reentrant calls to Property set) in phetsims/hookes-law#52, I'm inclined to leave the default as reentrant: true, and use this as a tool to identify cycles, not a requirement that all sims should be cycle-free. For example, there are corner cases in Hooke's Law where floating-point error causes a value's range to be exceeded, and there's not much choice other than to clamp to the range and cause a cycle.

[samreid]

Why not put the default as reentrant: false so that all of the re-entrant Properties must be labeled as such. How did you identify cycles if the default is reentrant: false? Or were you imagining a temporary code change in axon during testing?

[pixelzoom]

@samreid wrote:

Why not put the default as reentrant: false so that all of the re-entrant Properties must be labeled as such.

I haven't confirmed, but I think this might create a lot of work.

How did you identify cycles if the default is reentrant: false?

The default is reentrant: true. As mentioned in #179 (comment), I set reentrant: false for my model Properties (view Properties don't seem to have this problem). I started with all model Properties, then narrowed it down to model Properties that are instrumented for PhET-iO. This might not be scalable if you have a large number of instrumented Properties.

Or were you imagining a temporary code change in axon during testing?

Could do. Or a query parameter. I currently have a boolean reentrant query parameter in Hooke's Law that lets me set reentrant: false for the Properties that I was profiling.

[samreid]

I temporarily set Property.options.reentrant to default to false and ran a set of fuzz testing. I observed that the vast majority of property instances do not have a problem with reentrance. I saw 3 patterns for values that did have a problem with reentrance, here are examples of each of the three types:

value=0.9999999999999998, oldValue=1 // epsilon problem
value=-0.4277580409572783, oldValue=-0.5 // large delta problem
value=[Object], oldValue=[Object] // object changed to object problem.  May be same object?

Part of the documentation for Property.options.reentrant is:

Update cycles may be due to floating point error, faulty logic, etc.

I would like developers to be made aware of re-entrance during development, and for us to annotate all re-entrant Properties with reentrant: true so that maintainers will know that's an issue for that Property. Using ?fuzzMouse, I visited each sim and marked properties as reentrant: true as appropriate. I had to mark 25 sites to get every simulation to pass fuzz mouse at least twice. Bayes CT will probably pick up other cases that were missed during these fuzz sessions. I think we should catch them as they come up and mark them as such.

Based on the results of this investigation, I'll commit my working copy changes, including changing Property's default to reentrant: false and the 25 occurrences of reentrant: true for specific Property instances.

For future steps, it would be nice to (a) add code comments to each re-entrant Property indicating why they are re-entrant, or (b) implement it without re-entrance. I'll add a step to the code review document for that.

[pixelzoom]

As of 8/22/2018, 1:59:44pm, gravity-and-orbits is still failing with "Assertion failed: reentry detected" in CT.

[samreid]

I pushed a fix for the gravity and orbits problem that occurred on Bayes CT.

[samreid]

At today's meeting, we decided to create issues for the reentrant: true cases. I'll tackle this.

[pixelzoom]

@samreid FYI, I believe that I've already created issues for all sims that I'm responsible for. E.g. phetsims/acid-base-solutions#155.

[samreid]

Thanks @pixelzoom, I elevated priority to remind me to tackle the remainder sooner rather than later.

[samreid]

I created issues for the remaining cases. @pixelzoom can this issue be closed?