Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we improve performance further? #243

Closed
pixelzoom opened this issue Mar 1, 2023 · 9 comments
Closed

Can we improve performance further? #243

pixelzoom opened this issue Mar 1, 2023 · 9 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 1, 2023

phetsims/qa#898 was a performance test by QA.

In phetsims/qa#898 (comment), @liammulh reported:

These notes apply to:

Tester = LM
Device =
   (Seemingly has no name. Also has no cart sticker. Black with black case. Lion logo on the case.)
   iPad Model MR7F2LL/A
   iPadOS 16.3
   Safari 16.3 Mobile
Time = 1.5 hour

Specific Things

In the Derivative screen, compare responsiveness with 'Tangent' checkbox checked and unchecked.

Not noticeably worse with tangent checkbox checked.

In the Integral screen, compare responsiveness with 'Area Under Curve' checkbox checked and unchecked.

Not noticeably worse with area under curve checkbox checked.
...
The performance on the derivative, integral, and advanced screens was fine. The lab screen's performance is slightly worse.
...
Lab Screen Initially, with no configuration, the performance is noticeably worse than the other screens. However, if you click the button that allows you to create a piecewise function, the performance degrades to the point where the curve being drawn is well behind where your finger is on the screen.

In phetsims/qa#898 (comment), @stemilymill reported:

Tester = E2 
Device = Lenovo 100e Chromebook gen 2 MTK
ChromeOS Version 110.0.5481.112 (32-bit)
Time = 1.5 hr{max 2 hours} 

Testing with the chromebook I had the same experience as Liam, ...

On the Lab screen, the performance is significantly worse (mostly with piecewise function) but I would still consider it usable. This chromebook is not touchscreen, and the lag might just be less obtrusive with trackpad input than touch input.

The statement "I would still consider it usable" for the worst case (Lab screen) is encouraging. But we should investigate whether there's anywhere else that we might improve performance. Assigning to @veillette and myself for discussion.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 1, 2023

Also note that we need to do another round of QA performance testing with labeledLines and labeledPoints turned on, because those PhET-iO-only features will impact performance. From phetsims/qa#898 (comment):

In 2/27/2023 Calculus Grapher design meeting, we decided that we need further performance testing with some PhET-iO-only features enabled (labeledLines, labeldPoints). I polled @stemilymill and @liammulh, and we decided to continue with this issue for futher testing, since that testing should involve the same devices. I’ll assign this issue back to @stemilymill and @liammulh, with specific instructions, when it’s ready for further testing.

@phetsims phetsims deleted a comment from amanda-phet Mar 1, 2023
@veillette veillette changed the title Can we improve performance futher? Can we improve performance further? Mar 1, 2023
@pixelzoom
Copy link
Contributor Author

In AncillaryTool, each of the y Properties (yOriginalProperty, et.al.) is actually 3 DerivedProperties. See function createProperties. This is because we want to present discontinuties as null in Studio. If we dropped that requirement, we would only need 1 DerivedProperty, and would likely see some performance improvement for the tools.

@pixelzoom
Copy link
Contributor Author

In #233 (comment), I rewrote AncillaryTool and noted:

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 6, 2023

#259 reminded me that the canvas version of bamboo classes have better performance. We should consider switching to them. That's what I used in fourier-making-waves.

@pixelzoom
Copy link
Contributor Author

@veillette and I discussed during weekly pairing. I'm going to experiment with using the Canvas versions of bamboo classes. We probably only need to do this for CanvasLinePlot. There currently is no Canvas implementation for ScatterPlot, but it's not much performance burden.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 7, 2023

... I'm going to experiment with using the Canvas versions of bamboo classes.

The first problem I've encountered is that the APIs for LinePlot and CanvasLinePlot are not the same. And CanvasLinePlot does not support lineDash, which we use for discontinuousLinePlot in CurveNode. I supposed we could continue to use LinePlot for that case, but I'd prefer to use the same implementation for all line plots.

EDIT: I added CanvasLinePlotOptions.lineDash in phetsims/bamboo@ebd2a21.

pixelzoom added a commit to phetsims/bamboo that referenced this issue Mar 8, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 11, 2023

I spent ~30 minutes investigating what it would take to convert uses of LinePlot to CanvasLinePlot.

Summary:

  • Most of the changes would be in CurveNode.ts.
  • We'd need to add a ChartCanvasNode, and add both continuousLinePlot and discontinuousLinePlot to it.
  • Where we're using LinePlotOptions, we'd need to switch to CanvasLinePlotOptions, which are not the same. The stroke option is particularly problematic because CanvasLinePlot only supports Color | string | null, and not TColor - so no color Properties, which we're using all over the place. So we'd have to handle color updates ourselves, by listening to the color Property and calling CanvasLinePlot setStroke(...) and update().

So it's at least a couple of hours to convert to Canvas. I'm inclined to do the conversion, since Canvas had such better performance in Fourier. But let's defer the decision until we get performance results from phetsims/qa#898.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 11, 2023

A lot of redundant calls to curveChangeEmitter.emit were identified and addressed in #278. The sim feels much snappier to me. So now I'm less inclined to convert to Canvas.

@pixelzoom
Copy link
Contributor Author

Closing, and ready for sign-off in #266.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants