PropertyStateHandler unregisterOrderDependenciesForProperty is a performance bottleneck #316
Assignees
Comments
This was referenced Jul 28, 2020
|
There are 7000 Properties being cleared when we press Start Over, this leads to N^2 of 49,000,000. So we need a strategy or data structure that doesn't iterate over every Property when one Property is removed. Perhaps a pair of Maps? Or one Set and a Map? For instance, instead of a flat list |
|
I set out to try // Key = before, value = after
this.undeferBeforeUndeferMap = new Map();
this.undeferBeforeNotifyMap = new Map();
this.notifyBeforeUndeferMap = new Map();
this.notifyBeforeNotifyMap = new Map();While this would have fast O(1) lookups for keys, it will still have O(N) lookups for values. Do we need to have forward maps and reverse maps? Adding to Wednesday agenda for further discussion, but if there is a clearer direction before then, I can take further steps. |
|
This was identified by @zepumph as relevant to phetsims/natural-selection#140. And it's hoped that Natural Selection will be in dev testing by end of August 2020. |
|
Today @samreid, @chrisklus and I discussed this a bit more:
|
To get O(1) we should only be "looking up" Map keys. The reason we need Map values to be Sets is to get O(1) time in removal. |
This was referenced Aug 12, 2020
|
I worked on this all afternoon, and got all of the tests working, there are still console errors, but ran out of time. Here is my patch! Index: js/PropertyStateHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandler.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/PropertyStateHandler.js (date 1597282717866)
@@ -33,9 +33,74 @@
this.propertyOrderDependencies = [];
// @private {Object.<phetioID:string, boolean} - only populated with true values. A map of the Properties that are
- // in this.propertyOrderDependencies.
+ // in this.propertyOrderDependencies. TODO: we can probably get rid of this, or at least make it a Set, https://github.com/phetsims/axon/issues/316
this.propertiesInOrderDependencies = {};
+ // TODO: delete me? Don't we only ever need to know what goes before X, and don't need to know what goes after X?
+ // Map.<phetioID, phetioID[]>
+ this.undeferBeforeUndeferMap = new Map();
+ this.undeferBeforeUndeferMap.tempName = 'undeferBeforeUndeferMap'; // for testing
+ this.undeferBeforeNotifyMap = new Map();
+ this.undeferBeforeNotifyMap.tempName = 'undeferBeforeNotifyMap'; // for testing
+ this.notifyBeforeUndeferMap = new Map();
+ this.notifyBeforeUndeferMap.tempName = 'notifyBeforeUndeferMap'; // for testing
+ this.notifyBeforeNotifyMap = new Map();
+ this.notifyBeforeNotifyMap.tempName = 'notifyBeforeNotifyMap'; // for testing
+
+ // Map.<phetioID, Set.<phetioID>>
+ this.undeferAfterUndeferMap = new Map();
+ this.undeferAfterUndeferMap.tempName = 'undeferAfterUndeferMap'; // for testing
+ this.undeferAfterNotifyMap = new Map();
+ this.undeferAfterNotifyMap.tempName = 'undeferAfterNotifyMap'; // for testing
+ this.notifyAfterUndeferMap = new Map();
+ this.notifyAfterUndeferMap.tempName = 'notifyAfterUndeferMap'; // for testing
+ this.notifyAfterNotifyMap = new Map();
+ this.notifyAfterNotifyMap.tempName = 'notifyAfterNotifyMap'; // for testing
+
+ // TODO: rename
+ this.afterMaps = [
+ this.undeferAfterUndeferMap,
+ this.undeferAfterNotifyMap,
+ this.notifyAfterUndeferMap,
+ this.notifyAfterNotifyMap
+ ];
+
+ // TODO: rename
+ this.beforeMaps = [
+ this.undeferBeforeUndeferMap,
+ this.undeferBeforeNotifyMap,
+ this.notifyBeforeUndeferMap,
+ this.notifyBeforeNotifyMap
+ ];
+
+
+ // // {Object.<string,string[]>} - keys are beforeTuples, values are a list of afterTuples, used to look up the appropriate entries in the "after maps" above
+ // this.beforeTuples = {};
+
+ // TODO: DOC DAT
+// Map.<PropertyStatePhase, Map.<PropertyStatePhase, Map(above list)>> afterPhase->beforePhase-> map.<afterPhetioID> ->listOfAllBeforePhetioIDs
+ this.afterPhaseToMapMap = new Map();
+ const afterIsUndeferMap = new Map(); // where the "before" phase is "undefer"
+ afterIsUndeferMap.set( PropertyStatePhase.UNDEFER, this.undeferAfterUndeferMap );
+ afterIsUndeferMap.set( PropertyStatePhase.NOTIFY, this.undeferAfterNotifyMap );
+ this.afterPhaseToMapMap.set( PropertyStatePhase.UNDEFER, afterIsUndeferMap );
+ const afterIsNotifyMap = new Map(); // where the "before" phase is "notify"
+ afterIsNotifyMap.set( PropertyStatePhase.UNDEFER, this.notifyAfterUndeferMap );
+ afterIsNotifyMap.set( PropertyStatePhase.NOTIFY, this.notifyAfterNotifyMap );
+ this.afterPhaseToMapMap.set( PropertyStatePhase.NOTIFY, afterIsNotifyMap );
+
+// Map.<PropertyStatePhase, Map.<PropertyStatePhase, Map(above list)>> - afterPhase->beforePhase-> map.<beforePhetioID> ->listOfAfterPhetioIDs
+ // TODO: this is backwards (map-like afterPhase-> beforePhase), but it parallels the more important map above. . .
+ this.forDisposalOnlyBeforePhaseToMapMap = new Map();
+ const afterIsUndeferMap2 = new Map(); // where the "before" phase is "undefer"
+ afterIsUndeferMap2.set( PropertyStatePhase.UNDEFER, this.undeferBeforeUndeferMap );
+ afterIsUndeferMap2.set( PropertyStatePhase.NOTIFY, this.undeferBeforeNotifyMap );
+ this.forDisposalOnlyBeforePhaseToMapMap.set( PropertyStatePhase.UNDEFER, afterIsUndeferMap2 );
+ const beforeIsNotifyMap = new Map(); // where the "before" phase is "notify"
+ beforeIsNotifyMap.set( PropertyStatePhase.UNDEFER, this.notifyBeforeUndeferMap );
+ beforeIsNotifyMap.set( PropertyStatePhase.NOTIFY, this.notifyBeforeNotifyMap );
+ this.forDisposalOnlyBeforePhaseToMapMap.set( PropertyStatePhase.NOTIFY, beforeIsNotifyMap );
+
// @public (PropertyStateHandlerTests read-only)
this.initialized = false;
}
@@ -97,6 +162,65 @@
assert && assert( PropertyStatePhase.includes( phase ), `unexpected phase: ${phase}` );
}
+ /**
+ * @private
+ * @param beforeOrAfter
+ * @param beforePhase
+ * @param afterPhase
+ * @returns {*}
+ */
+ getMapFromPhases( beforeOrAfter, beforePhase, afterPhase ) {
+ if ( beforeOrAfter === 'after' ) {
+ return this.afterPhaseToMapMap.get( afterPhase ).get( beforePhase );
+ }
+ else if ( beforeOrAfter === 'before' ) {
+ return this.forDisposalOnlyBeforePhaseToMapMap.get( beforePhase ).get( afterPhase );
+ }
+ assert && assert( false );
+ }
+
+ /**
+ * TODO
+ * @param beforeMap
+ * @returns {*}
+ * @private
+ */
+ beforeMapToAfterMap( beforeMap ) {
+ // TODO: rename
+ if ( beforeMap === this.undeferBeforeUndeferMap ) {
+ return this.undeferAfterUndeferMap;
+ }
+ if ( beforeMap === this.undeferBeforeNotifyMap ) {
+ return this.notifyAfterUndeferMap;
+ }
+ if ( beforeMap === this.notifyBeforeUndeferMap ) {
+ return this.undeferAfterNotifyMap;
+ }
+ if ( beforeMap === this.notifyBeforeNotifyMap ) {
+ return this.notifyAfterNotifyMap;
+ }
+ } /**
+ * TODO
+ * @param afterMap
+ * @returns {*}
+ * @private
+ */
+ afterMapToAfterMap( afterMap ) {
+ // TODO: rename
+ if ( afterMap === this.undeferAfterUndeferMap ) {
+ return this.undeferBeforeUndeferMap;
+ }
+ if ( afterMap === this.undeferAfterNotifyMap ) {
+ return this.notifyBeforeUndeferMap;
+ }
+ if ( afterMap === this.notifyAfterUndeferMap ) {
+ return this.undeferBeforeNotifyMap;
+ }
+ if ( afterMap === this.notifyAfterNotifyMap ) {
+ return this.notifyBeforeNotifyMap;
+ }
+ }
+
/**
* Register that one Property must have a "Phase" applied for PhET-iO state before another Property's Phase. A Phase
* is an ending state in PhET-iO state set where Property values solidify, notifications for value changes are called.
@@ -117,8 +241,19 @@
this.propertiesInOrderDependencies[ beforeProperty.tandem.phetioID ] = true;
this.propertiesInOrderDependencies[ afterProperty.tandem.phetioID ] = true;
+ const beforeMapToPopulate = this.getMapFromPhases( 'before', beforePhase, afterPhase );
+
+ if ( !beforeMapToPopulate.has( beforeProperty.tandem.phetioID ) ) {
+ beforeMapToPopulate.set( beforeProperty.tandem.phetioID, [] ); // TODO: wait should this be a Set? 40 minutes later, I really think so!
+ }
+ beforeMapToPopulate.get( beforeProperty.tandem.phetioID ).push( afterProperty.tandem.phetioID );
- this.propertyOrderDependencies.push( new OrderDependency( beforeProperty.tandem.phetioID, beforePhase, afterProperty.tandem.phetioID, afterPhase ) );
+ const afterMapToPopulate = this.getMapFromPhases( 'after', beforePhase, afterPhase );
+
+ if ( !afterMapToPopulate.has( afterProperty.tandem.phetioID ) ) {
+ afterMapToPopulate.set( afterProperty.tandem.phetioID, new Set() );// TODO: oh wait, maybe this one should this be a Set.
+ }
+ afterMapToPopulate.get( afterProperty.tandem.phetioID ).add( beforeProperty.tandem.phetioID );
}
/**
@@ -140,15 +275,45 @@
this.validateInstrumentedProperty( property );
assert && assert( this.propertyInAnOrderDependency( property ), 'Property must be registered in an order dependency to be unregistered' );
- for ( let i = 0; i < this.propertyOrderDependencies.length; i++ ) {
- const propertyOrderDependency = this.propertyOrderDependencies[ i ];
+ const phetioIDToRemove = property.tandem.phetioID;
+
+ this.beforeMaps.forEach( beforeMap => {
+ if ( beforeMap.has( phetioIDToRemove ) ) {
+ beforeMap.get( phetioIDToRemove ).forEach( phetioID => {
+ const setOfAfterMapIDs = this.beforeMapToAfterMap( beforeMap ).get( phetioID );
+ setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove );
+ } );
+ }
+ } );
+ this.beforeMaps.forEach( map => map.delete( phetioIDToRemove ) );
- if ( propertyOrderDependency.usesPhetioID( property.tandem.phetioID ) ) {
- arrayRemove( this.propertyOrderDependencies, propertyOrderDependency );
- i--;
+ this.afterMaps.forEach( afterMap => {
+ if ( afterMap.has( phetioIDToRemove ) ) {
+ afterMap.get( phetioIDToRemove ).forEach( phetioID => {
+ const listOfAfterMapIDs = this.afterMapToAfterMap( afterMap ).get( phetioID );
+ listOfAfterMapIDs && arrayRemove( listOfAfterMapIDs, phetioIDToRemove );
+ } );
}
+ } );
+ this.afterMaps.forEach( map => map.delete( phetioIDToRemove ) );
+
+ if ( assert ) {
+ this.beforeMaps.forEach( map => {
+ for ( const [ key, valuePhetioIDs ] of map ) {
+ assert && assert( key !== phetioIDToRemove, 'should not be a before key' );
+ assert && assert( !valuePhetioIDs.includes( phetioIDToRemove ), 'should not be in a before value list' );
+ }
+ } );
+
+ this.afterMaps.forEach( map => {
+ for ( const [ key, valuePhetioIDSet ] of map ) {
+ assert && assert( key !== phetioIDToRemove, 'should not be a before key' );
+ assert && assert( !valuePhetioIDSet.has( phetioIDToRemove ), 'should not be in a before value list' );
+ }
+ } );
}
- delete this.propertiesInOrderDependencies[ property.tandem.phetioID ];
+
+ delete this.propertiesInOrderDependencies[ phetioIDToRemove ];
}
/**
@@ -161,22 +326,6 @@
undeferAndNotifyProperties( phetioIDsInState ) {
assert && assert( this.initialized, 'must be initialized before getting called' );
- // Ignore order dependencies that do not apply to the list of phetioIDs that are getting set this time. This is because
- // they do not apply to the list of phetioIDs passed in via state.
- const orderDependenciesToIgnore = [];
- for ( let j = 0; j < this.propertyOrderDependencies.length; j++ ) {
- const orderDependency = this.propertyOrderDependencies[ j ];
-
- // If either side of the order dependency is not in the state, then ignore the order dependency entirely
- if ( phetioIDsInState.indexOf( orderDependency.beforePhetioID ) === -1 ||
- phetioIDsInState.indexOf( orderDependency.afterPhetioID ) === -1 ) {
- orderDependenciesToIgnore.push( orderDependency );
- }
- }
-
- // {OrderDependency[]} - This list accounts for order dependencies the do not apply to the phetioIDs set in this setState() call
- const orderDependenciesToLookAt = _.without( this.propertyOrderDependencies, ...orderDependenciesToIgnore );
-
// {Object.<string,boolean>} - true if a phetioID + phase pair has been applied, keys are the combination of
// phetioIDs and phase, see PhaseCallback.getTerm()
const completedPhases = {};
@@ -190,24 +339,25 @@
// Error case logging
if ( numberOfIterations > 5000 ) {
- this.errorInUndeferAndNotifyStep( completedPhases, orderDependenciesToLookAt );
+ // this.errorInUndeferAndNotifyStep( completedPhases );
}
// Try to undefer as much as possible before notifying
- this.attemptToApplyPhases( PropertyStatePhase.UNDEFER, completedPhases, orderDependenciesToLookAt );
- this.attemptToApplyPhases( PropertyStatePhase.NOTIFY, completedPhases, orderDependenciesToLookAt );
+ this.attemptToApplyPhases( PropertyStatePhase.UNDEFER, completedPhases );
+ this.attemptToApplyPhases( PropertyStatePhase.NOTIFY, completedPhases );
}
}
/**
* @param {Object.<string,boolean>} completedPhases
- * @param {OrderDependency[]} orderDependencies
* @private
*/
- errorInUndeferAndNotifyStep( completedPhases, orderDependencies ) {
+ errorInUndeferAndNotifyStep( completedPhases ) {
// combine phetioID and Phase into a single string to keep this process specific.
const stillToDoIDPhasePairs = this.phaseCallbacks.map( item => item.phetioID + item.phase );
+
+ // TODO: support maps instead of orderDependencies list
const relevantOrderDependencies = orderDependencies.filter( orderDependency => {
return stillToDoIDPhasePairs.indexOf( orderDependency.getBeforeTerm() ) >= 0 ||
stillToDoIDPhasePairs.indexOf( orderDependency.getAfterTerm() ) >= 0;
@@ -241,6 +391,22 @@
}
}
+ /**
+ * Only for Testing!
+ * Get the number of order dependencies registered in this class
+ * @public
+ * @returns {number}
+ */
+ getNumberOfOrderDependencies() {
+ let count = 0;
+ this.afterMaps.forEach( map => {
+ for ( const [ , value ] of map ) {
+ count += value.size;
+ }
+ } );
+ return count;
+ }
+
/**
* Go through all phases still to be applied, and apply them if the order dependencies allow it. Only apply for the
* particular phase provided. In general UNDEFER must occur before the same phetioID gets NOTIFY.
@@ -248,9 +414,8 @@
*
* @param {PropertyStatePhase} phase - only apply PhaseCallbacks for this particular PropertyStatePhase
* @param {Object.<string,boolean>} completedPhases - map that keeps track of completed phases
- * @param {OrderDependency[]} orderDependencies
*/
- attemptToApplyPhases( phase, completedPhases, orderDependencies ) {
+ attemptToApplyPhases( phase, completedPhases ) {
for ( let i = 0; i < this.phaseCallbacks.length; i++ ) {
const phaseCallbackToPotentiallyApply = this.phaseCallbacks[ i ];
@@ -258,7 +423,7 @@
// this.phaseCallbacks includes all phases, only try to
// check the order dependencies to see if this has to be after something that is incomplete.
if ( phaseCallbackToPotentiallyApply.phase === phase &&
- this.phetioIDCanApplyPhase( phaseCallbackToPotentiallyApply.phetioID, phase, completedPhases, orderDependencies ) ) {
+ this.phetioIDCanApplyPhase( phaseCallbackToPotentiallyApply.phetioID, phase, completedPhases ) ) {
// Fire the listener;
phaseCallbackToPotentiallyApply.listener();
@@ -274,29 +439,61 @@
}
}
+ /**
+ * TODO: do we need this? Could it be a Map? COuld it be better?
+ * @private
+ * @param map
+ * @returns {*}
+ */
+ getBeforePhaseFromMap( map ) {
+ if ( map === this.undeferAfterUndeferMap || map === this.notifyAfterUndeferMap ) {
+ return PropertyStatePhase.UNDEFER;
+ }
+ if ( map === this.undeferAfterNotifyMap || map === this.notifyAfterNotifyMap ) {
+ return PropertyStatePhase.NOTIFY;
+ }
+ }
+
+
/**
* @private
* @param {string} phetioID
* @param {PropertyStatePhase} phase
* @param {Object.<string,boolean>} completedPhases - map that keeps track of completed phases
- * @param {OrderDependency[]} orderDependencies
* @returns {boolean} - if the provided phase can be applied given the dependency order dependencies of the state engine.
*/
- phetioIDCanApplyPhase( phetioID, phase, completedPhases, orderDependencies ) {
+ phetioIDCanApplyPhase( phetioID, phase, completedPhases ) {
// Undefer must happen before notify
if ( phase === PropertyStatePhase.NOTIFY && !completedPhases[ phetioID + PropertyStatePhase.UNDEFER ] ) {
return false;
}
- // check the order dependencies to see if this has to be after something that is incomplete.
- // all must pass
- for ( let i = 0; i < orderDependencies.length; i++ ) {
- const orderDependency = orderDependencies[ i ];
- if ( orderDependency.afterPhetioID === phetioID && orderDependency.afterPhase === phase ) {
+ /*
+ IF: phase === notify
+ THEN maps to check should be:
+ notifyAfterNotifyMap
+ notifyAfterUndeferMap
+*/
+
+ let mapsToCheck = [ this.undeferAfterUndeferMap, this.undeferAfterNotifyMap ];
+ if ( phase === PropertyStatePhase.NOTIFY ) {
+ mapsToCheck = [ this.notifyAfterUndeferMap, this.notifyAfterNotifyMap ];
+ }
+
+ // O(2)
+ for ( let i = 0; i < mapsToCheck.length; i++ ) {
+ const mapToCheck = mapsToCheck[ i ];
+ if ( !mapToCheck.has( phetioID ) ) {
+ return true;
+ }
+ const setOfThingsThatShouldComeFirst = mapToCheck.get( phetioID );
+
+ // O(K) where K is the number of elements that should come before Property X
+ for ( const beforePhetioID of setOfThingsThatShouldComeFirst ) {
// check if the before phase for this order dependency has already been completed
- if ( !completedPhases[ orderDependency.getBeforeTerm() ] ) {
+ if ( !completedPhases[ beforePhetioID + this.getBeforePhaseFromMap( mapToCheck ) ] ) {
return false;
}
}
@@ -330,49 +527,5 @@
}
}
-// POJSO for an order dependency. See registerPropertyOrderDependency
-class OrderDependency {
-
- /**
- * @param {string} beforePhetioID
- * @param {PropertyStatePhase} beforePhase
- * @param {string} afterPhetioID
- * @param {PropertyStatePhase} afterPhase
- */
- constructor( beforePhetioID, beforePhase, afterPhetioID, afterPhase ) {
-
- // @public
- this.beforePhetioID = beforePhetioID;
- this.beforePhase = beforePhase;
- this.afterPhetioID = afterPhetioID;
- this.afterPhase = afterPhase;
- }
-
- /**
- * @public
- * @returns {string} - unique term for the before id/phase pair
- */
- getBeforeTerm() {
- return this.beforePhetioID + this.beforePhase;
- }
-
- /**
- * @public
- * @returns {string} - unique term for the before id/phase pair
- */
- getAfterTerm() {
- return this.afterPhetioID + this.afterPhase;
- }
-
- /**
- * @public
- * @param {string} phetioID
- * @returns {boolean} - if this order dependency uses the provided phetioID
- */
- usesPhetioID( phetioID ) {
- return this.beforePhetioID === phetioID || this.afterPhetioID === phetioID;
- }
-}
-
axon.register( 'PropertyStateHandler', PropertyStateHandler );
export default PropertyStateHandler;
\ No newline at end of file
Index: js/PropertyStateHandlerTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandlerTests.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/PropertyStateHandlerTests.js (date 1597282143069)
@@ -18,6 +18,7 @@
import PropertyStateHandler from './PropertyStateHandler.js';
import propertyStateHandlerSingleton from './propertyStateHandlerSingleton.js';
import PropertyStatePhase from './PropertyStatePhase.js';
+QUnit.log( context => { if ( !context.result ) { debugger; }} );
QUnit.module( 'PropertyStateHandler' );
@@ -47,34 +48,31 @@
tandem: Tandem.GENERAL.createTandem( 'cProperty' )
} );
+ const originalOrderDependencyLength = propertyStateHandler.getNumberOfOrderDependencies();
+ const getOrderDependencyLength = () => propertyStateHandler.getNumberOfOrderDependencies() - originalOrderDependencyLength;
+
+
propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyB, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 1, 'one expected' );
- const aToBDependency = propertyStateHandler.propertyOrderDependencies[ 0 ];
+ assert.ok( getOrderDependencyLength() === 1, 'one expected' );
propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 2, 'two expected' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 0 ] === aToBDependency, 'push expected instead of unshift1' );
- const aToCDependency = propertyStateHandler.propertyOrderDependencies[ 1 ];
+ assert.ok( getOrderDependencyLength() === 2, 'two expected' );
propertyStateHandler.registerPhetioOrderDependency( propertyB, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 3, 'three expected' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 0 ] === aToBDependency, 'push expected instead of unshift2' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 1 ] === aToCDependency, 'push expected instead of unshift3' );
- const bToCDependency = propertyStateHandler.propertyOrderDependencies[ 2 ];
+ assert.ok( getOrderDependencyLength() === 3, 'three expected' );
propertyStateHandler.unregisterOrderDependenciesForProperty( propertyA );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 1, 'a was in two' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 0 ] === bToCDependency, 'push expected instead of unshift3' );
+ assert.ok( getOrderDependencyLength() === 1, 'a was in two' );
propertyStateHandler.unregisterOrderDependenciesForProperty( propertyB );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 0, 'none now' );
+ assert.ok( getOrderDependencyLength() === 0, 'none now' );
propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
propertyStateHandler.registerPhetioOrderDependency( propertyB, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 2, 'none now' );
+ assert.ok( getOrderDependencyLength() === 2, 'none now' );
propertyStateHandler.unregisterOrderDependenciesForProperty( propertyC );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 0, 'none now' );
+ assert.ok( getOrderDependencyLength() === 0, 'none now' );
propertyA.dispose();
propertyB.dispose();
Index: js/DerivedPropertyTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/DerivedPropertyTests.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/DerivedPropertyTests.js (date 1597280378200)
@@ -135,8 +135,8 @@
const parentTandem = Tandem.GENERAL;
- const originalOrderDependencyLength = propertyStateHandlerSingleton.propertyOrderDependencies.length;
- const getOrderDependencyLength = () => propertyStateHandlerSingleton.propertyOrderDependencies.length - originalOrderDependencyLength;
+ const originalOrderDependencyLength = propertyStateHandlerSingleton.getNumberOfOrderDependencies();
+ const getOrderDependencyLength = () => propertyStateHandlerSingleton.getNumberOfOrderDependencies() - originalOrderDependencyLength;
const firstProperty = new Property( 1, {
tandem: parentTandem.createTandem( 'firstProperty' ),
Currently, this is potentially the most prototypey code I have ever written. It is far from a commit (although I think not far from feature complete). I look forward to refinement, and then, later, review with @samreid! To test:
|
|
Tests are working with some cleanup done; not yet working in a sim though because we aren't excluding order dependencies on elements that aren't in the state to be set: Index: js/PropertyStateHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandler.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/PropertyStateHandler.js (date 1597297614081)
@@ -33,9 +33,66 @@
this.propertyOrderDependencies = [];
// @private {Object.<phetioID:string, boolean} - only populated with true values. A map of the Properties that are
- // in this.propertyOrderDependencies.
+ // in this.propertyOrderDependencies. TODO: we can probably get rid of this, or at least make it a Set, https://github.com/phetsims/axon/issues/316
this.propertiesInOrderDependencies = {};
+ // TODO: delete me? Don't we only ever need to know what goes before X, and don't need to know what goes after X?
+ // Map.<phetioID, phetioID[]>
+ this.undeferBeforeUndeferMap = new OrderDependencyMap( PropertyStatePhase.UNDEFER, PropertyStatePhase.UNDEFER, 'undeferBeforeUndeferMap' );
+ this.undeferBeforeNotifyMap = new OrderDependencyMap( PropertyStatePhase.UNDEFER, PropertyStatePhase.NOTIFY, 'undeferBeforeNotifyMap' );
+ this.notifyBeforeUndeferMap = new OrderDependencyMap( PropertyStatePhase.NOTIFY, PropertyStatePhase.UNDEFER, 'notifyBeforeUndeferMap' );
+ this.notifyBeforeNotifyMap = new OrderDependencyMap( PropertyStatePhase.NOTIFY, PropertyStatePhase.NOTIFY, 'notifyBeforeNotifyMap' );
+
+ // Map.<phetioID, Set.<phetioID>>
+ this.undeferAfterUndeferMap = new OrderDependencyMap( PropertyStatePhase.UNDEFER, PropertyStatePhase.UNDEFER, 'undeferAfterUndeferMap' );
+ this.undeferAfterNotifyMap = new OrderDependencyMap( PropertyStatePhase.NOTIFY, PropertyStatePhase.UNDEFER, 'undeferAfterNotifyMap' );
+ this.notifyAfterUndeferMap = new OrderDependencyMap( PropertyStatePhase.UNDEFER, PropertyStatePhase.NOTIFY, 'notifyAfterUndeferMap' );
+ this.notifyAfterNotifyMap = new OrderDependencyMap( PropertyStatePhase.NOTIFY, PropertyStatePhase.NOTIFY, 'notifyAfterNotifyMap' );
+
+ // TODO: rename
+ this.afterMaps = [
+ this.undeferAfterUndeferMap,
+ this.undeferAfterNotifyMap,
+ this.notifyAfterUndeferMap,
+ this.notifyAfterNotifyMap
+ ];
+
+ // TODO: rename
+ this.beforeMaps = [
+ this.undeferBeforeUndeferMap,
+ this.undeferBeforeNotifyMap,
+ this.notifyBeforeUndeferMap,
+ this.notifyBeforeNotifyMap
+ ];
+
+
+ // // {Object.<string,string[]>} - keys are beforeTuples, values are a list of afterTuples, used to look up the appropriate entries in the "after maps" above
+ // this.beforeTuples = {};
+
+ // TODO: DOC DAT
+// Map.<PropertyStatePhase, Map.<PropertyStatePhase, Map(above list)>> afterPhase->beforePhase-> map.<afterPhetioID> ->listOfAllBeforePhetioIDs
+ this.afterPhaseToMapMap = new Map();
+ const afterIsUndeferMap = new Map(); // where the "before" phase is "undefer"
+ afterIsUndeferMap.set( PropertyStatePhase.UNDEFER, this.undeferAfterUndeferMap );
+ afterIsUndeferMap.set( PropertyStatePhase.NOTIFY, this.undeferAfterNotifyMap );
+ this.afterPhaseToMapMap.set( PropertyStatePhase.UNDEFER, afterIsUndeferMap );
+ const afterIsNotifyMap = new Map(); // where the "before" phase is "notify"
+ afterIsNotifyMap.set( PropertyStatePhase.UNDEFER, this.notifyAfterUndeferMap );
+ afterIsNotifyMap.set( PropertyStatePhase.NOTIFY, this.notifyAfterNotifyMap );
+ this.afterPhaseToMapMap.set( PropertyStatePhase.NOTIFY, afterIsNotifyMap );
+
+// Map.<PropertyStatePhase, Map.<PropertyStatePhase, Map(above list)>> - afterPhase->beforePhase-> map.<beforePhetioID> ->listOfAfterPhetioIDs
+ // TODO: this is backwards (map-like afterPhase-> beforePhase), but it parallels the more important map above. . .
+ this.forDisposalOnlyBeforePhaseToMapMap = new Map();
+ const afterIsUndeferMap2 = new Map(); // where the "before" phase is "undefer"
+ afterIsUndeferMap2.set( PropertyStatePhase.UNDEFER, this.undeferBeforeUndeferMap );
+ afterIsUndeferMap2.set( PropertyStatePhase.NOTIFY, this.undeferBeforeNotifyMap );
+ this.forDisposalOnlyBeforePhaseToMapMap.set( PropertyStatePhase.UNDEFER, afterIsUndeferMap2 );
+ const beforeIsNotifyMap = new Map(); // where the "before" phase is "notify"
+ beforeIsNotifyMap.set( PropertyStatePhase.UNDEFER, this.notifyBeforeUndeferMap );
+ beforeIsNotifyMap.set( PropertyStatePhase.NOTIFY, this.notifyBeforeNotifyMap );
+ this.forDisposalOnlyBeforePhaseToMapMap.set( PropertyStatePhase.NOTIFY, beforeIsNotifyMap );
+
// @public (PropertyStateHandlerTests read-only)
this.initialized = false;
}
@@ -97,6 +154,67 @@
assert && assert( PropertyStatePhase.includes( phase ), `unexpected phase: ${phase}` );
}
+ /**
+ * @private
+ * @param beforeOrAfter
+ * @param beforePhase
+ * @param afterPhase
+ * @returns {*}
+ */
+ getMapFromPhases( beforeOrAfter, beforePhase, afterPhase ) {
+ if ( beforeOrAfter === 'after' ) {
+ return this.afterPhaseToMapMap.get( afterPhase ).get( beforePhase );
+ }
+ else if ( beforeOrAfter === 'before' ) {
+ return this.forDisposalOnlyBeforePhaseToMapMap.get( beforePhase ).get( afterPhase );
+ }
+ assert && assert( false );
+ }
+
+ /**
+ * TODO
+ * @param beforeMap
+ * @returns {*}
+ * @private
+ */
+ beforeMapToAfterMap( beforeMap ) {
+ // TODO: rename
+ if ( beforeMap === this.undeferBeforeUndeferMap ) {
+ return this.undeferAfterUndeferMap;
+ }
+ if ( beforeMap === this.undeferBeforeNotifyMap ) {
+ return this.notifyAfterUndeferMap;
+ }
+ if ( beforeMap === this.notifyBeforeUndeferMap ) {
+ return this.undeferAfterNotifyMap;
+ }
+ if ( beforeMap === this.notifyBeforeNotifyMap ) {
+ return this.notifyAfterNotifyMap;
+ }
+ }
+
+ /**
+ * TODO
+ * @param afterMap
+ * @returns {*}
+ * @private
+ */
+ afterMapToAfterMap( afterMap ) {
+ // TODO: rename
+ if ( afterMap === this.undeferAfterUndeferMap ) {
+ return this.undeferBeforeUndeferMap;
+ }
+ if ( afterMap === this.undeferAfterNotifyMap ) {
+ return this.notifyBeforeUndeferMap;
+ }
+ if ( afterMap === this.notifyAfterUndeferMap ) {
+ return this.undeferBeforeNotifyMap;
+ }
+ if ( afterMap === this.notifyAfterNotifyMap ) {
+ return this.notifyBeforeNotifyMap;
+ }
+ }
+
/**
* Register that one Property must have a "Phase" applied for PhET-iO state before another Property's Phase. A Phase
* is an ending state in PhET-iO state set where Property values solidify, notifications for value changes are called.
@@ -117,8 +235,19 @@
this.propertiesInOrderDependencies[ beforeProperty.tandem.phetioID ] = true;
this.propertiesInOrderDependencies[ afterProperty.tandem.phetioID ] = true;
+ const beforeMapToPopulate = this.getMapFromPhases( 'before', beforePhase, afterPhase );
+
+ if ( !beforeMapToPopulate.has( beforeProperty.tandem.phetioID ) ) {
+ beforeMapToPopulate.set( beforeProperty.tandem.phetioID, [] ); // TODO: wait should this be a Set? 40 minutes later, I really think so!
+ }
+ beforeMapToPopulate.get( beforeProperty.tandem.phetioID ).push( afterProperty.tandem.phetioID );
- this.propertyOrderDependencies.push( new OrderDependency( beforeProperty.tandem.phetioID, beforePhase, afterProperty.tandem.phetioID, afterPhase ) );
+ const afterMapToPopulate = this.getMapFromPhases( 'after', beforePhase, afterPhase );
+
+ if ( !afterMapToPopulate.has( afterProperty.tandem.phetioID ) ) {
+ afterMapToPopulate.set( afterProperty.tandem.phetioID, new Set() );// TODO: oh wait, maybe this one should this be a Set.
+ }
+ afterMapToPopulate.get( afterProperty.tandem.phetioID ).add( beforeProperty.tandem.phetioID );
}
/**
@@ -140,15 +269,45 @@
this.validateInstrumentedProperty( property );
assert && assert( this.propertyInAnOrderDependency( property ), 'Property must be registered in an order dependency to be unregistered' );
- for ( let i = 0; i < this.propertyOrderDependencies.length; i++ ) {
- const propertyOrderDependency = this.propertyOrderDependencies[ i ];
+ const phetioIDToRemove = property.tandem.phetioID;
+
+ this.beforeMaps.forEach( beforeMap => {
+ if ( beforeMap.has( phetioIDToRemove ) ) {
+ beforeMap.get( phetioIDToRemove ).forEach( phetioID => {
+ const setOfAfterMapIDs = this.beforeMapToAfterMap( beforeMap ).get( phetioID );
+ setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove );
+ } );
+ }
+ } );
+ this.beforeMaps.forEach( map => map.delete( phetioIDToRemove ) );
- if ( propertyOrderDependency.usesPhetioID( property.tandem.phetioID ) ) {
- arrayRemove( this.propertyOrderDependencies, propertyOrderDependency );
- i--;
+ this.afterMaps.forEach( afterMap => {
+ if ( afterMap.has( phetioIDToRemove ) ) {
+ afterMap.get( phetioIDToRemove ).forEach( phetioID => {
+ const listOfAfterMapIDs = this.afterMapToAfterMap( afterMap ).get( phetioID );
+ listOfAfterMapIDs && arrayRemove( listOfAfterMapIDs, phetioIDToRemove );
+ } );
}
+ } );
+ this.afterMaps.forEach( map => map.delete( phetioIDToRemove ) );
+
+ if ( assert ) {
+ this.beforeMaps.forEach( map => {
+ for ( const [ key, valuePhetioIDs ] of map ) {
+ assert && assert( key !== phetioIDToRemove, 'should not be a before key' );
+ assert && assert( !valuePhetioIDs.includes( phetioIDToRemove ), 'should not be in a before value list' );
+ }
+ } );
+
+ this.afterMaps.forEach( map => {
+ for ( const [ key, valuePhetioIDSet ] of map ) {
+ assert && assert( key !== phetioIDToRemove, 'should not be a before key' );
+ assert && assert( !valuePhetioIDSet.has( phetioIDToRemove ), 'should not be in a before value list' );
+ }
+ } );
}
- delete this.propertiesInOrderDependencies[ property.tandem.phetioID ];
+
+ delete this.propertiesInOrderDependencies[ phetioIDToRemove ];
}
/**
@@ -161,22 +320,6 @@
undeferAndNotifyProperties( phetioIDsInState ) {
assert && assert( this.initialized, 'must be initialized before getting called' );
- // Ignore order dependencies that do not apply to the list of phetioIDs that are getting set this time. This is because
- // they do not apply to the list of phetioIDs passed in via state.
- const orderDependenciesToIgnore = [];
- for ( let j = 0; j < this.propertyOrderDependencies.length; j++ ) {
- const orderDependency = this.propertyOrderDependencies[ j ];
-
- // If either side of the order dependency is not in the state, then ignore the order dependency entirely
- if ( phetioIDsInState.indexOf( orderDependency.beforePhetioID ) === -1 ||
- phetioIDsInState.indexOf( orderDependency.afterPhetioID ) === -1 ) {
- orderDependenciesToIgnore.push( orderDependency );
- }
- }
-
- // {OrderDependency[]} - This list accounts for order dependencies the do not apply to the phetioIDs set in this setState() call
- const orderDependenciesToLookAt = _.without( this.propertyOrderDependencies, ...orderDependenciesToIgnore );
-
// {Object.<string,boolean>} - true if a phetioID + phase pair has been applied, keys are the combination of
// phetioIDs and phase, see PhaseCallback.getTerm()
const completedPhases = {};
@@ -188,34 +331,54 @@
while ( this.phaseCallbacks.length > 0 ) {
numberOfIterations++;
+
+ if ( numberOfIterations > 3000 ) {
+ window.hi = true;
+ }
+
// Error case logging
if ( numberOfIterations > 5000 ) {
- this.errorInUndeferAndNotifyStep( completedPhases, orderDependenciesToLookAt );
+ this.errorInUndeferAndNotifyStep( completedPhases );
}
// Try to undefer as much as possible before notifying
- this.attemptToApplyPhases( PropertyStatePhase.UNDEFER, completedPhases, orderDependenciesToLookAt );
- this.attemptToApplyPhases( PropertyStatePhase.NOTIFY, completedPhases, orderDependenciesToLookAt );
+ this.attemptToApplyPhases( PropertyStatePhase.UNDEFER, completedPhases );
+ this.attemptToApplyPhases( PropertyStatePhase.NOTIFY, completedPhases );
}
}
/**
* @param {Object.<string,boolean>} completedPhases
- * @param {OrderDependency[]} orderDependencies
* @private
*/
- errorInUndeferAndNotifyStep( completedPhases, orderDependencies ) {
+ errorInUndeferAndNotifyStep( completedPhases ) {
// combine phetioID and Phase into a single string to keep this process specific.
const stillToDoIDPhasePairs = this.phaseCallbacks.map( item => item.phetioID + item.phase );
- const relevantOrderDependencies = orderDependencies.filter( orderDependency => {
- return stillToDoIDPhasePairs.indexOf( orderDependency.getBeforeTerm() ) >= 0 ||
- stillToDoIDPhasePairs.indexOf( orderDependency.getAfterTerm() ) >= 0;
+
+ const relevantOrderDependencies = [];
+
+ this.beforeMaps.forEach( map => {
+ for ( const [ beforePhetioID, afterPhetioIDs ] of map ) {
+ afterPhetioIDs.forEach( afterPhetioID => {
+ const beforeTerm = beforePhetioID + map.beforePhase;
+ const afterTerm = afterPhetioID + map.afterPhase;
+ if ( stillToDoIDPhasePairs.includes( beforeTerm ) || stillToDoIDPhasePairs.includes( afterTerm ) ) {
+ relevantOrderDependencies.push( {
+ beforeTerm: beforeTerm,
+ afterTerm: afterTerm
+ } );
+ }
+ } );
+ }
} );
+
const completedPhasePairs = _.keys( completedPhases ).filter( completedID => {
for ( let i = 0; i < relevantOrderDependencies.length; i++ ) {
const relevantOrderDependency = relevantOrderDependencies[ i ];
- if ( relevantOrderDependency.usesPhetioID( completedID ) ) {
+
+ // TODO: startsWith does not mean that it is the actual phetioID, it could be a parent. Perhaps we need to be able to split the term back in half, or be more OO.
+ if ( relevantOrderDependency.beforeTerm.startsWith( completedID ) || relevantOrderDependency.afterTerm.startsWith( completedID ) ) {
return true;
}
}
@@ -228,7 +391,7 @@
console.log( 'completed phase pairs that share phetioIDs', completedPhasePairs );
console.log( 'order dependencies that apply to the still todos', relevantOrderDependencies );
relevantOrderDependencies.forEach( orderDependency => {
- string += `${orderDependency.getBeforeTerm()}\t${orderDependency.getAfterTerm()}\n`;
+ string += `${orderDependency.beforeTerm}\t${orderDependency.afterTerm}\n`;
} );
console.log( '\n\nin graphable form:\n\n', string );
@@ -241,6 +404,22 @@
}
}
+ /**
+ * Only for Testing!
+ * Get the number of order dependencies registered in this class
+ * @public
+ * @returns {number}
+ */
+ getNumberOfOrderDependencies() {
+ let count = 0;
+ this.afterMaps.forEach( map => {
+ for ( const [ , value ] of map ) {
+ count += value.size;
+ }
+ } );
+ return count;
+ }
+
/**
* Go through all phases still to be applied, and apply them if the order dependencies allow it. Only apply for the
* particular phase provided. In general UNDEFER must occur before the same phetioID gets NOTIFY.
@@ -248,9 +427,8 @@
*
* @param {PropertyStatePhase} phase - only apply PhaseCallbacks for this particular PropertyStatePhase
* @param {Object.<string,boolean>} completedPhases - map that keeps track of completed phases
- * @param {OrderDependency[]} orderDependencies
*/
- attemptToApplyPhases( phase, completedPhases, orderDependencies ) {
+ attemptToApplyPhases( phase, completedPhases ) {
for ( let i = 0; i < this.phaseCallbacks.length; i++ ) {
const phaseCallbackToPotentiallyApply = this.phaseCallbacks[ i ];
@@ -258,7 +436,7 @@
// this.phaseCallbacks includes all phases, only try to
// check the order dependencies to see if this has to be after something that is incomplete.
if ( phaseCallbackToPotentiallyApply.phase === phase &&
- this.phetioIDCanApplyPhase( phaseCallbackToPotentiallyApply.phetioID, phase, completedPhases, orderDependencies ) ) {
+ this.phetioIDCanApplyPhase( phaseCallbackToPotentiallyApply.phetioID, phase, completedPhases ) ) {
// Fire the listener;
phaseCallbackToPotentiallyApply.listener();
@@ -274,29 +452,64 @@
}
}
+ /**
+ * TODO: do we need this? Could it be a Map? COuld it be better?
+ * @private
+ * @param map
+ * @returns {*}
+ */
+ getBeforePhaseFromMap( map ) {
+ if ( map === this.undeferAfterUndeferMap || map === this.notifyAfterUndeferMap ) {
+ return PropertyStatePhase.UNDEFER;
+ }
+ if ( map === this.undeferAfterNotifyMap || map === this.notifyAfterNotifyMap ) {
+ return PropertyStatePhase.NOTIFY;
+ }
+ }
+
+
/**
* @private
* @param {string} phetioID
* @param {PropertyStatePhase} phase
* @param {Object.<string,boolean>} completedPhases - map that keeps track of completed phases
- * @param {OrderDependency[]} orderDependencies
* @returns {boolean} - if the provided phase can be applied given the dependency order dependencies of the state engine.
*/
- phetioIDCanApplyPhase( phetioID, phase, completedPhases, orderDependencies ) {
+ phetioIDCanApplyPhase( phetioID, phase, completedPhases ) {
+ if ( window.hi ) {
+ debugger;
+ }
// Undefer must happen before notify
if ( phase === PropertyStatePhase.NOTIFY && !completedPhases[ phetioID + PropertyStatePhase.UNDEFER ] ) {
return false;
}
- // check the order dependencies to see if this has to be after something that is incomplete.
- // all must pass
- for ( let i = 0; i < orderDependencies.length; i++ ) {
- const orderDependency = orderDependencies[ i ];
- if ( orderDependency.afterPhetioID === phetioID && orderDependency.afterPhase === phase ) {
+ /*
+ IF: phase === notify
+ THEN maps to check should be:
+ notifyAfterNotifyMap
+ notifyAfterUndeferMap
+*/
+
+ let mapsToCheck = [ this.undeferAfterUndeferMap, this.undeferAfterNotifyMap ];
+ if ( phase === PropertyStatePhase.NOTIFY ) {
+ mapsToCheck = [ this.notifyAfterUndeferMap, this.notifyAfterNotifyMap ];
+ }
+
+ // O(2)
+ for ( let i = 0; i < mapsToCheck.length; i++ ) {
+ const mapToCheck = mapsToCheck[ i ];
+ if ( !mapToCheck.has( phetioID ) ) {
+ return true;
+ }
+ const setOfThingsThatShouldComeFirst = mapToCheck.get( phetioID );
+
+ // O(K) where K is the number of elements that should come before Property X
+ for ( const beforePhetioID of setOfThingsThatShouldComeFirst ) {
// check if the before phase for this order dependency has already been completed
- if ( !completedPhases[ orderDependency.getBeforeTerm() ] ) {
+ if ( !completedPhases[ beforePhetioID + mapToCheck.beforePhase ] ) {
return false;
}
}
@@ -330,47 +543,13 @@
}
}
-// POJSO for an order dependency. See registerPropertyOrderDependency
-class OrderDependency {
+class OrderDependencyMap extends Map {
- /**
- * @param {string} beforePhetioID
- * @param {PropertyStatePhase} beforePhase
- * @param {string} afterPhetioID
- * @param {PropertyStatePhase} afterPhase
- */
- constructor( beforePhetioID, beforePhase, afterPhetioID, afterPhase ) {
-
- // @public
- this.beforePhetioID = beforePhetioID;
+ constructor( beforePhase, afterPhase, varName ) {
+ super();
this.beforePhase = beforePhase;
- this.afterPhetioID = afterPhetioID;
this.afterPhase = afterPhase;
- }
-
- /**
- * @public
- * @returns {string} - unique term for the before id/phase pair
- */
- getBeforeTerm() {
- return this.beforePhetioID + this.beforePhase;
- }
-
- /**
- * @public
- * @returns {string} - unique term for the before id/phase pair
- */
- getAfterTerm() {
- return this.afterPhetioID + this.afterPhase;
- }
-
- /**
- * @public
- * @param {string} phetioID
- * @returns {boolean} - if this order dependency uses the provided phetioID
- */
- usesPhetioID( phetioID ) {
- return this.beforePhetioID === phetioID || this.afterPhetioID === phetioID;
+ this.varName = varName; // useful while in development
}
}
Index: js/PropertyTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyTests.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/PropertyTests.js (date 1597296351395)
@@ -259,8 +259,8 @@
QUnit.test( 'propertyStateHandlerSingleton tests for Property', assert => {
const parentTandem = Tandem.GENERAL;
- const originalOrderDependencyLength = propertyStateHandlerSingleton.propertyOrderDependencies.length;
- const getOrderDependencyLength = () => propertyStateHandlerSingleton.propertyOrderDependencies.length - originalOrderDependencyLength;
+ const originalOrderDependencyLength = propertyStateHandlerSingleton.getNumberOfOrderDependencies();
+ const getOrderDependencyLength = () => propertyStateHandlerSingleton.getNumberOfOrderDependencies() - originalOrderDependencyLength;
const firstProperty = new Property( 1, {
tandem: parentTandem.createTandem( 'firstProperty' ),
@@ -273,9 +273,6 @@
propertyStateHandlerSingleton.registerPhetioOrderDependency( firstProperty, PropertyStatePhase.NOTIFY, secondProperty, PropertyStatePhase.UNDEFER );
- let orderDependency = propertyStateHandlerSingleton.propertyOrderDependencies[ originalOrderDependencyLength ];
- assert.ok( orderDependency.beforePhetioID === firstProperty.tandem.phetioID );
-
firstProperty.dispose();
assert.ok( getOrderDependencyLength() === 0, 'dispose removes order dependency' );
@@ -288,8 +285,6 @@
} );
assert.ok( getOrderDependencyLength() === 1, 'just added orderDependency from phetioDependencies' );
- orderDependency = propertyStateHandlerSingleton.propertyOrderDependencies[ originalOrderDependencyLength ];
- assert.ok( orderDependency.beforePhetioID === thirdProperty.tandem.phetioID );
secondProperty.dispose();
assert.ok( getOrderDependencyLength() === 0, 'dispose removes order dependency' );
Index: js/PropertyStateHandlerTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandlerTests.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/PropertyStateHandlerTests.js (date 1597282143069)
@@ -18,6 +18,7 @@
import PropertyStateHandler from './PropertyStateHandler.js';
import propertyStateHandlerSingleton from './propertyStateHandlerSingleton.js';
import PropertyStatePhase from './PropertyStatePhase.js';
+QUnit.log( context => { if ( !context.result ) { debugger; }} );
QUnit.module( 'PropertyStateHandler' );
@@ -47,34 +48,31 @@
tandem: Tandem.GENERAL.createTandem( 'cProperty' )
} );
+ const originalOrderDependencyLength = propertyStateHandler.getNumberOfOrderDependencies();
+ const getOrderDependencyLength = () => propertyStateHandler.getNumberOfOrderDependencies() - originalOrderDependencyLength;
+
+
propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyB, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 1, 'one expected' );
- const aToBDependency = propertyStateHandler.propertyOrderDependencies[ 0 ];
+ assert.ok( getOrderDependencyLength() === 1, 'one expected' );
propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 2, 'two expected' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 0 ] === aToBDependency, 'push expected instead of unshift1' );
- const aToCDependency = propertyStateHandler.propertyOrderDependencies[ 1 ];
+ assert.ok( getOrderDependencyLength() === 2, 'two expected' );
propertyStateHandler.registerPhetioOrderDependency( propertyB, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 3, 'three expected' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 0 ] === aToBDependency, 'push expected instead of unshift2' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 1 ] === aToCDependency, 'push expected instead of unshift3' );
- const bToCDependency = propertyStateHandler.propertyOrderDependencies[ 2 ];
+ assert.ok( getOrderDependencyLength() === 3, 'three expected' );
propertyStateHandler.unregisterOrderDependenciesForProperty( propertyA );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 1, 'a was in two' );
- assert.ok( propertyStateHandler.propertyOrderDependencies[ 0 ] === bToCDependency, 'push expected instead of unshift3' );
+ assert.ok( getOrderDependencyLength() === 1, 'a was in two' );
propertyStateHandler.unregisterOrderDependenciesForProperty( propertyB );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 0, 'none now' );
+ assert.ok( getOrderDependencyLength() === 0, 'none now' );
propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
propertyStateHandler.registerPhetioOrderDependency( propertyB, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 2, 'none now' );
+ assert.ok( getOrderDependencyLength() === 2, 'none now' );
propertyStateHandler.unregisterOrderDependenciesForProperty( propertyC );
- assert.ok( propertyStateHandler.propertyOrderDependencies.length === 0, 'none now' );
+ assert.ok( getOrderDependencyLength() === 0, 'none now' );
propertyA.dispose();
propertyB.dispose();
Index: js/DerivedPropertyTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/DerivedPropertyTests.js (revision 0563f589d8e3660e93bad4493cc5cc84028ea678)
+++ js/DerivedPropertyTests.js (date 1597280378200)
@@ -135,8 +135,8 @@
const parentTandem = Tandem.GENERAL;
- const originalOrderDependencyLength = propertyStateHandlerSingleton.propertyOrderDependencies.length;
- const getOrderDependencyLength = () => propertyStateHandlerSingleton.propertyOrderDependencies.length - originalOrderDependencyLength;
+ const originalOrderDependencyLength = propertyStateHandlerSingleton.getNumberOfOrderDependencies();
+ const getOrderDependencyLength = () => propertyStateHandlerSingleton.getNumberOfOrderDependencies() - originalOrderDependencyLength;
const firstProperty = new Property( 1, {
tandem: parentTandem.createTandem( 'firstProperty' ),
|
|
I am feeling good enough to commit tonight. I don't think it is perfect, but all axon unit tests are passing, as well as fuzzing state wrappers for CAF,GP, and fricition. Also aqua standalone fuzzing for both brands looks right. There are still some TODOs, and I think that we will find better ways to conduct the data structures. I was a bit confused by this screenshot, taken of the upstream sim in CAF state wrapper, notice that there is one more dependency in the "before" direction as opposed to the "after". I don't think it is a bug, since you can have a multi-to-one relationship in a directional manner, but I want to think about it more: I'm exciting to keep refining this. I also haven't done any performance-related testing to see the speed bump (in a good way). |
zepumph
added a commit
that referenced
this issue
Aug 13, 2020
|
I just committed only to see that Natural selection is having a problem with this change set. Tagging @pixelzoom so that he is aware that there is a fuzzing error related to this work that is only in NS from what I see. I will look into it tomorrow. Overall I'm happy with the progress, and I think it is better than not to push forward. I put a lot of TODOs in |
|
Thanks for the heads up @zepumph. I've created issue phetsims/natural-selection#175 to track in natural-selection. FYI, this blocks publication of the next prototype RC, which we planned to do out of master. So labeling as "block publication". |
zepumph
added a commit
that referenced
this issue
Aug 14, 2020
zepumph
added a commit
that referenced
this issue
Aug 14, 2020
zepumph
added a commit
that referenced
this issue
Aug 14, 2020
|
The obvious bug is fixed above. I'm not sure though why that wasn't failing anything. That seems like a red flag to me. |
After further investigation, it looks like that is only being used during error logging. Still good to fix, but not as monumental of a bug as I originally thought. |
|
Everything has been completed here. @samreid anything else? |
zepumph
added a commit
that referenced
this issue
Aug 27, 2020
|
I didn't see responses or commits for the other review recommendations in #316 (comment), did I just look in the wrong place? |
|
See d96cb12. Everything should be fixed. |
|
Thanks, sorry I missed that. Regarding this comment:
[ mapPair.beforeMap, mapPair.afterMap ].forEach( map => {
The current code is this.mapPairs.forEach( mapPair => {
[ mapPair.beforeMap, mapPair.afterMap ].forEach( map => {
if ( map.has( phetioIDToRemove ) ) {
map.get( phetioIDToRemove ).forEach( phetioID => {
const setOfAfterMapIDs = map.otherMap.get( phetioID );
setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove );
// Clear out empty entries to avoid having lots of empty Sets sitting around
setOfAfterMapIDs.size === 0 && map.otherMap.delete( phetioID );
} );
}
map.delete( phetioIDToRemove );
} );
} )Should this call a method in this.mapPairs.forEach( mapPair => mapPair.unregisterOrderDependency( property ) );This would mirror the existing method |
|
I'm not clear on what feedback you want from me. I have not been following the implementation of this, and I have no familiarity with PropertyStateHandler. Also a question... On 8/19/2020 in phetsims/natural-selection#140 (comment), I was told that this issue was ready to test, so @samreid and I did test. Has there been further performance improvement since then? Do I need to reopen that issue and re-test? |
|
Sorry, I inadvertently assigned @pixelzoom when I meant to assign @zepumph, my mistake. I'm not aware of any changes in this issue that warrant retesting. |
zepumph
added a commit
that referenced
this issue
Aug 28, 2020
|
Thank you for spoon feeding me there, I needed it! I agree that is much better. How about this? |
|
I inspected the commit and it looks good. I also ran a memory test on Natural Selection, and did not see any leaks. Nice work, thanks! Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In phetsims/natural-selection#140, we're investigating big performance differences in
brand=phetvsbrand=phet-iofor Natural Selection.In phetsims/natural-selection#140 (comment):
The text was updated successfully, but these errors were encountered: