Lag problem on Firefox when hands are near proportion

[brooklynlash]

Test device
Lenovo ThinkPad

Operating System
Win10

Browser
Firefox

Problem description
This is for phetsims/qa#609
Not sure if this is machine-dependent, but there is a performance issue when moving a hand side-to-side when hands are near proportion.

Visuals

Troubleshooting information:
!!!!! DO NOT EDIT !!!!!
Name: ‪Ratio and Proportion‬
URL: https://phet-dev.colorado.edu/html/ratio-and-proportion/1.0.0-rc.2/phet/ratio-and-proportion_all_phet.html
Version: 1.0.0-rc.2 2021-02-13 05:25:44 UTC
Features missing: touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0
Language: en-US
Window: 1728x858
Pixel Ratio: 2.2222222222222223/1
WebGL: WebGL 1.0
GLSL: WebGL GLSL ES 1.0
Vendor: Mozilla (Mozilla)
Vertex: attribs: 16 varying: 30 uniform: 4095
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

[zepumph]

I did a performance test in chrome by spamming a hand back and forth. It often went through the "in proportion" state, but not always. The timing on it shows that 3% (of 5400 ms) was taken up with PDOMUtils.setTextContent. Basically because of these two multilinks:

EDIT this one is not contributed, just the one below:https://github.com/phetsims/ratio-and-proportion/blob/9cfb1cc91edeed9cb06bcff713d8d55db1ca7ab7/js/common/view/BothHandsPDOMNode.js

ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.js

Lines 63 to 81 in ffe99a2

	// This derivedProperty is already dependent on all other dependencies for getStateOfSimString
	Property.multilink( [
	targetRatioProperty,
	tickMarkViewProperty,
	ratioTupleProperty,
	ratioFitnessProperty
	], ( currentTargetRatio, tickMarkView, currentTuple ) => {
	stateOfSimNode.innerContent = StringUtils.fillIn( ratioAndProportionStrings.a11y.discover.screenSummary.qualitativeStateOfSim, {
	ratioFitness: ratioDescriber.getRatioFitness( false ), // lowercase
	currentChallenge: ratioToChallengeNameMap.get( currentTargetRatio ).lowercase,
	distance: handPositionsDescriber.getDistanceRegion( true )
	} );
	leftHandBullet.innerContent = StringUtils.fillIn( ratioAndProportionStrings.a11y.leftHandBullet, {
	position: handPositionsDescriber.getHandPositionDescription( currentTuple.antecedent, tickMarkView )
	} );
	rightHandBullet.innerContent = StringUtils.fillIn( ratioAndProportionStrings.a11y.rightHandBullet, {
	position: handPositionsDescriber.getHandPositionDescription( currentTuple.consequent, tickMarkView )
	} );
	} );

Basically every time any of these change at all, we redraw all the innerContent. ParallelDOM.setInnerContent already checks to make sure it isn't setting duplicate content. Thus I fell like we are at a space where we are limited by HTML setting if we wanted to shave down these 130ms at all.

I was surprised to not see something about mouse input, especially because I make a fair amount of scrap with Vector2 in RatioHalf.js. Especially with the listener forwarding that using DynamicProperty creates here.

Personally, with my chrome 4x throttled, I didn't find performance trouble during this case. @brooklynlash, could you send me some deets about what "Lenovo ThinkPad" is, like processor/year/gpu/cpu stuff?

[zepumph]

I'm going to change my estimate for how much this Multilink in DiscoverScreenSummaryNode is taking. Here is the screenshot where I highlighted the pieces I think are effected by this single multilink:

Every time this multilink is called, upwards of three redraws for paragraphs in the screen summary. This not only takes time to call HTMLElement.textContent, but also that cascades to to update layout and style on the HTML style. My current understanding is that "Recalculate Style" is coming from setting that text content. I would like to figure out why this is calling style operations.

[zepumph]

I found that even though we added positionInPDOM, there is still some default style added to elements that aren't posistioned. like so:

<input data-trail-id="1127-1128-1290-1297-1292-1402-1298-1299" id="1127-1128-1290-1297-1292-1402-1298-1299" data-focusable="true" class="a11y-pdom-element" type="range" aria-orientation="vertical" aria-valuetext="hands, at challenge ratio" min="0" max="1" step="0.01" aria-valuenow="0.2" style="top: 0px; left: 0px; width: 1px; height: 1px;">

@jessegreenberg , please note this patch, I don't know if it is safe, but I feel like the class a11y-pdom-element covers this style attribute. Can we get rid of this? Can you explain what we need both for?

Index: js/accessibility/pdom/PDOMPeer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/PDOMPeer.js b/js/accessibility/pdom/PDOMPeer.js
--- a/js/accessibility/pdom/PDOMPeer.js	(revision e35a38f68d7b793ebf745c2f7aec7ae099fd36fa)
+++ b/js/accessibility/pdom/PDOMPeer.js	(date 1614047872282)
@@ -936,12 +936,12 @@
       }
       else {
 
-        // not positioning, just move off screen
-        scratchSiblingBounds.set( PDOMPeer.OFFSCREEN_SIBLING_BOUNDS );
-        setClientBounds( this._primarySibling, scratchSiblingBounds );
-        if ( this._labelSibling ) {
-          setClientBounds( this._labelSibling, scratchSiblingBounds );
-        }
+        // // not positioning, just move off screen
+        // scratchSiblingBounds.set( PDOMPeer.OFFSCREEN_SIBLING_BOUNDS );
+        // setClientBounds( this._primarySibling, scratchSiblingBounds );
+        // if ( this._labelSibling ) {
+        //   setClientBounds( this._labelSibling, scratchSiblingBounds );
+        // }
       }
     }
 

[zepumph]

@jessegreenberg and I discussed some possible improvements for PDOM performance improvement. All of which seem quite scary to do in RC. As a result we don't want this to block RaP, but we will create an issue in scenery to investigate.

1. Separate out position-focused style from the a11y-pdom-element rules,
2. Apply the above patch and not set clientBounds style on non positioned elements.
3. Look into batching attribute setting to a single time per frame.
4. Try to minimize the amount that we ask for data from the DOM element, and instead just try to keep that in the PDOM trait.

This is all speculation, as Chrome performance tracking was hard to reproduce the above lag with throttling. I will keep working on it.

[zepumph]

I created phetsims/scenery#1163. Unassigning.

[zepumph]

After more investigation, I found that really the solution is to limit the number of times textContent is set to a PDOM element per frame. I made phetsims/scenery#1439 to implement it, but I don't think this issue warrants it at this time. I want to tag @emily-phet in case she would rather implement the PDOM change over in phetsims/scenery#1439 as part of publishing @emily-phet. I recommend deferring this issue personally. Let me know!

[emily-phet]

@zepumph Deferring is fine with me.

[zepumph]

Sounds good! Thanks.

For next time:

After more investigation, I found that really the solution is to limit the number of times textContent is set to a PDOM element per frame.

To be clear. I never experienced any performance issues, but I did use the performance tab to see how much computation was going into setting textContent. I believe that they are linked, but have not totally confirmed just yet.