Throttle events in polygon #1444

nicolecomputer · 2024-07-24T21:48:23Z

Summary:

Throttles the events in polygon to increase performance and decrease CPU load. This is especially important for Safari. This change lowers CPU usage from 87% to 30% on a M3 macbook. The increase in performance will be more on phones and iPads where this will also decrease energy usage.

40 FPS was chosen as a mark that feels response and good to use but doesn't put too much stress on the machine.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2185

Test plan:

To test before:

Load the dev environment on Safari
Open the Timelines pane and start recording
The CPU usage should be high (it'll vary based on your machine)

To test after:

Repeat test above with this change
The CPU usage should be lower

khan-actions-bot · 2024-07-24T21:48:41Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/pretty-planets-sleep.md, packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-07-24T21:50:46Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (e41b7e0) and published it to npm. You
can install it using the tag PR1444.

Example:

yarn add @khanacademy/perseus@PR1444

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1444

github-actions · 2024-07-24T21:52:06Z

Size Change: +860 B (+0.1%)

Total Size: 855 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	273 kB	+569 B (+0.21%)
`packages/perseus/dist/es/index.js`	415 kB	+291 B (+0.07%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.2 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.6 kB
`packages/math-input/dist/es/strings.js`	1.73 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-linter/dist/es/index.js`	21.6 kB
`packages/perseus/dist/es/strings.js`	3.21 kB
`packages/pure-markdown/dist/es/index.js`	3.67 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-07-24T21:56:10Z

Codecov Report

Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.26%. Comparing base (c6a5cbe) to head (e41b7e0).
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
+ Coverage   69.86%   70.26%   +0.40%     
==========================================
  Files         505      508       +3     
  Lines      105665   105672       +7     
  Branches     7670    10801    +3131     
==========================================
+ Hits        73826    74254     +428     
+ Misses      31659    31418     -241     
+ Partials      180        0     -180

Files	Coverage Δ
.../src/widgets/interactive-graphs/graphs/polygon.tsx	`89.65% <54.54%> (-8.09%)`	⬇️

... and 166 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bf32b7...e41b7e0. Read the comment docs.

benchristel · 2024-07-24T23:07:41Z

packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

@@ -42,6 +42,7 @@ export const PolygonGraph = (props: Props) => {
    });

    const active = hovered || focused || dragging;
+    const [lastMoveTime, setLastMoveTime] = React.useState<number>(0);


Thank you for doing this optimization work!

Could we use a ref instead of state here? That would prevent unnecessary re-renders. I believe a ref is appropriate because we will never need to update DOM elements when lastMoveTime changes.

Yepppppp. You're absolutely right. Fixed up

benchristel · 2024-07-24T23:11:59Z

packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

-                    }
+                    onMove={(destination: vec.Vector2) => {
+                        const now = Date.now();
+                        const targetFPS = 40;


What's the rationale for capping FPS at 40? For displays that refresh at 60Hz (or browsers that repaint at 60Hz?), this will result in every third frame getting dropped. I guess my question is, why not 30 fps?

Loosely held opinion: 40 fps feels about right. I've noticed in games it's where things start to feel smooth (which is totally subjective). Some data, though: If I cap at 40fps I get around 26% CPU usage. If I cap at 60 I get around 40%.

benchristel

🎉 LGTM!

@benchristel

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Major Changes - [#1456](#1456) [`b868801fa`](b868801) Thanks [@benchristel](https://github.com/benchristel)! - Remove unused "Show ruler" option from the interactive graph editor. The new Mafs version of the interactive graph does not implement the ruler, and we have no plans to implement it, since it can't be made accessible and isn't used in Khan Academy's existing content. - [#1450](#1450) [`2216ad012`](2216ad0) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove Unit aka UnitInput widget ### Minor Changes - [#1451](#1451) [`9bc4812fc`](9bc4812) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Upgrade Mafs Dependency to v0.19.0 for Domain Restricted Functions - [#1441](#1441) [`9bc264ce1`](9bc264c) Thanks [@Myranae](https://github.com/Myranae)! - Turn off interactivity when Interactive Graph in hint mode - [#1437](#1437) [`7a448e77c`](7a448e7) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon to be filled on hover - [#1422](#1422) [`c386515ad`](c386515) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Segment and Linear System graph start coords UI ### Patch Changes - [#1448](#1448) [`84675574c`](8467557) Thanks [@nishasy](https://github.com/nishasy)! - Cleaned up the `startCoords` code in the stateful mafs graph useEffect - [#1444](#1444) [`130ab9446`](130ab94) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Throttles point-moving performance in polygon - [#1445](#1445) [`bb1ac584b`](bb1ac58) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - useDraggable - fix to ignore keyup events (we don't want to treat keyup events as an intent to move - we have keydown for that) - Updated dependencies \[[`7e71f8e8a`](7e71f8e)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Major Changes - [#1456](#1456) [`b868801fa`](b868801) Thanks [@benchristel](https://github.com/benchristel)! - Remove unused "Show ruler" option from the interactive graph editor. The new Mafs version of the interactive graph does not implement the ruler, and we have no plans to implement it, since it can't be made accessible and isn't used in Khan Academy's existing content. - [#1450](#1450) [`2216ad012`](2216ad0) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove Unit aka UnitInput widget ### Minor Changes - [#1422](#1422) [`c386515ad`](c386515) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Segment and Linear System graph start coords UI ### Patch Changes - [#1446](#1446) [`4985d2d4c`](4985d2d) Thanks [@nishasy](https://github.com/nishasy)! - Rename StartCoordSettings to StartCoordsSettings - [#1448](#1448) [`84675574c`](8467557) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Refactor and clean up start coords UI implementation - Updated dependencies \[[`b868801fa`](b868801), [`84675574c`](8467557), [`7e71f8e8a`](7e71f8e), [`9bc4812fc`](9bc4812), [`130ab9446`](130ab94), [`9bc264ce1`](9bc264c), [`bb1ac584b`](bb1ac58), [`2216ad012`](2216ad0), [`7a448e77c`](7a448e7), [`c386515ad`](c386515)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1454](#1454) [`7e71f8e8a`](7e71f8e) Thanks [@Myranae](https://github.com/Myranae)! - Update mathjax-renderer version ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`7e71f8e8a`](7e71f8e)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1443

Nicole Watts added 2 commits July 23, 2024 15:57

wip: hook

1288cdc

wip: throttle events for polygon

aee13f7

nicolecomputer self-assigned this Jul 24, 2024

khan-actions-bot requested a review from a team July 24, 2024 21:48

Delete packages/perseus/src/util/use-throttle.ts

d1f60a0

nicolecomputer requested review from jeremywiebe and mark-fitzgerald July 24, 2024 21:48

wip: changeset

6dd364a

Merge branch 'new/throttle' of github.com:Khan/perseus into new/throttle

1ae01cc

benchristel reviewed Jul 24, 2024

View reviewed changes

wip: use ref instead of state

e41b7e0

khan-actions-bot requested a review from a team July 25, 2024 16:51

nicolecomputer requested a review from benchristel July 25, 2024 16:54

benchristel approved these changes Jul 25, 2024

View reviewed changes

nicolecomputer merged commit 130ab94 into main Jul 25, 2024
10 of 11 checks passed

nicolecomputer deleted the new/throttle branch July 25, 2024 17:41

khan-actions-bot mentioned this pull request Jul 25, 2024

Version Packages #1443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throttle events in polygon #1444

Throttle events in polygon #1444

nicolecomputer commented Jul 24, 2024

khan-actions-bot commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 24, 2024 •

edited

Loading

benchristel Jul 24, 2024 •

edited

Loading

nicolecomputer Jul 25, 2024 •

edited

Loading

benchristel Jul 24, 2024

nicolecomputer Jul 25, 2024

benchristel left a comment

Throttle events in polygon #1444

Throttle events in polygon #1444

Conversation

nicolecomputer commented Jul 24, 2024

Summary:

Test plan:

khan-actions-bot commented Jul 24, 2024 • edited Loading

Gerald

github-actions bot commented Jul 24, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Jul 24, 2024 • edited Loading

codecov bot commented Jul 24, 2024 • edited Loading

Codecov Report

benchristel Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

nicolecomputer Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

benchristel Jul 24, 2024

Choose a reason for hiding this comment

nicolecomputer Jul 25, 2024

Choose a reason for hiding this comment

benchristel left a comment

Choose a reason for hiding this comment

khan-actions-bot commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 24, 2024 •

edited

Loading

benchristel Jul 24, 2024 •

edited

Loading

nicolecomputer Jul 25, 2024 •

edited

Loading