phetsims / kite

A library for creating, manipulating and displaying 2D shapes in JavaScript.
http://scenerystack.org/
MIT License
15 stars 6 forks source link

Reduce memory by using TinyEmitter instead of Events in Segment #79

Closed samreid closed 5 years ago

samreid commented 5 years ago

Discovered in https://github.com/phetsims/wave-interference/issues/399

This patch uses around 1% ~1MB less memory in Wave Interference, by using TinyEmitter instead of Events in Segment.

I'm not too familiar with Kite and I don't know if this patch is sufficient. Also, will this have slightly better performance because it is using TinyEmitter direct calls instead of Events which requires key lookups?

``` Index: js/segments/Arc.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Arc.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Arc.js (date 1560875262000) @@ -337,7 +337,7 @@ ( this.anticlockwise && this._startAngle - this._endAngle > Math.PI * 2 ) ), 'Not handling arcs with start/end angles that show differences in-between browser handling' ); - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/Quadratic.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Quadratic.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Quadratic.js (date 1560875262000) @@ -273,7 +273,7 @@ this._bounds = null; // {Bounds2|null} this._svgPathFragment = null; // {string|null} - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/util/Subpath.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/util/Subpath.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/util/Subpath.js (date 1560875262000) @@ -180,7 +180,7 @@ // Hook up an invalidation listener, so if this segment is invalidated, it will invalidate our subpath! // NOTE: if we add removal of segments, we'll need to remove these listeners, or we'll leak! - segment.onStatic( 'invalidated', this._invalidateListener ); + segment.invalidationEmitter.addListener( this._invalidateListener ); return this; // allow chaining }, @@ -565,8 +565,8 @@ else { subpaths = [ new Subpath( leftSegments.concat( lineStyles.cap( lastSegment.end, lastSegment.endTangent ) ) - .concat( rightSegments ) - .concat( lineStyles.cap( firstSegment.start, firstSegment.startTangent.negated() ) ), + .concat( rightSegments ) + .concat( lineStyles.cap( firstSegment.start, firstSegment.startTangent.negated() ) ), null, true ) ]; } Index: js/segments/Line.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Line.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Line.js (date 1560875262000) @@ -204,7 +204,7 @@ this._bounds = null; // {Bounds2|null} this._svgPathFragment = null; // {string|null} - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/EllipticalArc.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/EllipticalArc.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/EllipticalArc.js (date 1560875262000) @@ -439,7 +439,7 @@ ( this._anticlockwise && this._startAngle - this._endAngle > Math.PI * 2 ) ), 'Not handling elliptical arcs with start/end angles that show differences in-between browser handling' ); - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/Cubic.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Cubic.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Cubic.js (date 1560875262000) @@ -344,7 +344,7 @@ this._bounds = null; // {Bounds2|null} this._svgPathFragment = null; // {string|null} - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/Segment.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Segment.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Segment.js (date 1560875429000) @@ -14,9 +14,9 @@ var Bounds2 = require( 'DOT/Bounds2' ); var BoundsIntersection = require( 'KITE/ops/BoundsIntersection' ); - var Events = require( 'AXON/Events' ); var inherit = require( 'PHET_CORE/inherit' ); var kite = require( 'KITE/kite' ); + var TinyEmitter = require( 'AXON/TinyEmitter' ); var Util = require( 'DOT/Util' ); /** @@ -78,7 +78,7 @@ * transformed( matrix ) - returns a new segment that represents this segment after transformation by the matrix */ function Segment() { - Events.call( this ); + this.invalidationEmitter = new TinyEmitter(); } kite.register( 'Segment', Segment ); @@ -90,7 +90,7 @@ */ var identityFunction = function identityFunction( x ) { return x; }; - inherit( Events, Segment, { + inherit( Object, Segment, { /** * Will return true if the start/end tangents are purely vertical or horizontal. If all of the segments of a shape * have this property, then the only line joins will be a multiple of pi/2 (90 degrees), and so all of the types of @@ -254,6 +254,7 @@ var dashIndex = 0; var dashOffset = 0; var isInside = true; + function nextDashIndex() { dashIndex = ( dashIndex + 1 ) % lineDash.length; isInside = !isInside; @@ -274,7 +275,7 @@ var initiallyInside = isInside; // Recursively progress through until we have mostly-linear segments. - (function recur( t0, t1, p0, p1, depth ) { + ( function recur( t0, t1, p0, p1, depth ) { // Compute the t/position at the midpoint t value var tMid = ( t0 + t1 ) / 2; var pMid = self.positionAt( tMid ); @@ -307,7 +308,7 @@ recur( t0, tMid, p0, pMid, depth + 1 ); recur( tMid, t1, pMid, p1, depth + 1 ); } - })( 0, 1, this.start, this.end, 0 ); + } )( 0, 1, this.start, this.end, 0 ); return { values: values, ```

Also tagging for https://github.com/phetsims/wave-interference/issues/408 in case we want to address this before Wave Interference 2.0

samreid commented 5 years ago

The same pattern can be applied in Subpath. Not a significant memory savings there but it's nice to have a uniform approach:

``` Index: js/segments/Arc.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Arc.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Arc.js (date 1560875262000) @@ -337,7 +337,7 @@ ( this.anticlockwise && this._startAngle - this._endAngle > Math.PI * 2 ) ), 'Not handling arcs with start/end angles that show differences in-between browser handling' ); - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/Quadratic.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Quadratic.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Quadratic.js (date 1560875262000) @@ -273,7 +273,7 @@ this._bounds = null; // {Bounds2|null} this._svgPathFragment = null; // {string|null} - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/util/Subpath.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/util/Subpath.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/util/Subpath.js (date 1560876056000) @@ -14,12 +14,12 @@ var Arc = require( 'KITE/segments/Arc' ); var Bounds2 = require( 'DOT/Bounds2' ); - var Events = require( 'AXON/Events' ); var inherit = require( 'PHET_CORE/inherit' ); var kite = require( 'KITE/kite' ); var Line = require( 'KITE/segments/Line' ); var LineStyles = require( 'KITE/util/LineStyles' ); var Segment = require( 'KITE/segments/Segment' ); + var TinyEmitter = require( 'AXON/TinyEmitter' ); var Vector2 = require( 'DOT/Vector2' ); /** @@ -33,8 +33,7 @@ * @param {boolean} [closed] */ function Subpath( segments, points, closed ) { - Events.call( this ); - + this.invalidatedEmitter = new TinyEmitter(); var self = this; // @public {Array.} @@ -75,7 +74,7 @@ kite.register( 'Subpath', Subpath ); - inherit( Events, Subpath, { + inherit( Object, Subpath, { /** * Returns the bounds of this subpath. It is the bounding-box union of the bounds of each segment contained. @@ -146,7 +145,7 @@ if ( !this._invalidatingPoints ) { this._bounds = null; this._strokedSubpathsComputed = false; - this.trigger0( 'invalidated' ); + this.invalidatedEmitter.emit(); } }, @@ -180,7 +179,7 @@ // Hook up an invalidation listener, so if this segment is invalidated, it will invalidate our subpath! // NOTE: if we add removal of segments, we'll need to remove these listeners, or we'll leak! - segment.onStatic( 'invalidated', this._invalidateListener ); + segment.invalidationEmitter.addListener( this._invalidateListener ); return this; // allow chaining }, @@ -565,8 +564,8 @@ else { subpaths = [ new Subpath( leftSegments.concat( lineStyles.cap( lastSegment.end, lastSegment.endTangent ) ) - .concat( rightSegments ) - .concat( lineStyles.cap( firstSegment.start, firstSegment.startTangent.negated() ) ), + .concat( rightSegments ) + .concat( lineStyles.cap( firstSegment.start, firstSegment.startTangent.negated() ) ), null, true ) ]; } Index: js/segments/Line.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Line.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Line.js (date 1560875262000) @@ -204,7 +204,7 @@ this._bounds = null; // {Bounds2|null} this._svgPathFragment = null; // {string|null} - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/Shape.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/Shape.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/Shape.js (date 1560876434000) @@ -1789,7 +1789,7 @@ this.subpaths.push( subpath ); // listen to when the subpath is invalidated (will cause bounds recomputation here) - subpath.onStatic( 'invalidated', this._invalidateListener ); + subpath.invalidatedEmitter.addListener( this._invalidateListener ); this.invalidate(); Index: js/segments/EllipticalArc.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/EllipticalArc.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/EllipticalArc.js (date 1560875262000) @@ -439,7 +439,7 @@ ( this._anticlockwise && this._startAngle - this._endAngle > Math.PI * 2 ) ), 'Not handling elliptical arcs with start/end angles that show differences in-between browser handling' ); - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/Cubic.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Cubic.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Cubic.js (date 1560875262000) @@ -344,7 +344,7 @@ this._bounds = null; // {Bounds2|null} this._svgPathFragment = null; // {string|null} - this.trigger0( 'invalidated' ); + this.invalidationEmitter.emit(); }, /** Index: js/segments/Segment.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/segments/Segment.js (revision 274f6465213b0568aad39ea7732d31313733eb0e) +++ js/segments/Segment.js (date 1560875429000) @@ -14,9 +14,9 @@ var Bounds2 = require( 'DOT/Bounds2' ); var BoundsIntersection = require( 'KITE/ops/BoundsIntersection' ); - var Events = require( 'AXON/Events' ); var inherit = require( 'PHET_CORE/inherit' ); var kite = require( 'KITE/kite' ); + var TinyEmitter = require( 'AXON/TinyEmitter' ); var Util = require( 'DOT/Util' ); /** @@ -78,7 +78,7 @@ * transformed( matrix ) - returns a new segment that represents this segment after transformation by the matrix */ function Segment() { - Events.call( this ); + this.invalidationEmitter = new TinyEmitter(); } kite.register( 'Segment', Segment ); @@ -90,7 +90,7 @@ */ var identityFunction = function identityFunction( x ) { return x; }; - inherit( Events, Segment, { + inherit( Object, Segment, { /** * Will return true if the start/end tangents are purely vertical or horizontal. If all of the segments of a shape * have this property, then the only line joins will be a multiple of pi/2 (90 degrees), and so all of the types of @@ -254,6 +254,7 @@ var dashIndex = 0; var dashOffset = 0; var isInside = true; + function nextDashIndex() { dashIndex = ( dashIndex + 1 ) % lineDash.length; isInside = !isInside; @@ -274,7 +275,7 @@ var initiallyInside = isInside; // Recursively progress through until we have mostly-linear segments. - (function recur( t0, t1, p0, p1, depth ) { + ( function recur( t0, t1, p0, p1, depth ) { // Compute the t/position at the midpoint t value var tMid = ( t0 + t1 ) / 2; var pMid = self.positionAt( tMid ); @@ -307,7 +308,7 @@ recur( t0, tMid, p0, pMid, depth + 1 ); recur( tMid, t1, pMid, p1, depth + 1 ); } - })( 0, 1, this.start, this.end, 0 ); + } )( 0, 1, this.start, this.end, 0 ); return { values: values, ```

Note I've used slightly different names for the emitters. Not sure if that helps us keep them clear/searchable or if it's just more confusing.

jonathanolson commented 5 years ago

Using TinyEmitter in kite sounds great. Would you want to commit those changes to a branch? I wasn't able to figure out yet how to apply those patches.

samreid commented 5 years ago

I committed to a branch. For future reference, WebStorm or IDEA can apply patches via "VCS" --> "Apply Patch From Clipboard". I noticed Shape also employs Events for a similar purpose, but that isn't in the branch.

samreid commented 5 years ago

Converting Shape seems straightforward-- it seem there are only usages in Shape and Path.

ariel-phet commented 5 years ago

@samreid @jonathanolson while this is fresh in the mind and will help WI 2.0, lets make it happen. @samreid would be ideal to be done before publication.

samreid commented 5 years ago

This is one of the last issues marked for Wave Interference 2.0. Let me know if I can do anything to help.

jonathanolson commented 5 years ago

Looks good. Added the Shape/Path conversion, merged to master. @samreid can you verify those changes?

samreid commented 5 years ago

The changes look good. Do you want to keep these error-checking lines?

    this.on = function() { throw new Error( 'on' ); };
    this.onStatic = function() { throw new Error( 'onStatic' ); };

If I understand correctly, on and onStatic are now undefined in Shape, so these would throw errors anyways (as would off and off static). If we are keeping those error checking lines, we should have them check the entire API and apply that pattern in Segment. But I'm presuming they were temporary and should probably be deleted. @jonathanolson thoughts?

jonathanolson commented 5 years ago

But I'm presuming they were temporary and should probably be deleted. @jonathanolson thoughts?

Yup, fixed, thanks for the catch!

samreid commented 5 years ago

Looks great, thanks! Closing.