Using ImageHandleManager for managing multiple Image handles in Image class #2905

amartya4256 · 2025-12-22T12:23:13Z

This PR introduces ImageHandleManager, an inner class to Image responsible for managing the storage, creation and manipulation of ImageHandles. This class provides methods which can be used to dead with ImageHandle instances inside an image for managing it with convenience while hiding the complexity of how the ImageHandle lifecycle is managed inside itself.

All the places where zoomToImageHandle map is accessed for any purposes is replaced by the wrapper methods provided by ImageHandleManager. Each image initializes a final field imageHandleManager with the instance of ImageHandleManager. Every instance of ImageHandle created within the Image object is registered in the manager itself and the manager is responsible for cleaning the dangling resources when are not supposed to be used anymore or when a cleanup is needed. Also the manager creates handles in a thread-safe way, making sure no 2 consumers create the same handle at the same time.

This fixes vi-eclipse/Eclipse-Platform#562

HeikoKlare

I really like the idea to wrap the handle-for-zoom mapping into a separate class. That clearly assigns all related management functionality to that class. I would be in favor of extracting the pure refactoring of the management functionality in a dedicated class into a separate PR. That PR will just improve code quality and comprehensibility and not improve thread safety.

You find several questions and proposals in the detailed comments. The probably most essential point is that to my understanding the implementation is not fully thread safe.

In addition, I am not sure if the concept of using a synchronized data structure is fully sufficient for all cases here (e.g., if we also include the rest of the API such as getBounds() and getImageData()). At least the complexity could increase such that ensuring correctness and maintainability becomes complex. Maybe we better see that when we adapt those methods as well, but I could imagine that simply synchronizing write access might be sufficient (as we don't expect multiple consumers to write for different zooms) and provides reduced compexity and error proneness.

HeikoKlare · 2025-12-30T08:29:17Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

 			return function.apply(temporaryHandle);
 		} finally {
-			temporaryHandle.destroy();
+			imageHandleManager.destroy(temporaryHandle);


Why do you need to destroy the handle via the handle manager even though it was not created via the manager (so it will not be present in its handle map anyway)?

It will. All the ImageHandles are registered in the manager. It is in the constructor of ImageHandle. Maybe we should only allow creation of ImageHandle using the manager then. This will simplify it. We want to register all the ImageHandles as we actively perform cleanup of dangling ImageHandles.

The dangling ImageHandles can be those which were created temporarily for obtaining the ImageData or for some other purpose. We want to track all the ImageHandles in the Manager.

HeikoKlare · 2025-12-30T08:29:59Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

 			ImageHandle handle = initializeHandleFromSource(new ZoomContext(zoomToRetrieve));
 			ImageData data = handle.getImageData();
-			handle.destroy();
+			imageHandleManager.destroy(handle);


Same as above: why do you need to destroy the handle via the handle manager even though it was not created via the manager (so it will not be present in its handle map anyway)?

This applies to all newImageData methods.

HeikoKlare · 2025-12-30T08:36:32Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+				return imageHandle;
 			}
-			return zoomLevelToImageHandle.get(targetZoom);
+			return imageHandleManager.getImageHandle(targetZoom);


This code looks a bit weird (but also did before) because we call a _new_ImageHandle method and then return an existing one. From my understanding, this line of code should never be reached anyway. In case there is a memGC, there should also be an image handle for it, so that newImageHandle will never be called unless the zoom of that handle is not fitting (and then the if block above is executed).

HeikoKlare · 2025-12-30T08:38:29Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+	ImageHandle imageHandle = imageHandleManager.getImageHandle(zoom);
+	if (imageHandle != null) {
+		return imageHandle.getImageData();


This code is still prone to race conditions: there might not be a handle yet, then another thread comes and creates that handle, but since we have not seen that handle here, we call imageProvider.newImage, which will expensively create another handle, extract image data out of it and then cache that image data (although the cached image data will never be used as there is a handle already, so that part of memory is effectively dead). Is that intended?

HeikoKlare · 2025-12-30T08:40:57Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

 	long newHandle = drawable.internal_new_GC(newData);
 	if (newHandle == 0) SWT.error(SWT.ERROR_NO_HANDLES);
-	init(drawable, newData, newHandle);
+	initWithImageHandle(drawable, newData, newHandle, imageHandle);


Why is this change to GC necessary? Is it a bug in the existing implementation?

Yes this method is created from the earlier implementation of the PlainImageDataProvider as in case it uses GC to scale, it uses this specific method. In the init method of GC, it uses the Image.win32_getHandle, which would want to create a handle and since we already are in the middle of creation of the handle it will fail. Since we also had a specific method in the past for this case, the idea is to use the ImageHandle we just obtained and pass that directly instead of calling win32_getHandle.

HeikoKlare · 2025-12-30T08:45:59Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+			final ImageHandle imageHandleFuture = zoomLevelToImageHandle.get(zoom);
+			if (imageHandleFuture == null) {
+				return null;
+			}
+			return imageHandleFuture;


There is no future involved here and the value of zoomLevelToHandle.get(zoom) could directly be returned, couldn't it?

My bad, forgot to rename it. Went with approach of Futures earlier.

HeikoKlare · 2025-12-30T08:47:23Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+		private Map<Integer, ImageHandle> zoomLevelToImageHandle = new ConcurrentHashMap<>();
+		private Set<ImageHandle> imageHandles = new HashSet<>();


Why do we need two separate data structure managing image handles? They are managed in different ways and will mostly (but not competely) contains the same handles.

The Set contains all the distinct imageHandles created for the image temporary and permanent ones. Any dangling imageHandle can be tracked here and disposed upon cleanups.

HeikoKlare · 2025-12-30T08:49:30Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+			return zoomLevelToImageHandle.containsKey(zoom);
+		}
+
+		boolean hasNoBaseHandle() {


Should this be hasNoHandle()? It's unclear what a "base" handle is supposed to be.

Fair point.

HeikoKlare · 2025-12-30T08:53:12Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+		void destroy(ImageHandle imageHandle) {
+			zoomLevelToImageHandle.remove(imageHandle.zoom, imageHandle);
+			imageHandle.destroy();
+			imageHandles.remove(imageHandle);
+			cleanupDanglingImageHandles();
+		}


This method feels wrong to me: either the ImageHandleManager manages the handles and does not auto destroy (with the other method accepting a Predicate) or a handle is managed separately, but then it should not be artificially be passed over this manager.

If I am not mistaken, at least the imageHandles set, the register method and the destroy(ImageHandle) should be removed.

This commit adds a class ImageHandleManager in Image to manage multiple ImageHandle for different zoom hence encapsulating the usages and operations based on zoomLevelToImageHandle.

This commit makes the ImageHandle creation and ImageData caching process synchronous to avoid any racing condition and inefficient memory usages.

github-actions · 2025-12-30T16:11:13Z

Test Results (win32)

27 files - 7 27 suites - 7 4m 12s ⏱️ -5s
4 571 tests - 57 4 500 ✅ - 55 70 💤 - 3 1 ❌ +1
110 runs - 57 110 ✅ - 54 0 💤 - 3 0 ❌ ±0

For more details on these failures, see this check.

Results for commit 0e6ad0d. ± Comparison against base commit 383ed2a.

This pull request removes 57 tests.

AllWin32Tests ImageWin32Tests ‑ testDisposeDrawnImageBeforeRequestingTargetForOtherZoom
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[2] false
AllWin32Tests ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[2] false
AllWin32Tests ImageWin32Tests ‑ test_getImageData_fromCopiedImage
AllWin32Tests ImageWin32Tests ‑ test_getImageData_fromImageForImageDataFromImage
AllWin32Tests TestTreeColumn ‑ test_ColumnOrder
…

amartya4256 marked this pull request as draft December 22, 2025 12:24

HeikoKlare reviewed Dec 30, 2025

View reviewed changes

amartya4256 force-pushed the amartya4256/image_concurrency_fix branch from 93108dc to e9cb651 Compare December 30, 2025 15:32

amartya4256 and others added 2 commits December 30, 2025 16:44

Added ImageHandleManager to manage ImageHandle in Image

754103f

This commit adds a class ImageHandleManager in Image to manage multiple ImageHandle for different zoom hence encapsulating the usages and operations based on zoomLevelToImageHandle.

Make ImageHandle & ImageData write ops synchronous

0e6ad0d

This commit makes the ImageHandle creation and ImageData caching process synchronous to avoid any racing condition and inefficient memory usages.

amartya4256 force-pushed the amartya4256/image_concurrency_fix branch from e9cb651 to 0e6ad0d Compare December 30, 2025 15:56

		private Map<Integer, ImageHandle> zoomLevelToImageHandle = new ConcurrentHashMap<>();
		private Set<ImageHandle> imageHandles = new HashSet<>();

Using ImageHandleManager for managing multiple Image handles in Image class #2905

Are you sure you want to change the base?

Using ImageHandleManager for managing multiple Image handles in Image class #2905

Conversation

amartya4256 commented Dec 22, 2025

Uh oh!

HeikoKlare 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!

amartya4256 Dec 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 30, 2025

Test Results (win32)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amartya4256 Dec 30, 2025 •

edited

Loading