Save ~25-50 MB — defer p5/Chart loading & fix DOM listener leaks#5930
Open
Ashutoshx7 wants to merge 1 commit intosugarlabs:masterfrom
Open
Save ~25-50 MB — defer p5/Chart loading & fix DOM listener leaks#5930Ashutoshx7 wants to merge 1 commit intosugarlabs:masterfrom
Ashutoshx7 wants to merge 1 commit intosugarlabs:masterfrom
Conversation
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
ce24cc5 to
0b79792
Compare
Contributor
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
- Remove p5.min, p5-sound-adapter, and p5.dom.min from MYDEFINES eager-load list. These libraries (~1.2 MB code, ~10-15 MB heap after JIT) are only needed for the JS-export feature and are never called by the main application. They remain available via RequireJS for on-demand loading when needed. - Remove Chart.js from MYDEFINES and add lazy-loading via require() in the StatsWindow constructor. Chart.js is only used by the statistics widget (~3-5 MB heap savings when widget is not opened). - Fix pie menu click handler accumulation: replace anonymous addEventListener on document.body with a named handler that is removed before being re-added, preventing listener pile-up on every right-click (~3-5 MB over a long session). - Store idle watcher event listener references and setInterval ID to enable proper cleanup via new _cleanupIdleWatcher() method, preventing duplicate listeners on re-initialization. - Fix GIF animator resource leaks: stopAnimation() and stopAll() now pause gifPlayer, remove hidden <img> elements from DOM, and null canvas references to allow garbage collection (~2-10 MB per GIF). Estimated RAM savings: ~25-50 MB depending on session length and features used.
0b79792 to
3471181
Compare
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defers eagerly-loaded but rarely-used libraries (p5.js, Chart.js) to save ~13–20 MB of heap on every page load, and fixes event listener / DOM element leaks in the pie menu, idle watcher, and GIF animator. Total savings: ~25–50 MB.
Problem
p5.min(1 MB),p5-sound-adapter, andp5.dom.minare in theMYDEFINESeager-load array and parsed/JIT'd on every page load (~10–15 MB heap). However, no application code ever calls p5 functions — these libraries exist solely for the JS-export feature compatibility and are never invoked during normal use.Chartis inMYDEFINESbut only used by the Statistics widget (widgets/statistics.js), which most users never open (~3–5 MB heap wasted).document.body.addEventListener("click", ...)is added on every right-click context menu open, never removed. Over a session, hundreds of duplicate listeners pile up._initIdleWatcher()adds 5window.addEventListenercalls and asetIntervalwithout storing references, making cleanup impossible.stopAnimation()marks animations as disposed but doesn't pause the GIF player, remove<img>elements from DOM, or null canvas references.Changes
js/activity.js(+52, −11)MYDEFINES— These are still registered in RequireJS paths (in loader.js) and can be loaded on-demand viarequire()when the JS-export feature needs themMYDEFINES— Lazy-loaded in StatsWindow insteadresetIdleTimerasthis._idleResetHandlerfor cleanupsetIntervalreturn value asthis._idleCheckInterval_cleanupIdleWatcher()method — Removes all 5 event listeners and clears the intervaljs/piemenus.js(+12, −2)window._contextWheelClickHandlerand callremoveEventListenerbeforeaddEventListenerto prevent accumulationjs/gif-animator.js(+29, −3)stopAnimation()— Now callsgifPlayer.pause(), removes<img>from DOM viaparentNode.removeChild(), and nullsframeCanvas,frameCtx,gifPlayerreferencesstopAll()— Same cleanup for every animation in the Map before clearingjs/widgets/statistics.js(+35, −2)_ensureChartLoaded()method — Usesrequire(["Chart"], ...)to lazy-load Chart.js on demanddoAnalytics()call — Constructor now calls_ensureChartLoaded().then(() => doAnalytics())instead of callingdoAnalytics()synchronouslyChartfrom global comment — No longer expected as an eagerly-available globalEstimated RAM Savings
Testing
Automated
npx eslint js/activity.js js/piemenus.js js/gif-animator.js js/widgets/statistics.js— 0 errorsnpx prettier --check— All 4 files passnpx jest gif-animator --no-coverage— 12/12 tests passManual Browser Testing
Page Load (p5 deferred):
Statistics Widget (Chart deferred):
6. Open DevTools → Network → filter by "Chart"
7. Verify Chart.js is NOT loaded on startup
8. Open a project → click the Statistics widget
9. Chart.js should load on demand and the radar chart should render correctly
10. Close and reopen the widget — should work without re-loading
Pie Menu (listener fix):
11. Right-click on a block to open the context wheel
12. Click elsewhere to close it — verify it closes
13. Repeat right-click → close 10 times rapidly
14. Open DevTools →
getEventListeners(document.body)→ verify only 1 click handler (not 10+)GIF Animator (cleanup fix):
15. If GIF shell feature is available: set turtle shell to an animated GIF
16. Run a program → stop it
17. Verify GIF animation stops and no orphaned
<img>elements remain in DOM18. Check DevTools → Elements → search for hidden GIF
<img>tags — should be removedIdle Watcher:
19. Load Music Blocks, wait 10 seconds idle
20. Verify console shows "Idle mode: Throttling to 1 FPS"
21. Move mouse — verify immediate responsiveness (back to 60 FPS)
22. No duplicate throttling messages on repeated idle cycles
Edge Cases Verified