search

phetsims / density-buoyancy-common

Common code for the Density and Buoyancy simulations
GNU General Public License v3.0
0 stars 2 forks source link

Improve Boat and Bottle and BottleView implementation details #144

Closed samreid closed 1 month ago

samreid commented 4 months ago

From TODOs in https://github.com/phetsims/density-buoyancy-common/issues/86

samreid commented 4 months ago

We would like to consult with @jonathanolson for 26 minutes or so to make progress on these.

samreid commented 3 months ago

We consulted with @jonathanolson for 26 minutes, and removed unnecessary TODOs. There are 2 left and @zepumph and I know how to proceed.

samreid commented 3 months ago

I'll continue here

samreid commented 3 months ago

This patch demonstrates a savings of about 40,000 Vector2 allocations over 10 seconds of using the boat.

```diff Subject: [PATCH] Remove babel from the directories that require precommit hooks, see https://github.com/phetsims/chipper/issues/1445 --- Index: kite/js/segments/Cubic.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/kite/js/segments/Cubic.ts b/kite/js/segments/Cubic.ts --- a/kite/js/segments/Cubic.ts (revision d780083bfeb93b5e110c0554dad80adfe85e81b2) +++ b/kite/js/segments/Cubic.ts (date 1718889800102) @@ -199,7 +199,7 @@ * * This method is part of the Segment API. See Segment.js's constructor for more API documentation. */ - public positionAt( t: number ): Vector2 { + public positionAt( t: number, returnValue?: Vector2 ): Vector2 { assert && assert( t >= 0, 'positionAt t should be non-negative' ); assert && assert( t <= 1, 'positionAt t should be no greater than 1' ); @@ -210,10 +210,20 @@ const mtt = 3 * mt * t * t; const ttt = t * t * t; - return new Vector2( - this._start.x * mmm + this._control1.x * mmt + this._control2.x * mtt + this._end.x * ttt, - this._start.y * mmm + this._control1.y * mmt + this._control2.y * mtt + this._end.y * ttt - ); + const x = this._start.x * mmm + this._control1.x * mmt + this._control2.x * mtt + this._end.x * ttt; + const y = this._start.y * mmm + this._control1.y * mmt + this._control2.y * mtt + this._end.y * ttt; + + window.count = window.count || 0; + window.count++; + + window.savings = window.savings || 0; + if ( !!returnValue ) { + window.savings++; + } + + console.log( 'savings', window.savings, 'count', window.count ); + + return returnValue ? returnValue.setXY( x, y ) : new Vector2( x, y ); } /** Index: density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts b/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts --- a/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts (revision 2c16d7c6e988d7e2ffb5a3638f88f3271cef8d25) +++ b/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts (date 1718889670618) @@ -134,9 +134,12 @@ const d = p0.y - blockHalfWidth / scale; const ts = phet.dot.Utils.solveCubicRootsReal( a, b, c, d ); + + const xz = new Vector2( 0, 0 ); + ts.forEach( ( t: number ) => { if ( t >= 0 && t <= 1 ) { - const xz = cubic.positionAt( t ); + cubic.positionAt( t, xz ); interiorPoints.push( new Vector2( xz.x, y ) ); } } ); @@ -284,7 +287,7 @@ * @returns - Whether the water is completely filled */ public static fillWaterVertexArray( waterY: number, boatX: number, boatY: number, liters: number, poolBounds: Bounds3, positionArray: Float32Array, wasFilled: boolean ): boolean { - + const outsideBottomY = -BoatDesign.DESIGN_BOAT_HEIGHT; const scale = BoatDesign.getScaleForLiters( liters ); const designY = BoatDesign.getDesignY( boatY, scale ); @@ -326,13 +329,15 @@ poolBounds.maxX, poolBounds.maxZ ), waterY ); + const p0 = new Vector2( 0, 0 ); + const p1 = new Vector2( 0, 0 ); + for ( let i = 0; i < CROSS_SECTION_SAMPLES; i++ ) { const t0 = i / CROSS_SECTION_SAMPLES; const t1 = ( i + 1 ) / CROSS_SECTION_SAMPLES; - // TODO: reduce these allocations? https://github.com/phetsims/density-buoyancy-common/issues/144 - const p0 = cubic.positionAt( t0 ); - const p1 = cubic.positionAt( t1 ); + cubic.positionAt( t0, p0 ); + cubic.positionAt( t1, p1 ); const p0x = ( p0.x - BoatDesign.DESIGN_CENTROID.x ) * scale + boatX; const p0z = p0.y * scale; @@ -393,12 +398,15 @@ const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( designY ), true ); const cubic = new Cubic( ...controlPoints ); + const p0 = new Vector2( 0, 0 ); + const p1 = new Vector2( 0, 0 ); + for ( let i = 0; i < CROSS_SECTION_SAMPLES; i++ ) { const t0 = i / CROSS_SECTION_SAMPLES; const t1 = ( i + 1 ) / CROSS_SECTION_SAMPLES; - const p0 = cubic.positionAt( t0 ); - const p1 = cubic.positionAt( t1 ); + cubic.positionAt( t0, p0 ); + cubic.positionAt( t1, p1 ); const p0x = ( p0.x - BoatDesign.DESIGN_CENTROID.x ) * scale; const p0z = p0.y * scale; @@ -430,9 +438,12 @@ const designY = ( -BoatDesign.DESIGN_BOAT_HEIGHT + ( isInside ? BoatDesign.DESIGN_WALL_THICKNESS : 0 ) ) * sample / ( heightSamples - 1 ); const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( designY ), isInside ); const cubic = new Cubic( ...controlPoints ); + + const point = new Vector2( 0, 0 ); + return _.range( 0, parametricSamples ).map( pSample => { const t = pSample / ( parametricSamples - 1 ); - const point = cubic.positionAt( t ); + cubic.positionAt( t, point ); return BoatDesign.designToModel( new Vector3( point.x, designY, point.y ), liters ); } ); } ); Index: kite/js/segments/Segment.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/kite/js/segments/Segment.ts b/kite/js/segments/Segment.ts --- a/kite/js/segments/Segment.ts (revision d780083bfeb93b5e110c0554dad80adfe85e81b2) +++ b/kite/js/segments/Segment.ts (date 1718889489708) @@ -69,17 +69,17 @@ // if the method name is found on the segment, it is called with the expected signature // function( options ) : Array[Segment] instead of using our brute-force logic methodName?: KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]>; + KeysMatching Segment[]> | + KeysMatching Segment[]> | + KeysMatching Segment[]> | + KeysMatching Segment[]> | + KeysMatching Segment[]>; }; type PiecewiseLinearOrArcRecursionOptions = { curvatureThreshold: number; errorThreshold: number; - errorPoints: [number, number]; + errorPoints: [ number, number ]; }; type PiecewiseLinearOrArcOptions = { ```

@jonathanolson and @zepumph and I discussed that we should make a more holistic approach to supporting mutation in Segment.ts, without just adding this to an isolated part. Here are the methods in Segment.ts that return a Vector2:

  public abstract get start(): Vector2;
  public abstract get end(): Vector2;
  public abstract get startTangent(): Vector2;
  public abstract get endTangent(): Vector2;
  public abstract positionAt( t: number ): Vector2;
  public abstract tangentAt( t: number ): Vector2;

We discussed that we should add an optional returnValue?:Vector2 to the latter 4 of these. However, after reviewing the implementations of other Segment subtypes, it looks like a lot of work to rewrite the arithmetic to avoid that allocation, for example, EllipticalArc:

  public positionAt( t: number ): Vector2 {
    return this.positionAtAngle( this.angleAt( t ) );
  }
  public positionAtAngle( angle: number ): Vector2 {
    return this.getUnitTransform().transformPosition2( Vector2.createPolar( 1, angle ) );
  }

Or in Line:


  public positionAt( t: number ): Vector2 {
    return this._start.plus( this._end.minus( this._start ).times( t ) );
  }

So it would be odd to add a returnValue?:Vector2 parameter and still generate other garbage along the way. Also, if we continue with this, we may want to put more effort into having kite internally call the allocation-free ones. However, I saw that Line.positionAt is only called 24 times during startup of buoyancy (and not called during the runtime).

I'm also not convinced that complicating the API and implementation of all this is a good tradeoff for the performance boost in this case which can be attained in other ways. Let's chat before continuing.

samreid commented 1 month ago

I'll double check and refresh the patch above

samreid commented 1 month ago

Refreshed patch, with count/savings instrumentation:

```diff Subject: [PATCH] Remove babel from the directories that require precommit hooks, see https://github.com/phetsims/chipper/issues/1445 --- Index: kite/js/segments/Segment.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/kite/js/segments/Segment.ts b/kite/js/segments/Segment.ts --- a/kite/js/segments/Segment.ts (revision d780083bfeb93b5e110c0554dad80adfe85e81b2) +++ b/kite/js/segments/Segment.ts (date 1724190304715) @@ -69,17 +69,17 @@ // if the method name is found on the segment, it is called with the expected signature // function( options ) : Array[Segment] instead of using our brute-force logic methodName?: KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]> | - KeysMatching Segment[]>; + KeysMatching Segment[]> | + KeysMatching Segment[]> | + KeysMatching Segment[]> | + KeysMatching Segment[]> | + KeysMatching Segment[]>; }; type PiecewiseLinearOrArcRecursionOptions = { curvatureThreshold: number; errorThreshold: number; - errorPoints: [number, number]; + errorPoints: [ number, number ]; }; type PiecewiseLinearOrArcOptions = { Index: kite/js/segments/Cubic.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/kite/js/segments/Cubic.ts b/kite/js/segments/Cubic.ts --- a/kite/js/segments/Cubic.ts (revision d780083bfeb93b5e110c0554dad80adfe85e81b2) +++ b/kite/js/segments/Cubic.ts (date 1724190304709) @@ -199,7 +199,7 @@ * * This method is part of the Segment API. See Segment.js's constructor for more API documentation. */ - public positionAt( t: number ): Vector2 { + public positionAt( t: number, returnValue?: Vector2 ): Vector2 { assert && assert( t >= 0, 'positionAt t should be non-negative' ); assert && assert( t <= 1, 'positionAt t should be no greater than 1' ); @@ -210,10 +210,20 @@ const mtt = 3 * mt * t * t; const ttt = t * t * t; - return new Vector2( - this._start.x * mmm + this._control1.x * mmt + this._control2.x * mtt + this._end.x * ttt, - this._start.y * mmm + this._control1.y * mmt + this._control2.y * mtt + this._end.y * ttt - ); + const x = this._start.x * mmm + this._control1.x * mmt + this._control2.x * mtt + this._end.x * ttt; + const y = this._start.y * mmm + this._control1.y * mmt + this._control2.y * mtt + this._end.y * ttt; + + window.count = window.count || 0; + window.count++; + + window.savings = window.savings || 0; + if ( !!returnValue ) { + window.savings++; + } + + console.log( 'savings', window.savings, 'count', window.count ); + + return returnValue ? returnValue.setXY( x, y ) : new Vector2( x, y ); } /** Index: density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts b/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts --- a/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts (revision c43e79ff25451e84a759ef4c09e0311a8b7512a7) +++ b/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts (date 1724205408094) @@ -131,9 +131,12 @@ const d = p0.y - blockHalfWidth / scale; const ts = phet.dot.Utils.solveCubicRootsReal( a, b, c, d ); + + const xz = new Vector2( 0, 0 ); + ts.forEach( ( t: number ) => { if ( t >= 0 && t <= 1 ) { - const xz = cubic.positionAt( t ); + cubic.positionAt( t, xz ); interiorPoints.push( new Vector2( xz.x, y ) ); } } ); @@ -300,14 +303,17 @@ poolBounds.maxX, poolBounds.maxZ ), fluidY ); + const p0 = new Vector2( 0, 0 ); + const p1 = new Vector2( 0, 0 ); + // Fill the vertices around the boat's shape in the fluid cross-section for ( let i = 0; i < CROSS_SECTION_SAMPLES; i++ ) { const t0 = i / CROSS_SECTION_SAMPLES; const t1 = ( i + 1 ) / CROSS_SECTION_SAMPLES; // Generate positions for the current and next sample points along the boat's cross-section - const p0 = cubic.positionAt( t0 ); - const p1 = cubic.positionAt( t1 ); + cubic.positionAt( t0, p0 ); + cubic.positionAt( t1, p1 ); const p0x = ( p0.x - BoatDesign.DESIGN_CENTROID.x ) * scale + boatX; const p0z = p0.y * scale; @@ -368,12 +374,15 @@ const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( designY ), true ); const cubic = new Cubic( ...controlPoints ); + const p0 = new Vector2( 0, 0 ); + const p1 = new Vector2( 0, 0 ); + for ( let i = 0; i < CROSS_SECTION_SAMPLES; i++ ) { const t0 = i / CROSS_SECTION_SAMPLES; const t1 = ( i + 1 ) / CROSS_SECTION_SAMPLES; - const p0 = cubic.positionAt( t0 ); - const p1 = cubic.positionAt( t1 ); + cubic.positionAt( t0, p0 ); + cubic.positionAt( t1, p1 ); const p0x = ( p0.x - BoatDesign.DESIGN_CENTROID.x ) * scale; const p0z = p0.y * scale; @@ -405,9 +414,12 @@ const designY = ( -BoatDesign.DESIGN_BOAT_HEIGHT + ( isInside ? BoatDesign.DESIGN_WALL_THICKNESS : 0 ) ) * sample / ( heightSamples - 1 ); const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( designY ), isInside ); const cubic = new Cubic( ...controlPoints ); + + const point = new Vector2( 0, 0 ); + return _.range( 0, parametricSamples ).map( pSample => { const t = pSample / ( parametricSamples - 1 ); - const point = cubic.positionAt( t ); + cubic.positionAt( t, point ); return BoatDesign.designToModel( new Vector3( point.x, designY, point.y ), liters ); } ); } );
samreid commented 1 month ago

I removed the instrumentation from the patch, and was getting ready to commit this:

```diff Subject: [PATCH] Remove babel from the directories that require precommit hooks, see https://github.com/phetsims/chipper/issues/1445 --- Index: kite/js/segments/Cubic.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/kite/js/segments/Cubic.ts b/kite/js/segments/Cubic.ts --- a/kite/js/segments/Cubic.ts (revision d780083bfeb93b5e110c0554dad80adfe85e81b2) +++ b/kite/js/segments/Cubic.ts (date 1724207122683) @@ -198,8 +198,10 @@ * segment. * * This method is part of the Segment API. See Segment.js's constructor for more API documentation. + * @param t - the distance along the cubic 0<=t<=1 + * @param returnValue - optional vector to store the result in, to avoid allocation */ - public positionAt( t: number ): Vector2 { + public positionAt( t: number, returnValue?: Vector2 ): Vector2 { assert && assert( t >= 0, 'positionAt t should be non-negative' ); assert && assert( t <= 1, 'positionAt t should be no greater than 1' ); @@ -210,10 +212,10 @@ const mtt = 3 * mt * t * t; const ttt = t * t * t; - return new Vector2( - this._start.x * mmm + this._control1.x * mmt + this._control2.x * mtt + this._end.x * ttt, - this._start.y * mmm + this._control1.y * mmt + this._control2.y * mtt + this._end.y * ttt - ); + const x = this._start.x * mmm + this._control1.x * mmt + this._control2.x * mtt + this._end.x * ttt; + const y = this._start.y * mmm + this._control1.y * mmt + this._control2.y * mtt + this._end.y * ttt; + + return returnValue ? returnValue.setXY( x, y ) : new Vector2( x, y ); } /** Index: density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts b/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts --- a/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts (revision c43e79ff25451e84a759ef4c09e0311a8b7512a7) +++ b/density-buoyancy-common/js/buoyancy/model/applications/BoatDesign.ts (date 1724207122672) @@ -118,7 +118,7 @@ const y = ( -BoatDesign.DESIGN_BOAT_HEIGHT + BoatDesign.DESIGN_WALL_THICKNESS ) * sample / ( insideSamples - 1 ); const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( y ), true ); - const cubic = new phet.kite.Cubic( ...controlPoints ); + const cubic = new Cubic( ...controlPoints ); const p0 = controlPoints[ 0 ]; const p1 = controlPoints[ 1 ]; @@ -130,10 +130,13 @@ const c = -3 * p0.y + 3 * p1.y; const d = p0.y - blockHalfWidth / scale; - const ts = phet.dot.Utils.solveCubicRootsReal( a, b, c, d ); + const ts = Utils.solveCubicRootsReal( a, b, c, d )!; + + const xz = new Vector2( 0, 0 ); + ts.forEach( ( t: number ) => { if ( t >= 0 && t <= 1 ) { - const xz = cubic.positionAt( t ); + cubic.positionAt( t, xz ); interiorPoints.push( new Vector2( xz.x, y ) ); } } ); @@ -300,14 +303,17 @@ poolBounds.maxX, poolBounds.maxZ ), fluidY ); + const p0 = new Vector2( 0, 0 ); + const p1 = new Vector2( 0, 0 ); + // Fill the vertices around the boat's shape in the fluid cross-section for ( let i = 0; i < CROSS_SECTION_SAMPLES; i++ ) { const t0 = i / CROSS_SECTION_SAMPLES; const t1 = ( i + 1 ) / CROSS_SECTION_SAMPLES; // Generate positions for the current and next sample points along the boat's cross-section - const p0 = cubic.positionAt( t0 ); - const p1 = cubic.positionAt( t1 ); + cubic.positionAt( t0, p0 ); + cubic.positionAt( t1, p1 ); const p0x = ( p0.x - BoatDesign.DESIGN_CENTROID.x ) * scale + boatX; const p0z = p0.y * scale; @@ -368,12 +374,15 @@ const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( designY ), true ); const cubic = new Cubic( ...controlPoints ); + const p0 = new Vector2( 0, 0 ); + const p1 = new Vector2( 0, 0 ); + for ( let i = 0; i < CROSS_SECTION_SAMPLES; i++ ) { const t0 = i / CROSS_SECTION_SAMPLES; const t1 = ( i + 1 ) / CROSS_SECTION_SAMPLES; - const p0 = cubic.positionAt( t0 ); - const p1 = cubic.positionAt( t1 ); + cubic.positionAt( t0, p0 ); + cubic.positionAt( t1, p1 ); const p0x = ( p0.x - BoatDesign.DESIGN_CENTROID.x ) * scale; const p0z = p0.y * scale; @@ -405,9 +414,12 @@ const designY = ( -BoatDesign.DESIGN_BOAT_HEIGHT + ( isInside ? BoatDesign.DESIGN_WALL_THICKNESS : 0 ) ) * sample / ( heightSamples - 1 ); const controlPoints = BoatDesign.getControlPoints( BoatDesign.getHeightRatioFromDesignY( designY ), isInside ); const cubic = new Cubic( ...controlPoints ); + + const point = new Vector2( 0, 0 ); + return _.range( 0, parametricSamples ).map( pSample => { const t = pSample / ( parametricSamples - 1 ); - const point = cubic.positionAt( t ); + cubic.positionAt( t, point ); return BoatDesign.designToModel( new Vector3( point.x, designY, point.y ), liters ); } ); } ); ```

Using the chrome dev tools on mac m1, I compared the before and after, and was surprised to see how little it mattered:

without the fix: 8mb teeth 7.5 teeth/sec 0% time spent in Vector2 constructor

with the fix: 8mb teeth 7.5 teeth/sec 0% time spent in Vector2 constructor

image

There is a chance the allocation characteristics may be different on a different platform, but from this view it may be preferable not to commit this change, since it somewhat reduces readability at the call sites.

Either way, it seems I should check in with @zepumph, so let's do that first.

samreid commented 1 month ago

Noting that no changes were committed for this issue for dev test 1.2.0-dev.4. We should decide what we want to do for RC.1.

zepumph commented 1 month ago

We didn't feel like Buoyancy's usage of Vector2s in KITE were worth the time for the above changes. We saw that there were about 200000 Vector2s created in the any sim screen in about 20 seconds. We saw 10000 for the boat in about 10 seconds. If we really care about performance, let's focus on it more broadly, as there are certainly more valuable spots to work on this.