Hotfix: client-side memory issues #1255

JoshuaVulcan · 2025-03-13T01:32:42Z

What does this PR do?

Fixes a memory leak issue with unrevoked object URLs being used in the generation of cleaned SVGs for map icon presentation.
Also adds a bit of memoization for additional performance gains.

I had a call with some admins from Lewa and SWT this morning about performance issues, and noticed our map image object URLs being recalculated at a high frequency. While looking into memoizing those calculated values for a performance gain, I noticed the object URLs weren't being revoked...in this context, this causes a memory leak. And if called that frequently, it could really pile up. So I fixed that, then added the memoization, and some test coverage to match.

I'd like to hotfix this into the current release as a first test to see if it helps out the folks at Lewa.

…n the cache to mitigate memory leak instead. test cleanup.

luixlive

I see some logical errors and have a suggestion to instead of using utility methods with global variables as cache mechanism to go for a more React-y approach. Maybe a context provider handling the caching and retrieval of images. Also, I read this code and kind of get a sense of what is doing but it feels very technical and not verbose enough to fully tell me what is doing. Maybe we could use this PR for a bit of a refactoring on the variable names, conditions, adding comments, etc... To make it cleaner and more readable 👍

luixlive · 2025-03-14T15:30:49Z

src/TrackLegend/TimeOfDaySettings/TimeZoneSelect/index.test.js


-    expect(setTimeOfDayTimeZone).toHaveBeenCalledTimes(1);
-    // This test may break if someday the IANA standard updates.
-    expect(setTimeOfDayTimeZone).toHaveBeenCalledWith('America/Guadeloupe');


I think we can keep this test uncommented and having partial coverage if all we do is just remove this line. We would at least make sure that setTimeOfDayTimeZone is called.

luixlive · 2025-03-14T15:42:06Z

src/utils/img.js

@@ -11,42 +12,83 @@ const imgNeedsHostAppended = url => {
  return true;
 };

-export const imgElFromSrc = (src, width = 30, height = null) => new Promise((resolve, reject) => {
-  let img = new Image();
+const imageCache = new Map();


Having a global variable as a cache store has a smell to me. Maybe it's time to re-work the approach of these methods? Couldn't we have a hook with a state variable with the "cache" that passes down through context the imgElFromSrc method? Something like that sounds like the React approach, easier to memoize, debug, etc...

It's less efficient to do it that way, but we can certainly React-ify it more if you would like. I would prefer just using a factory function to create a nice closure to wrap the cache in.

Well, it shouldn't be less efficient if it's properly memoized. Maybe we are talking about a couple renders and a couple milliseconds, but React re-renders hundreds of times and the cache mechanism will make those milliseconds be negligible. I think the React approach is a better fit four our React application, but I also understand it's out of the scope of a hotfix, just wanted to share the idea.

How often and how much data can/should be held by this cache? Would a regular js map be good enough for a cache? Or should we use like a bounded in-memory cache to avoid another memory leak - the cache can potentially grow indefinitely here.

Also, we could reach for persistent solutions like LocalStorage/SessionStorage.

luixlive · 2025-03-14T15:42:14Z

src/utils/img.js

+  return `${src}:${w}:${h}`;
+};
+
+export const imgElFromSrc = (src, baseUnit = null) => {


Could we use more readable method and variable names? imgElFromSrc, w, and h names aren't very descriptive. And specifically with the method, just by reading the name (even if we use complete words) doesn't really tell what it does, maybe adding a verb to the name could help.

Totally! Maybe in a separate technical debt task? This is a hotfix branch and these are by and large existing function names

luixlive · 2025-03-14T15:43:22Z

src/utils/img.js

+  const cacheKey = generateImageCacheKey(src, baseUnit, null);
+
+  if (imageCache.has(cacheKey)) {
+    return imageCache.get(cacheKey);


Maybe I don't have a full understanding of what is going on, but it feels like this is a patch and not the solution to the issue. Why do we need to cache the images? Shouldn't we instead ask ourselves why is this method being called so many times being asked for the same resources?

The render cycle for the map clusters builds the image strings each time, which is normal since that's how the functional programming nature of React componentry works, and then has traditionally relied on browser caching mechanisms to not over-request images which have already been fetched.

Since we're creating object URLs for some images, this causes a memory leak; the object URLs are being created each time, and are held in memory indefinitely. Having our own cache here prevents createObjectURL from being called more than needed, and thus patches the leak.

luixlive · 2025-03-14T15:45:14Z

src/utils/img.js

+      if (baseUnit && img.naturalWidth && img.naturalHeight) {
+        const widthIsLarger = img.naturalWidth > img.naturalHeight;
+
+        if (widthIsLarger || !widthIsLarger) {


What is this? 🤯 This is logically equivalent to if (true) hahaha. This condition will always be true, it doesn't make sense to have it wrapped.

Just a WIP typo. This isn't part of the latest change set.

luixlive · 2025-03-14T15:47:44Z

src/utils/img.js

@@ -57,5 +99,7 @@ export const calcUrlForImage = imagePath => {
    return imagePath;
  }
  const appendString = !!DAS_HOST ? `${DAS_HOST}/` : '';
-  return `${appendString}${imagePath}`.replace(/^http:\/\//i, 'https://').replace('.org//', '.org/');
+  const final = `${appendString}${imagePath}`.replace(/^http:\/\//i, 'https://').replace('.org//', '.org/');


Personally, I don't think this variable is helping a lot. final doesn't tell me anything about the content of this string. If the intention was to make clearer what is being returned, we could be a bit more verbose.

luixlive · 2025-03-14T15:48:02Z

src/utils/img.test.js

@@ -0,0 +1,214 @@
+import { imgElFromSrc, calcImgIdFromUrlForMapImages, calcUrlForImage, registerActiveURL } from './img';


Thanks for the tests!!

JoshuaVulcan · 2025-03-15T03:34:28Z

I see some logical errors and have a suggestion to instead of using utility methods with global variables as cache mechanism to go for a more React-y approach. Maybe a context provider handling the caching and retrieval of images. Also, I read this code and kind of get a sense of what is doing but it feels very technical and not verbose enough to fully tell me what is doing. Maybe we could use this PR for a bit of a refactoring on the variable names, conditions, adding comments, etc... To make it cleaner and more readable 👍

I agree with 100% of this -- can we do it outside of the scope of this hotfix? I'd like to apply this change in isolation to a couple of prod sites to test if it resolves a memory leak I noticed recently

Chouffe

Overall looks like a nice and pragmatic fix to the memory leak that is described in the PR description.

The main question I have is whether a regular js Map is enough as an in-memory cache because it is unbounded. If we know that only a small number of Images can be generated overall, it should be fine, but if bazillions of images are potentially generated we probably want to use something more robust than a Map to avoid another memory-leak.

Chouffe · 2025-03-17T16:34:42Z

src/utils/img.js

+    return Promise.reject('no src provided');
+  }
+
+  const cacheKey = generateImageCacheKey(src, baseUnit, null);


It looks like baseUnit is not always the width of the image:

if (baseUnit && img.naturalWidth && img.naturalHeight) { const widthIsLarger = img.naturalWidth > img.naturalHeight; const aspectRatio = widthIsLarger ? img.naturalHeight / img.naturalWidth : img.naturalWidth / img.naturalHeight; if (widthIsLarger) { img.width = baseUnit; img.height = Math.round(baseUnit * aspectRatio); } else { img.height = baseUnit; img.width = Math.round(baseUnit * aspectRatio); } } else if (baseUnit) { img.width = baseUnit; img.height = baseUnit * aspectRatio; } else { img.height = baseUnit; img.width = baseUnit * aspectRatio; }

Does it make sense to update the generateImageCacheKey function to use baseUnit instead of width, height parameters?

Chouffe · 2025-03-17T16:35:09Z

src/utils/img.js

+
+    img.addEventListener('load', cleanupAndResolve, { once: true });
+
+    img.onerror = (e) => {


Nice cleanup here

Chouffe · 2025-03-17T16:40:41Z

src/utils/img.js

@@ -11,42 +12,83 @@ const imgNeedsHostAppended = url => {
  return true;
 };

-export const imgElFromSrc = (src, width = 30, height = null) => new Promise((resolve, reject) => {
-  let img = new Image();
+const imageCache = new Map();


How often and how much data can/should be held by this cache? Would a regular js map be good enough for a cache? Or should we use like a bounded in-memory cache to avoid another memory leak - the cache can potentially grow indefinitely here.

Also, we could reach for persistent solutions like LocalStorage/SessionStorage.

JoshuaVulcan · 2025-03-17T16:47:32Z

Overall looks like a nice and pragmatic fix to the memory leak that is described in the PR description.

The main question I have is whether a regular js Map is enough as an in-memory cache because it is unbounded. If we know that only a small number of Images can be generated overall, it should be fine, but if bazillions of images are potentially generated we probably want to use something more robust than a Map to avoid another memory-leak.

@Chouffe great question! The reason I used to a Map to begin with was actually how nice it was to work with when busting cache items beyond a certain limit (I think I'd originally set it to be like 100 or 200 in my first pass at the code). But in real-world terms, there's a maximum of a few hundred possible for an ER site; as such, I removed the bounding/lifecycle management stuff and just kept it simple.

JoshuaVulcan added 2 commits March 12, 2025 18:04

fix: revoking object URLs to avoid memory leaks on image-heavy sites

2ad9770

feat:memorization for map image creation fn, for further optimization

7790858

JoshuaVulcan requested review from AlanCalvillo, luixlive, gaboDev and chrisj-er March 13, 2025 01:32

JoshuaVulcan added the deploy label Mar 13, 2025

JoshuaVulcan temporarily deployed to ERA-11293 March 13, 2025 01:32 — with GitHub Actions Inactive

JoshuaVulcan temporarily deployed to padas-app March 13, 2025 01:33 — with GitHub Actions Inactive

JoshuaVulcan added 3 commits March 12, 2025 22:47

feat:more robust/consistent memoization. test updates to match.

0df6005

fix:undoing url revocation except in cases of load failure. relying o…

72345f4

…n the cache to mitigate memory leak instead. test cleanup.

chore:tweak and code optimization

2a24628

luixlive requested changes Mar 14, 2025

View reviewed changes

JoshuaVulcan added 2 commits March 14, 2025 09:05

tweak:missed file commit from yesterday.

76344b0

PR feedback

050490f

JoshuaVulcan requested review from luixlive, Chouffe, netomo and gabrielborges-nimble March 15, 2025 03:38

Chouffe reviewed Mar 17, 2025

View reviewed changes

merging 2.113 up into this branch

d5f01af

JoshuaVulcan deployed to ERA-11293 March 17, 2025 21:45 — with GitHub Actions View deployment

JoshuaVulcan temporarily deployed to padas-app March 17, 2025 21:46 — with GitHub Actions Inactive

JoshuaVulcan merged commit 5e5f3f6 into release-2.113.1 Mar 17, 2025
3 checks passed

JoshuaVulcan deleted the ERA-11293 branch March 17, 2025 21:46

		@@ -0,0 +1,214 @@
		import { imgElFromSrc, calcImgIdFromUrlForMapImages, calcUrlForImage, registerActiveURL } from './img';


		img.addEventListener('load', cleanupAndResolve, { once: true });

		img.onerror = (e) => {

Hotfix: client-side memory issues #1255

Hotfix: client-side memory issues #1255

Uh oh!

Conversation

JoshuaVulcan commented Mar 13, 2025

What does this PR do?

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chouffe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

JoshuaVulcan Mar 14, 2025 •

edited

Loading

JoshuaVulcan commented Mar 15, 2025 •

edited

Loading