Implemented Double-Checked Locking in ImagehandleManager #2905
Conversation
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
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.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
| return imageHandle; | ||
| } | ||
| return zoomLevelToImageHandle.get(targetZoom); | ||
| return imageHandleManager.getImageHandle(targetZoom); |
There was a problem hiding this comment.
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).
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Show resolved
Hide resolved
| 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); |
There was a problem hiding this comment.
Why is this change to GC necessary? Is it a bug in the existing implementation?
There was a problem hiding this comment.
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.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
amartya4256
force-pushed
the
amartya4256/image_concurrency_fix
branch
2 times, most recently
from
e9cb651 to
0e6ad0d
Compare
amartya4256
force-pushed
the
amartya4256/image_concurrency_fix
branch
from
0e6ad0d to
1494ed0
Compare
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.
amartya4256
force-pushed
the
amartya4256/image_concurrency_fix
branch
from
1494ed0 to
22f6ab7
Compare
amartya4256
changed the title
2 participants
This PR handles the creation of handles / cached ImageData per zoom for Image objects in a thread-safe way by the means of double-checked locking. The implementation extends the ImageHandleManager in the following way:
runSynchronizedfor to run a block of code synchronously. This is utilized while calling getImageData to ensure thread safety.Depends on #2927
This fixes vi-eclipse/Eclipse-Platform#562