Add support for "transactions" in Property #209
Comments
|
After discussing this with @jonathanolson @samreid @chrisklus @mbarlow12, we came up with a strategy in which you can start an arbitrary number of transactions, and listeners will only be called once all of the transactions have ended (completed). Once |
|
What is next here? @samreid should this be reviewed by someone outside the phet-io team? |
|
@jonathanolson already took a quick look at it, but it would be ideal if @pixelzoom can schedule time to take a look. I'm sure he is going to have lots of good questions when he comes across it in the future, would be good to plan for it instead of having him stumble across it by accident. The code to review is in Property.js, you can search for "transaction" or review the aforementioned commits. The unit tests are in PropertyTests.js--also a good place to see a simple sample usage. |
|
I question whether "transaction" is the correct name for this feature, since it makes me think of database transactions, which is an atomic unit of work that typically involves multiple (different) actions. A Property transaction is simply a way of making multiple I don't have a better name to suggestion. But regardless of what this feature is called, this last point (being able to And some minor suggested changes to endTransaction: function() {
- assert && assert( this.transactionCount >= 1, 'end transaction called without corresponding startTransaction' );
+ assert && assert( this.transactionCount > 0, 'end transaction called without corresponding startTransaction' );
this.transactionCount--;
if ( this.transactionCount === 0 ) {
this._notifyListeners( this.transactionOriginalValue );
+ this.transactionOriginalValue = null;
}
}, |
|
@samreid and I discussed this via Slack. The current solution does not address the stated goal of being "able to set multiple Properties atomically". And it results in change notifications being in the wrong order in the PhET-iO data stream. Examples that @samreid and I discussed: |
|
For API, @pixelzoom and i preferred: // Option 5 -- able to support other types
const transaction = new Transaction([a,b]).start(); // chaining
a.value = 7;
transaction.end();Here were some other considerations: // Option 1
id = Property.startTransaction([a,b]);
a.value = 3;
Property.endTransaction(); // how does it know which transaction to end? Is it just a stack?
// Option 1.5 BAD
Property.startTransaction([a,b]);
a.value = 3;
Property.endTransaction([a]); // how does it know which transaction to end? Is it just a stack?
// Option 2 (*)
const transaction = Property.startTransaction([a,b]); // {Transaction} or maybe {PropertyTransaction}
a.value = 3;
transaction.end();
// Option 3
const endTransaction = Property.startTransaction([a,b]);
a.value = 3;
endTransaction();
// Option 4
const transaction = new Transaction([a,b]);
transaction.start();
a.value = 7;
transaction.end();
// Option 5 -- able to support other types WINNER!
const transaction = new Transaction([a,b]).start(); // chaining
a.value = 7;
transaction.end(); |
|
Would we ever want to "withhold notifications" from Emitter? Maybe one day, but we have no need for it at the moment. We discussed this case: const e = new Emitter();
e.startTransaction(); // nothing happens
e.emit('hello'); // nothing happens
e.emit('bye'); // nothing happens
e.emit(123); // nothing happens
e.endTransaction(); // hello bye 123 |
|
I tried a general implementation of Transaction (not tied to Property), but it became complicated because Property would need to know how to push/pop its initial values (at the beginning of transactions) so it would know whether to send notifications or not at the end of a transaction. Supporting that for singly-nested Transactions would be easy, but pushing/popping those seems complex and like it should not be part of Property. Instead, I'll look into writing a Property-specific Transaction class that can encapsulate that logic. UPDATE: I wasn't following the defer pattern properly. See following comments. |
var a = new Property(0);
var x = new Transaction(a).start()
a.value = 7;
var y = new Transaction(a).start() // captures initial value as 7
x.end(); // a still muted
a.value = 12;
a.value = 7;
y.end(); // no notification because transaction y thinks initial value was 7
// This doesn't seem like the desired behavior.
// This indicates that the Property should store its initial value before muting. |
|
I implemented this system as prescribed in #209 (comment) and added this test: const a = new Property( 0 );
const b = new Property( 0 );
const sum = new DerivedProperty( [ a, b ], ( a, b ) => a + b );
const transaction = new Transaction( [ a, b ] ).start();
const log = [];
a.lazyLink( a => log.push( 'a changed to ' + a ) );
b.lazyLink( b => log.push( 'b changed to ' + b ) );
sum.lazyLink( sum => log.push( 'sum changed to ' + sum ) );
a.value = 1;
b.value = 2;
a.value = 5;
b.value = 3;
transaction.end();However, this results in:
That is because const a = new Property( 0 );
const b = new Property( 0 );
const transaction = new Transaction( [ a, b ] ).start();
const log = [];
a.lazyLink( a => log.push( 'a changed to ' + a ) );
b.lazyLink( b => log.push( 'b changed to ' + b ) );
const sum = new DerivedProperty( [ a, b ], ( a, b ) => a + b );
sum.lazyLink( sum => log.push( 'sum changed to ' + sum ) );
a.value = 1;
b.value = 2;
a.value = 5;
b.value = 3;
transaction.end();but this resulted in:
Maybe this is because all the listeners are downstream, and maybe I should be checking the phet-io data stream. It seems it will have the same problem because
we would have to add another "phase" in ending the transaction, like so: // End defers that we started.
var actions = this.properties.map( e => e.popDefer() );
// NEW PHASE that sends out phetioEmitEvent
// ...
// If Property value is different than when initially deferred, and Property is no longer deferred, notify
// listeners
actions.forEach( action => action() );However, that seems like it will make the phetioDataStream deviate from other callback streams. I'll stash a patch here in case anybody else wants to take a look before I commit. |
Alternatively, we could add the DerivedProperty to the Transaction. Transaction could automatically sort the DerivedProperties to appear last in the list. Even though the notification order is still incorrect in the patch above, it still seems preferable to the prior implementation because the Properties retain their old values until the transaction is complete. |
That will not work. After |
|
This test: QUnit.test( 'basic tests', function( assert ) {
assert.ok( true, 'token test' );
const a = new NumberProperty( 0, { tandem: Tandem.generalTandem.createTandem( 'aProperty' ) } );
const b = new NumberProperty( 0, { tandem: Tandem.generalTandem.createTandem( 'bProperty' ) } );
const sum = new DerivedProperty( [ a, b ], ( a, b ) => a + b, {
phetioType: DerivedPropertyIO( NumberIO ),
tandem: Tandem.generalTandem.createTandem( 'sumProperty' )
} );
debugger;
const transaction = new Transaction( [ a, b, sum ] ).start();
const log = [];
a.lazyLink( a => console.log( 'a changed to ' + a ) );
b.lazyLink( b => console.log( 'b changed to ' + b ) );
sum.lazyLink( sum => console.log( 'sum changed to ' + sum ) );
a.value = 1;
b.value = 2;
a.value = 5;
b.value = 3;
assert.equal( log.length, 0, 'nothing should be logged yet' );
transaction.end();
// phet-io data stream:
// a changed to 5
// b changed to 3
// sum changed 8
// d changed to 20
} );Is outputting this result: |
|
I don't think we will be able to use the new Transaction class to solve the problem in https://github.com/phetsims/phet-io-wrappers/issues/229, because some new Properties may be created during traversal of the set state and they will not be in the Transaction. Perhaps we can use pushDefer/popDefer though. |
|
Is anyone else concerned about adding this new complexity to Property? |
|
I'm concerned, but I don't see a preferable alternate solution. So everyone is aware of the anticipated complexity increase, here is a patch that shows proposed changes to Property for this feature: Update: I realized that is not a great patch, because it shows the diff between the "Property.startTransaction" and "Property.pushDefer" paradigms. But it gives the gist. |
|
I reviewed the proposal with @zepumph @jessegreenberg @chrisklus @jbphet and @jonathanolson today and @jonathanolson pointed out this problematic case: // Imagine 2 properties
const a = new Property();
const b = new Property();
// in some library code
a.pushDefer();
// in my code
a.pushDefer();
b.pushDefer();
a.value = 12;
b.value = 123;
actions.push(a.popDefer());
actinos.push(b.popDefer());
//actions, etc.
actions.forEach(action=>action()); // send notifictanios if changed
// Problematic because you have inconsistent state for a and bIt was proposed that we purposefully limit the number of "defers" that a Property can have to 1 to avoid this pitfall. There was general agreement that this pattern seemed acceptable. I'll work on the fix above and some cleaning up to commit to master for further review and development. |
|
I notified dev-public:
I tested the following things before commit:
No issues noted. Committing. Follow up work:
|
|
Transaction appears unused. Can it be deleted? |
|
Fine with me to delete, we can always bring it back if needed. Also, @pixelzoom expressed concern for added complexity in #209 (comment). |
|
Using setDeferred directly and/or the solution from #276 seems more promising than Transaction, so I'll delete Transaction and its tests. |
|
I think I'll close this issue and we can continue in #276 |
From https://github.com/phetsims/phet-io-wrappers/issues/229, phet-io state has need to be able to set multiple Properties atomically. It would be nice to be able to tell a Property to hold off on notifying listeners until the transaction is complete.
The text was updated successfully, but these errors were encountered: