improve responsiveness of "Start Over" button #140
Comments
|
Query parameters
|
|
First, let's quantify performance. Steps:
For
For
|
|
I completed the first 5 rows in each table. @KatieWoe could you please complete the remaining rows in the above tables? |
|
|
|
@samreid and I paired on this via Zoom. We identified 2 performance bottlenecks in code that runs only for PhET-iO brand.
To investigate what kind of performance improvement we might get, we commented out the body of patchIndex: js/PhetioStateEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetioStateEngine.js (revision 675b683ee10353c13d7c162beaede92a47fe6dff)
+++ js/PhetioStateEngine.js (date 1595978134546)
@@ -152,7 +152,7 @@
// Iterate over the elements of the API in order. The order of appearance in the API file determines the order of
// save & load. This is important for sims that have order dependency between saved elements.
- this.phetioEngine.phetioIDs.forEach( phetioID => {
+ this.phetioEngine.getPhetioIDs().forEach( phetioID => {
const phetioObject = this.phetioEngine.phetioObjectMap[ phetioID ];
if ( phetioObject.phetioState && !phetioObject.phetioIsArchetype ) {
Index: js/phetioEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/phetioEngine.js (revision 675b683ee10353c13d7c162beaede92a47fe6dff)
+++ js/phetioEngine.js (date 1595978106934)
@@ -67,7 +67,7 @@
this.phetioObjectMap = {};
// @public (phet-io internal) {string[]} Array of all of the registered phetioIDs in the order they were registered.
- this.phetioIDs = [];
+ // this.phetioIDs = [];
// @private {Emitter} - Signifies when a new PhetioObject is added to PhET-iO.
this.phetioTypesAddedEmitter = new Emitter( {
@@ -278,7 +278,7 @@
assert && assert( !this.simConstructed, 'baselines are nulled out after sim is constructed. Call this before then.' );
const result = {};
- this.phetioIDs.forEach( phetioID => {
+ this.getPhetioIDs().forEach( phetioID => {
const phetioObject = this.phetioObjectMap[ phetioID ];
// The metadata for individual dynamic elements is not part of the API, but it's important to record this API
@@ -298,7 +298,7 @@
getPhetioElementsMetadata() {
const result = {};
- this.phetioIDs.forEach( phetioID => {
+ this.getPhetioIDs().forEach( phetioID => {
const phetioObject = this.phetioObjectMap[ phetioID ];
// The metadata for individual dynamic elements is not part of the API, but it's important to record this API
@@ -326,7 +326,7 @@
};
// Iterate through all PhetioObjects and add all associated types for each.
- this.phetioIDs.forEach( phetioID => {
+ this.getPhetioIDs().forEach( phetioID => {
if ( !this.phetioObjectMap[ phetioID ].phetioDynamicElement ) {
getAllRelatedTypes( this.phetioObjectMap[ phetioID ].phetioType ).forEach( addType );
}
@@ -437,7 +437,7 @@
this.phetioObjectMap[ phetioID ] = phetioObject;
// Keep track of the order things were added, so changes can be applied in the same order
- this.phetioIDs.push( phetioID );
+ // this.phetioIDs.push( phetioID );
// After the PhetioObject construction is finished, notify listeners that it is ready for interoperability.
timer.runOnNextFrame( () => { // eslint-disable-line bad-sim-text
@@ -527,7 +527,7 @@
* @public (phet-io-internal) called only by phetioEngine and PhetioEngineIO
*/
getPhetioIDs() {
- return this.phetioIDs;
+ return Object.keys( this.phetioObjectMap );
}
/**
@@ -579,10 +579,10 @@
delete this.phetioObjectMap[ phetioID ];
// Update the array of phetioIDs
- const index = this.phetioIDs.indexOf( phetioID );
- if ( index > -1 ) {
- this.phetioIDs.splice( index, 1 );
- }
+ // const index = this.phetioIDs.indexOf( phetioID );
+ // if ( index > -1 ) {
+ // this.phetioIDs.splice( index, 1 );
+ // }
// push onto a stack to call at end of frame
phetioAPIValidation.onPhetioObjectRemoved( phetioObject );We demonstrated big performance improvements on iPad6 + Safari 13, MacBook Pro + Safari 13, and Mac Book Pro + Chrome 84. And we demonstrated that improvements for both of these bottlenecks are needed. We decided to create issues in axon (for PropertyStateHandler) and phet-io (for phetioEngine) repos, and involve @zepumph . |
|
@KatieWoe no need to do performance testing on additional platforms. There are clear bottlenecks in PhET-iO, and we'll test after they are resolved. This issue is on hold until phetsims/axon#316 and https://github.com/phetsims/phet-io/issues/1689 are addressed. Then I think we should have the PhET-iO team take another look to see if there are any additional PhET-iO bottlenecks that can be improved. |
|
In #60 (comment), @samreid asked:
|
|
@samreid The performance improvement in phetsims/phet-io#1689 is on my radar, and I was hoping to test performance this past week. But publishing a prototype has been given higher priority, and that prototype does not include PhET-iO. So at this point I really don't know when I'll be getting to this. |
|
Testing the improvements made in phetsims/phet-io#1689 ... Following the steps in #140 (comment), here are test results for the 5 configurations that I tested previously. I took an average of 3 runs, and I included the previous average in the table. For
For
As expected, The next step would be to have @KatieWoe test on the other platforms (especially the Lovelace Chromebook). But I don't want to incur that cost until I verify that the iO team doesn't have plans to make any other general improvements for this issue. @samreid thoughts? |
On Slack, @zepumph and @samreid identified these 2 issues as relevant, "both about removing things and unregistering constraints or listeners": phetsims/axon#316 Labeling this issue as "on-hold" until those issues are addressed. |
|
A very unscientific attempt at emulating the performance of the Lovelace Chromebook test device... I tested 1.1.0-dev.6 The average of 3 runs was 1538 ms, which would be considered unacceptable performance. And I suspect that Lovelace will be a little slower than that. |
|
I ran this on my iPad7 with a built version, running standalone in PhET-iO brand, and ?showTimes, and it reported: time to mate: 348ms On a Lenovo N23 Yoga Chromebook time to mate: 1227ms |
|
On a build phet-io standalone off of master on an iPad7: to go to the 6th gen: "time to mate = 547 ms" second time (no refresh): third time (no refresh): @pixelzoom edit: So @zepumph's average is 162 ms for "Start Over". |
|
Following the testing steps in #140 (comment) with 1.1.0-dev.12 ... For
|
|
So based on the tests from @samreid @zepumph and me (in the 3 comments above), it looks like "Start Over" is acceptable on those 5 devices, including @samreid's Lenovo N23 Yoga Chromebook. Next step is to test on Lovelace Chromebook and any other test devices known to be "performance challenged". @ariel-phet what is the availability of test resources for this task? |
|
@pixelzoom I think we can have @phet-steele do a chromebook test. Considering the vast improvements your are seeing, I think if that test is solid, we can call it good. |
|
Following the testing steps in #140 (comment) with 1.1.0-dev.12 ... For
"Feynman - Chromebook" used in @phet-steele's tests is an Acer CB5-312T series, Model # N16Q10, aka Acer Chromebook 13. https://www.acer.com/ac/en/AE/content/model/NX.GL4EM.002, 2.1GHz Quad-core with 13.3" screen. |
|
In the above commit, I made the "time to Start Over" display a permanent feature of the sim, enabled via |
|
Over in #60 (comment), @samreid reported a 5.5% performance improvement in Following the testing steps in #60 (comment) with 1.1.0-dev.14 ... For
So I'm seeing a performance improvement across the board here, which I can rationalize as ~5% on macOS Chrome. Over in #60 I did not see a 5% improvement, and iO performance was significantly worse on iPad. @samreid is the improvement made to |
|
I think |
|
Thanks for clarifying. |
|
I agree that this is acceptable for performance. Thanks for doing all of this to make it as zippy as possible! |
Responsiveness of the "Start Over" button was identified as an issue in #60 (comment). It could use improvement for brand=phet, and is unacceptable with brand=phet-io.
The text was updated successfully, but these errors were encountered: