Closed samreid closed 4 years ago
For initial investigation, I tried adding a separate channel for DerivedProperties, so they can be updated before any notifications are sent out. I tested in the unit test harness.
Before Changes:
setting a to 5
a updated 5+2=3
c updated 5+2=7
After Changes:
setting a to 5
a updated 5+2=7
c updated 5+2=7
My implementation is missing improved Property.dispose
to get rid of the derived properties, and I haven't tested that it is fully general to an arbitrary tree size, or if there is a simpler implementation (maybe DerivedProperty doesn't need to link
at all).
@zepumph do you think this idea would address the problems we discussed in https://github.com/phetsims/molarity/issues/189#issuecomment-570748713 ?
I also need to fully read through https://github.com/phetsims/axon/issues/209 to see if this pattern addresses any of those problems.
UPDATE: Also, the proposed solution doesn't properly handle oldValue
in DerivedProperty notifications.
The problem described in https://github.com/phetsims/phet-io-wrappers/issues/229#issue-397432747 is not between a Property and a chain of DerivedProperties, so this solution is insufficient to solve that problem. setDeferred
still works for that problem.
Also, if we know we have to keep setDeferred
, should this feature (of getting DerivedProperty values right) be implemented using that feature?
I have a calendar event set for Feb 5.
Excellent, thanks!
Today @pixelzoom and @chrisklus and I discussed this problem in the context of https://github.com/phetsims/gas-properties/issues/182
We tried storing all of the DerivedProperty instances globally, then after all Property instances had taken their final value (but before notifying listeners), we defer DerivedProperties, and update their values.
This addressed at least one problem in Gas Properties.
We thought this might be more robust than the patch above because the patch above is too granular and may send out change notifications at the wrong times?
This would be good to discuss with @zepumph
I am happy to discuss. I poked around with the patch, but I wasn't quite able to understand how this might apply in a general, commitable way (without the global). It also isn't totally clear to me how this applies to phet-io state set vs whenever any Property value is set, and what the desired behavior is for those cases. Let's chat!
@samreid and I discussed this more today, and he brought me up to speed on the above two patched. We decided that it made the most sense to try to tackle this in a general way in axon instead of something specific to the phet-io state engine. As a result we started from the original patch and then added to it.
@samreid I worked further on this, mainly with tests. There are a few TODOs with open questions, especially as it pertains to the "deferred" derived Property test. Take a look and let me know what you think.
Fuzzing showed a few assertions in diffusion and gas-properties that looked like ". . . has not been populated yet." That seemed worrisome, and I didn't quite understand them. Otherwise tests are running (not like that is the end of the story or anything).
This is all I can do for today:
Here are the errors in phet brand:
@zepumph discussed this a bit more, particularly about how to make sure the Property listeners are notified before the DerivedProperty listeners. Here's a patch which may or may not be helpful! We are struggling to get all of these features working in concert:
I considered whether Multilink
should take precedence like we are discussing for DerivedProperty
. It seems many occurrences of Multilink
may be better off using DerivedProperty
, such as:
// Only show the AngleNode when it is selected via a checkbox and the laser is on
Property.multilink( [ showAnglesProperty, laserOnProperty ], ( showAngles, laserOn ) => {
this.visible = showAngles && laserOn;
} );
May be written as:
new DerivedProperty( [ showAnglesProperty, laserOnProperty ], ( showAngles, laserOn ) =>
showAngles && laserOn
).linkAttribute( this, 'visible' )
On second thought, that doesn't seem like a clear win.
It seems like many occurrences of Multilink
are treated like Property
listeners, and may not need precedence. But I'm not sure how this should be decided. One other possibility would be if they use the same infrastructure, perhaps we could share code between DerivedProperty
and Multilink
.,
Is it important that the Property keeps its current value when changing to a deferred value? If not, maybe it would be simpler if both Property and DerivedProperty could mark dirty=true
if they take a value that doesn't match their current value, and they have not sent notifications yet. But I'm wondering if PhET-iO requires the Property to keep its old value. But there's another issue about not reading from Property
at all while it is deferred. Maybe we would have the same rule for dirty
?
Maybe we should discuss https://github.com/phetsims/axon/issues/281 further before proceeding on this issue?
I worked on this more today, with a new philosophy:
This significantly cleaned up the setDeferred
implementation (even though it changes the semantics), and gives us control over the listener notifications.
Things are mostly working, but several simulations have a problem in PaintColorProperty disposal:
testSims=build-a-fraction,build-an-atom,diffusion,expression-exchange,fraction-matcher,fractions-equality,fractions-intro,fractions-mixed-numbers,gas-properties,gases-intro,isotopes-and-atomic-mass,scenery-phet,sun,unit-rates
I've also lost the ability to error out on a re-entrant Property, but we may be able to restore that, if it's still desirable.
Patch:
Another idea that may help. At the end of _notifyListeners
check if the current value has changed. If so, run _notifyListeners
again. I'm also curious if a Property should notify for every single value change, or if some (such as re-entrant ones) should be "batched" and omitted. But I wonder if the batching/skipping is causing the PaintColorProperty disposal problem. It seems Property in master has an implicit list of new values, and notifies for every single value. I'm not sure how to reconcile that with the proposal, unless we make the new values explicit (say in a list or queue).
I'm assigned here, but I don't have a good concept of what to do. It seems like we will need to pair to bring us to a commit. I have questions about some of the above. I'm not clear about how to handle re-entrant Properties, but I would lean towards doing the exact same thing as we do now, and perhaps change that, if desired, in another change set.
I think in general Multlink should not be handled the same, since it is more generally just following the traditional "listener" pattern. A lack of value attached to the Multilink is key there, and I don't think we should change it as brainstormed in https://github.com/phetsims/axon/issues/276#issuecomment-605518935
@samreid want to pair on this sometime soon and try to finish it up?
Today @samreid and I worked on this for many hours. Here is a bit how the day went:
It would be nice to solve this generally in axon. What if DerivedProperty
values are changed before all notifications go out. The issue is that this isn't just a problem with DerivedProperties, but listener order in general through "intermediate states." Anytime there is a Multilink
and you need to change two dependencies, it goes through an intermediate state.
This is actually an issue with how Gas Properties' model has a listener order dependency. In the sim and normal user interface, it is impossible to trigger the wrong listener order (and in many cases assertions make sure of that). We can trigger a problem like ("lightParticles has not been populated yet") with ?shuffleListeners
to demonstrate this issue. Note that this issue is also found without ?shuffleListeners
when running in the state wrapper.
PhetioStateEngine
is an order of magnitude harder to solve that ?shuffleListeners
because any Properties listeners can be called before any other. Let's say we have the following listener structure for Properties. A has a listeners that changes B, B has a listener that changes C, (i.e. A=>B=>C). ?shuffleListeners
would still keep that ordering where A listeners fire before B and so on. But the state engine fires listeners in any order.
What if we create a way to set order dependency in PhetioStateEngine
? With not that much code we could add this relationship, and also make that happen automatically for DerivedProperties. Let's call this changeset "state set order dependencies."
Index: axon/js/DerivedProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- axon/js/DerivedProperty.js (revision e9fa76fdf8f3e0a6fdb74e57eb5a97d495b34b5b)
+++ axon/js/DerivedProperty.js (date 1586831785944)
@@ -63,6 +63,13 @@
};
self.dependencyListeners.push( listener );
dependency.lazyLink( listener );
+
+ // register that for PhET-iO state, each dependency should be set (and therefore have its listeners called) before
+ // the DerivedProperty. NOTE: in practice right now all are undefered before calling the first dependency so the
+ // order in which listeners call will likely be: dependencyA, derivedProperty, dependencyB, derivedProperty, etc. . .
+ if ( Tandem.PHET_IO_ENABLED && this.isPhetioInstrumented() && dependency.isPhetioInstrumented() ) {
+ phet.phetio.phetioEngine.phetioStateEngine.registerOrderDependency( dependency, this );
+ }
} )( dependency, i );
}
}
Index: phet-io/js/PhetioStateEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- phet-io/js/PhetioStateEngine.js (revision 470d0fd156f24c6c7c3cd7373616336c5ca5571a)
+++ phet-io/js/PhetioStateEngine.js (date 1586831785939)
@@ -53,6 +53,10 @@
// @private {PhetioState|null} - keep track of the most recent state set to the sim. This is used to restore to
// that state for the "reset all" case.
this.mostRecentSavedState = null;
+
+ // {Object.<string,PhetioObject>} - where keys are the phetioIDs. For each key, every value must be set before the
+ // key can be set.
+ this.beforeMap = {};
}
/**
@@ -168,12 +172,15 @@
let currentPhetioIDs = _.keys( state ).filter( key => key.indexOf( phetioIDPrefix ) === 0 );
let passIndex = 0;
+ // {Object.<phetioID:string,boolean>} - a list of all phetioIDs that have successfully had their state set.
+ const completeMap = {};
+
// Run as many passes as necessary to set all parts of the state. This assumes all operations are commutative.
while ( currentPhetioIDs.length > 0 ) {
if ( passIndex > 5 ) {
console.log( 'pass: ' + passIndex + ', keys.length = ' + currentPhetioIDs.length + ', keys = ' + currentPhetioIDs );
}
- currentPhetioIDs = this.iterate( currentPhetioIDs, state, actions, stateSetOnceEmitter );
+ currentPhetioIDs = this.iterate( currentPhetioIDs, state, actions, stateSetOnceEmitter, completeMap );
passIndex++;
}
};
@@ -240,35 +247,42 @@
* @returns {string[]} - list of phetioIDs for the next pass
* @private
*/
- iterate( currentPhetioIDs, state, actions, stateSetOnceEmitter ) {
+ iterate( currentPhetioIDs, state, actions, stateSetOnceEmitter, stateCompleteMap ) {
let changed = false;
const nextPass = [];
currentPhetioIDs.forEach( phetioID => {
- try {
- let phetioObject = null;
+ if ( !this.isValid( stateCompleteMap, phetioID ) ) {
+ nextPass.push( phetioID );
+ }
+ else {
+
+ try {
+ let phetioObject = null;
- // If we already have a reference to the PhetioObject
- if ( this.phetioEngine.phetioObjectMap.hasOwnProperty( phetioID ) ) {
- phetioObject = this.phetioEngine.getPhetioObject( phetioID );
- }
- else {
+ // If we already have a reference to the PhetioObject
+ if ( this.phetioEngine.phetioObjectMap.hasOwnProperty( phetioID ) ) {
+ phetioObject = this.phetioEngine.getPhetioObject( phetioID );
+ }
+ else {
- // If we don't have a reference to the PhetioObject, create one. Could throw CouldNotYetDeserializeError.
- phetioObject = this.create( state, phetioID );
- }
+ // If we don't have a reference to the PhetioObject, create one. Could throw CouldNotYetDeserializeError.
+ phetioObject = this.create( state, phetioID );
+ }
- // Could throw CouldNotYetDeserializeError
- this.setStateForPhetioObject( phetioObject, state, actions, stateSetOnceEmitter );
- changed = true;
- }
- catch( e ) {
- if ( e instanceof CouldNotYetDeserializeError || e.message === 'CouldNotYetDeserializeError' ) {
+ // Could throw CouldNotYetDeserializeError
+ this.setStateForPhetioObject( phetioObject, state, actions, stateSetOnceEmitter );
+ stateCompleteMap[ phetioID ] = true;
+ changed = true;
+ }
+ catch( e ) {
+ if ( e instanceof CouldNotYetDeserializeError || e.message === 'CouldNotYetDeserializeError' ) {
- // If the object couldn't be created (if a dependency didn't exist yet), then wait for the next pass.
- nextPass.push( phetioID );
- }
- else { throw e; }
+ // If the object couldn't be created (if a dependency didn't exist yet), then wait for the next pass.
+ nextPass.push( phetioID );
+ }
+ else { throw e; }
+ }
}
} );
@@ -312,7 +326,10 @@
// only do this if the PhetioObject is already not deferred
if ( phetioObject.setDeferred && !phetioObject.isDeferred ) {
phetioObject.setDeferred( true );
- stateSetOnceEmitter.addListener( () => actions.push( phetioObject.setDeferred( false ) ) );
+ stateSetOnceEmitter.addListener( () => {
+ const potentialAction = phetioObject.setDeferred( false );
+ potentialAction && actions.push( potentialAction );
+ } );
}
type.setValue && type.setValue( phetioObject, value );
}
@@ -427,6 +444,36 @@
}
}
+ /**
+ * TODO: rename
+ * A function that determines if a phetioID is valid and ready for state setting
+ * @private
+ *
+ * @param {Object.<phetioID:string,boolean>}completeMap
+ * @param phetioID
+ * @returns {boolean}
+ */
+ isValid( completeMap, phetioID ) {
+ if ( this.beforeMap[ phetioID ] ) {
+ return _.every( this.beforeMap[ phetioID ], dependency => completeMap[ dependency.tandem.phetioID ] );
+ }
+ return true;
+ }
+
+
+ /**
+ * Register that one PhetioObject must be set for state before another one
+ * @public
+ *
+ * @param {PhetioObject} firstPhetioObject - the object that must be set before the second
+ * @param {PhetioObject} secondPhetioObject
+ */
+ registerOrderDependency( firstPhetioObject, secondPhetioObject ) {
+ // TODO: assert both are instrumented
+ this.beforeMap[ secondPhetioObject.tandem.phetioID ] = [] || this.beforeMap[ secondPhetioObject.tandem.phetioID ];
+ this.beforeMap[ secondPhetioObject.tandem.phetioID ].push( firstPhetioObject );
+ }
+
/**
* Capture and emit initial state, and, if specified via query parameters, listen for simulation frames to emit
* additional states. Should only be called once per instance.
Then we find that we also have to specifically undefer in that order also. Take this for example:
xHolderArray = [];
yHolderArray = [];
xNumberProperty = new Property( 2 )
yNumberProperty = new Property( 1 )
xNumberProperty.link( numberOfX => { xHolderArray = new Array( numberOfX) )
yNumberProperty.link( numberOfY => { yHolderArray = new Array( numberOfY) )
totalNumberProperty = new DerivedProperty( [xNumberProperty, yNumberProperty], (x,y)=> x+y);
totalNumberProperty.link(()=>{
assert( xHolderArray.length === xNumberProperty.value );
assert( yHolderArray.length === yNumberProperty.value );
} );
// constraints
xNumberProperty and yNumberProperty must have the first listener called before totalNuberProperty listener gets called
// solved by:
undefer xNumberProperty
undefer yNumberProperty
notify xNumberProperty
notify yNumberPropertyListeners
undefer totalNumberProperty
notify totalNubmerProperty
Note that a primary way to solve this would be to instrument the arrays also. Then their values will already be correct by the time listeners first. In general, @samreid and I are finding most of the difficultly when listeners cause other Properties to change their value.
Now that we have the patch defined in (4), go through GasProperties and try to list out the order dependencies. This didn't work very well! Either we run into the bug in (5), where one fix was to try to undefer based on the dependency order. Undeferring in batches (buggy!). Here is a case where that algorithm doesn't work:
aProperty = new Property(2)
bProperty = new Property(1)
aProperty.link(a=>{
assert( a>bProperty.value);
})
bProperty.link( b=>{
assert( b<aProperty.value);
})
// constaints
a.value must be right before calling b listeners
b.value must be right before calling a listeners
// solved by the original state set undeferring algorithm:
undefer a
undefer b
notify a
notify b
After all that! I tried to just fix Gas Properties with the mentality that its model is imperfect because it contains listener orders (which is reasonable to think that many sims do!). Thus we can apply workarounds in the PhET-iO state set case such that the assertions trying to guard against listener order don't fire. This was my least favorite approach, and one that I'm not that interested in generally, but I want to note that after the below patch, which involved two state set guards, one Property instrumentation (plus making it reentrant:true), and one total hack (see ParticleSystem.addParticle
), the sim successfully fuzzes without erroring in phet brand, phet-io brand, and state fuzzing (after multiple minutes of fuzz). I think we can do better on the temperature problem.
Note that no changes are needed in regards to (4) and the state engine for these gas properties changes.
If this was my sim, I would have committed parts of this, as it is at least 4 steps closer to a working state wrapper. Many of them may feel like workarounds, but I think that this point is arguable, since much of them are based on the fact that we are working with a model that contains listener order dependencies that we need to try to configure with a state engine that expects order independence.
It also isn't totally clear that declaring order dependencies to the state engine is the cleanest solution. At the very least it feels like duplicating the same logic in two spots. For examples:
const myProperty = new Property( 2 );
const myOtherProperty = new Property( 5 );
myProperty.link( value => {
myOtherProeprty.value = calculateBasedOnOtherValue( value );
} );
myOtherProperty.link( ()=>{
assert( iBetterBeCalledAfterTheAboveListener);
})
phet.phetio.phetioEngine.phetioStateEngine.registerOrderDependency( myProperty, myOtherProperty );
I can see a future for the state engine code in DerivedProperty.js, but couldn't find any places where it fixed a problem.
Over to you @samreid for any questions.
If this was my sim, I would have committed parts of this, as it is at least 4 steps closer to a working state wrapper.
Re the patch for (7) in https://github.com/phetsims/axon/issues/276#issuecomment-613198365... I added the suggested instrumentation of BaseContainer, after fixing some problems that the patch would have introduced (e.g. crashing the sim when chaining widthProperty
in Studio). As for the other changes in the patch, I'm not at all a fan of phet.joist.sim.isSettingPhetioStateProperty
-- it feels like a hack, a last resort, and I'd like to avoid that approach if at all possible. And its only necessary because:
Many of them may feel like workarounds, but I think that this point is arguable, since much of them are based on the fact that we are working with a model that contains listener order dependencies that we need to try to configure with a state engine that expects order independence.
PhET sims are not order independent. Basing a state engine based on that requirement is a mistake.
A reminder that ?shuffleListeners
was intended to identify order dependencies so that they could be reworked if necessary, documented if not. It was not intended to eliminate order dependencies, nor has there ever been a goal to have total order independence. And as you said elsewhere in this comment, there are undoubtedly many sims that have some form of order dependency. (At least in this Gas Properties, they are documented and verified with assertions.) Furthermore, total order independence is (I would argue) unnecessary, undesirable, and in some cases so difficult as to be impractical/impossible.
A state engine that expects order independence is a bad match for PhET sims. It doesn't match the reality of how sims are implemented. And changing sims to match the needs of the state engine will be (and has been) not only expensive, but will also make it more difficult to write sims.
I investigated a new constraints strategy that gives more fine-grained control over when "undefer" and "notify" should happen relative to each other. It also removes assertions from one DerivedProperty derivation in gas-properties, with a note about why. That may be important in how this is working and I should test that in isolation. This is likely overcomplicated but it fuzzed for 30+ minutes on Gas Properties without an error. I wanted to store a record of it before I continue investigation.
UPDATE: Eventually I did hit a fuzzing error in Gas Properties here, but I noticed this fuzz error is in the source:
This patch is essentially a cleaned up version of the prior comment. It's fuzzing well in Gas Properties and Projectile Motion.
The main idea from the patch is splitting out the 2 primary phases of Property notification.
undefer
- the point at which it is undeferred and takes its new valuenotify
- the point at which the Property notifies its listeners of the new valueThe patch introduces an API call like the one in https://github.com/phetsims/axon/issues/276#issuecomment-613198365 but with the ability to specify relative order dependence between undefer
and notify
.
For example, I added the two bottom lines from this snippet in IdealGasLawModel:
// Verify that we're not in a bad 'Hold Constant' state.
assert && this.holdConstantProperty.link( holdConstant => {
// values that are incompatible with an empty container
assert && assert( !( this.particleSystem.numberOfParticlesProperty.value === 0 &&
( holdConstant === HoldConstant.TEMPERATURE ||
holdConstant === HoldConstant.PRESSURE_T ||
holdConstant === HoldConstant.PRESSURE_V ) ),
`bad holdConstant state: ${holdConstant} with numberOfParticles=${this.particleSystem.numberOfParticlesProperty.value}` );
// values that are incompatible with zero pressure
assert && assert( !( this.pressureModel.pressureProperty.value === 0 &&
( holdConstant === HoldConstant.PRESSURE_V ||
holdConstant === HoldConstant.PRESSURE_T ) ),
`bad holdConstant state: ${holdConstant} with pressure=${this.pressureModel.pressureProperty.value}` );
} );
// Make sure the values in the holdConstantProperty have the correct values before calling the holdConstantProperty listeners
Tandem.registerOrderDependency( this.particleSystem.numberOfParticlesProperty, 'notify', this.holdConstantProperty, 'notify' );
Tandem.registerOrderDependency( this.pressureModel.pressureProperty, 'notify', this.holdConstantProperty, 'notify' );
And DerivedProperty uses this to make sure that (a) Dependencies should notify before the DerivedProperty undefers, so it will be sure to have the right value. (b) All dependencies should undefer before any of them notify (I'm not 100% sure this one is necessary since DerivedProperty caches dependency values and will still go through thrashing)
Next step is to discuss with @zepumph and @pixelzoom.
Looks promising, looking forward to discussing. I realize that the above patch is a proposal, but a few things about the API for the discussion agenda:
Inconsistent terminology and parameter names. You described 'undefer' and 'notify' as "phases" in the previous comment, and that made sense to me. Then the API threw me a curveball, didn't use "phases", and instead uses 2 other names. It refers to "terms" in Tandem.js (beforeTerm
, afterTerm
) and "actions" in PhetioStateEngine.js (firstAction
, secondAction
). The API also uses "before" and "after" (Tandem.js) interchangeably with "first" and "second" (PhetioStateEngine.js). Since Tandem.registerOrderDependency
delegates to PhetioStateEngine.registerOrderDependency
, can the terminology and parameter names be the same throughout the API?
'undefer' and 'notify' are an appropriate place to use Enumeration
rather than string literals.
I'm not clear on what the default behavior is for this feature. Is there a set of args to Tandem.registerOrderDependency
that correspond to the default behavior? Maybe I missed it in the patch, but it should be documented somewhere.
I agree it will be nice to unify terminology and use Enumeration
. The behavior in master is to do all undefer
as a batch in arbitrary order, then do all notify
as a batch in arbitrary order.
stateSetOnceEmitter.emit(); // <-- does all of the undefers
actions.forEach( action => action && action() ); // <-- does all the notifies
The patch enforces that undefer must happen before notify
// Undefer must happen before notify
if ( action === 'notify' ) {
if ( !isComplete( phetioID, 'undefer' ) ) {
return false;
}
}
and that specified constraints are met, otherwise the order is arbitrary.
Thanks for clarifying. Sounds like the next step is a discussion. @samreid will you schedule?
@pixelzoom, and @samreid and I discussed this issue more today. Then @samreid and I worked further on the above patch. This is as far as I got, though I thought it was close to commit (as it is working in gas properties), there is an order dependency infinite loop in ph-scale, so here is more of a patch:
Here is a summary of the meeting notes and further work, much of which was added in the patch:
Tools in the toolbox for phet-io state
RE this list above: It is purposefully ordered from ideal to least ideal. In gas properties for https://github.com/phetsims/gas-properties/issues/182 we were had to go with (3) because it didn't make sense enough to alter the sim code itself, but it was much preferred over linking to the setting state Property.
I will keep working with @samreid.
After some investigation into the ph-scale problem with @samreid, I logged the directionality of the order dependencies into an online graph visualization tool. This is what came up.
Note that we lose the granularity of 'notify' vs 'undefer', but it helps us reevaluate if these automatic order depedency registrations are actually possible to add generally
@samreid had the great idea of looking at each phase/id combo as a node. Look at the marked doubled arrowed edge. That seems buggy:
Here is the debug code added to make this happen:
const stillTODOIDs = undeferList.concat( notifyList ).map( item => item.phetioID )
.filter( id => id.startsWith( 'phScale.microScreen.model.solution' ) )
const relevantConstraints = this.constraints.filter( constraint => {
return stillTODOIDs.indexOf( constraint[ 0 ] ) >= 0 ||
stillTODOIDs.indexOf( constraint[ 2 ] ) >= 0;
} );
// console.log( relevantConstraints );
// relevantConstraints.forEach( t => console.log( t[ 0 ], t[ 1 ].name, '\t\t\t', t[ 2 ], t[ 3 ].name ) );
let string = '';
const map = x => x.replace( 'phScale.microScreen.model.solution.', '' );
relevantConstraints.forEach( t => { string += `${map( t[ 0 ] )}${t[ 1 ].name}\t${map( t[ 2 ] )}${t[ 3 ].name}\n`; } );
console.log( string );
Now @samreid and I are having fun. Here is an image of the sim once we remove the buggy dependency registration between all of a DerivedProperty's dependencies:
Then we set up a way to just view derived Property dependencies outside the context of PhET-iO state. Here is a picture of this relationship in Ph-scale (so cool!)
We are happy to find that the ph-scale bug is fixed by removing dependency registration between all of a derivedProperty's dependencies. This isn't needed especially after we add better support for deferring DerivedProperty. Commits coming soon.
UPDATE: thanks https://csacademy.com/app/graph_editor/
I ended up committing the code because things were looking very promising! We now have a more explicit way of detecting order dependency issues in the state engine.
Summary of changes:
DerivedProperty
in the state engine (and in general). This fixed https://github.com/phetsims/gas-properties/issues/178 because it changed logic in dependency listeners such that we only call the derivation function once when we are about to become undeferred.MeasuringTapeNode
. Note that it links to the DerivedProperty and to some of its dependencies. Perhaps another way to fix this would be to remove the basePositionProperty and tipPositionProperty from that multilink (since they are already dependencies to measuredDistanceProperty). I'm not worried about it, because it still makes sense. In the general case I can't think of why listeners need to fire first for dependencies when the DerivedProperty will still be recalling the derivation function. There are a few TODOs that need to be done at a high priority:
phase
field.Phase
the enum lives in AXON/Property. That is a bit weird, but I think it is best. @samreid please review the commits, and the above TODOs at a high priority.
There are a bunch of new Errors in CT, "Impossible set state: from undeferAndNotifyProperties; ordering constaints cannot be satisfied". Are they related to these changes?
For example:
molarity : phet-io-state-fuzz : require.js : run
https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io-wrappers/state/?sim=molarity&phetioDebug&fuzz&postMessageToParent&postMessageOnLoad&postMessageOnError&postMessageOnBeforeUnload
Uncaught Error: Assertion failed: Impossible set state: from undeferAndNotifyProperties; ordering constaints cannot be satisfied
Error: Assertion failed: Impossible set state: from undeferAndNotifyProperties; ordering constaints cannot be satisfied
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/assert/js/assert.js:22:13)
at PhetioStateEngine.errorImpossibleSetState (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/PhetioStateEngine.js:391:15)
at PhetioStateEngine.undeferAndNotifyProperties (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/PhetioStateEngine.js:276:12)
at PhetioStateEngine.setState (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/PhetioStateEngine.js:182:10)
at PhetioEngineIO.implementation (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/PhetioEngineIO.js:200:43)
at PhetioCommandProcessor.processCommand (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/phetioCommandProcessor.js:266:51)
at https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/phetioCommandProcessor.js:160:36
at Array.map (<anonymous>)
at PhetioCommandProcessor.processCommands (https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/phetioCommandProcessor.js:158:30)
at https://bayes.colorado.edu/continuous-testing/snapshot-1587622935351/phet-io/js/phetioCommandProcessor.js:99:16
id: Bayes Chrome
Approximately 4/23/2020, 12:22:15 AM
This caused many errors as @pixelzoom mentioned above, which I'm surprised to see and bummed since before committing I fuzzed 4 state wrappers (just none that had this trouble). Here is the list of sims to handle:
Sorry to hear that this created new problems. Also note that this did not fix one of the Gas Properties problems, and created a new variation on that problem, see https://github.com/phetsims/gas-properties/issues/178#issuecomment-618476464
Here are the problemed constraints for EFAC (blocks) and RIAW (sound related):
For efac the problem seems to be that things are waiting on archetypes to get state set first. We should update that so that we don't add constraints about archetypes.
Above I am ignoring dependencies that try to be registered on dynamic element archetypes. This fixed the EFAC bug. I don't like how I am doing it though, so I added a TODO to this issue.
For sims that have sound enabled in them, there is a constraint placed on Sim.activeProperty. In the state wrapper this Property is removed from state in the downstream sim so that it is never made active. This meant that a constraint that was dependent on the activeProperty would never be fulfilled/completed. The above makes sure that the state engine only cares about the constraints that apply to the state that is actually being passed in. This will also solve (untested) problems with setting state on the reset all button, because that only takes a certain subtree of state and sets that.
After the above commit, MAL and RIAW are fixed, I'm not sure what other sims this applied to, as state tests have not been run on CT in some hours. I'll keep working my way down the above list.
@chrisklus helped me track down that actually all the above sims were just a single bug. The issue was that there was an order dependency registered about a phetioID that wasn't set in state.
In EFAC that was a beakerGroup archetype associated with a multilink in BeakerContainerView.
In all other sims it was because soundManager.enabledProperty and browserTabVisibleProperty depends on the sim.activeProperty, (via a multilink in soundManager). But in the state wrapper we strip out the activeProperty from the state so that it doesn't change the downstream sim.
The solution to both is to just ignore any registered order dependency if both phetioIDs in it are not being set in the state for that setState
call. The code was written at the top of undeferAndNotifyProperties
and seems to be working well! I'm excited to check back after full runs tomorrow to make sure things are working on CT as expected.
We also were then able to fix https://github.com/phetsims/gas-properties/issues/178, and I explained to @chrisklus about the deferred logic changes to DerivedProperty.
We are excited with the current state of things. I'm looking forward to seeing if all bugs are squashed on CT tonight, and will check back in the morning.
There are still checkboxes from https://github.com/phetsims/axon/issues/276#issuecomment-618106191 to discuss, and @chrisklus and I really want to do some more performance modifications. There are many nested loops currently, and it would be nice to have this as fast as possible.
Some in-progress changes for unregistering order depenencies and getting some tests going that @zepumph and I started this morning:
All checkboxes have been done above to a basic extent. Here are the final TODOs:
undeferAndNotifyProperties
with a performance-optimization mindset. We will keep at it!I did some refactoring, and doc improvements. This includes separating out sub functions into their own methods. We did that for setState
methods like iterate
and setStateForObject
and it worked well, so I think it should be done here.
In https://github.com/phetsims/phet-io/commit/0275c6f1b7baabe1ceef7f50bb282473b3c6d319 I changed the algorithm a bit. I eagerly removed any PhaseCallback instances that don't have a listener. This likely isn't much of an improvement in many cases, but generally it felt good to never have anything waiting on a noop function to be called as an order dependency.
@chrisklus can you please take a look at the above changes, and then take one more pass through this new code. Let me know if any other potential improvements expose themselves to you. I'm happy to talk through anything else before close.
Thanks @zepumph! Agreed that eagerly removing any with null listeners is better than assigning them noop functions.
I added arguments and docs to errorInUndeferAndNotifyStep
in the commit above. Everything else is looking good to me, back to you.
I accidentally committed something I was using to test my fix in https://github.com/phetsims/phet-io/commit/ea11e64f3c9b0e0afe75ea6c106d83d4dea3725a and it broke phet-io sims on CT, reverted above.
Thanks @zepumph! Agreed that eagerly removing any with null listeners is better than assigning them noop functions.
Is the following use case problematic?
otherProperty -> property1 -> property2 -> property3.
Note here that property3 implicitly has an order dependency on property1. Now let's say that property2 has a null listener. This means that property2 will be completed immediately, potentially before property1, and thus property3 may complete a phase before property1.
The question I have is, "if this case is buggy because property3 really should come after property1, then shouldn't property3 declare that as an order dependency?"
We could call it, and just make a decision to favor clarity and readability, removing the short circuit for no-op listeners. That could save us a potential gotcha in the future. That said it feels like it could be a significant speed up (though I haven't profiled it).
Any more thoughts?
Why was the short-circuit for no-op listeners introduced/proposed? In the example in the preceding comment, it seems like property3 should really come after property1. And property3 shouldn't have to declare an explicit dependency on property1, it should happen implicitly through the transitive relationship via property2.
UPDATE: I now see the explanation in https://github.com/phetsims/axon/issues/276#issuecomment-622167980. But I can't tell if that rule prevents the transitive behavior we described.
Can we set up a test to determine if that case is problematic? It seems like two possible next steps for this issue are that (to see if the short-circuit is bug-free), or to profile some use cases during setState to see if there is a noticeable performance increase when using the short-circuit. I'm up to pair on this.
I think the best way forward is to bring back the safer logic, and continue on with other issues. If speed ends up being a problem for the state engine, I believe that other solutions with have a better cost/benefit. For example https://github.com/phetsims/phet-io/issues/1658.
Bringing it back now unless there are objections.
Implemented above. Anything else for this issue?
I think this is ready to close.
I'd love a final glance by @chrisklus if he thinks it needs it. Anything else here? Did we fully optimize the new code?
Please note that this is not the end of this issue. We supported Property well here, but Emitters and ObservableArray listeners are still on the table in https://github.com/phetsims/phet-io/issues/1634 and https://github.com/phetsims/phet-io/issues/1661 respectively. I'm excited to pick that up over there!
I think it's looking good. Any possible optimizations I see feel minor and insignificant. Looking forward to getting the other implementations going. Ready to close!
Reopening, I found a TODO in Property:
* TODO: add a deregistrations, https://github.com/phetsims/axon/issues/276
From https://github.com/phetsims/molarity/issues/189#issuecomment-570748713