Skip to content

[Win32] Avoid unnecessary creation of GC operations #3318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HeikoKlare wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[Win32] Avoid unnecessary creation of GC operations #3318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d47b225
[Win32] Copy patterns in GC only when necessary
HeikoKlare May 21, 2026
9b5c11a
[Win32] Avoid unnecessary creation of GC operations
HeikoKlare May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4901,16 +4901,31 @@ public void setBackgroundPattern (Pattern pattern) {
storeAndApplyOperationForExistingHandle(new SetBackgroundPatternOperation(pattern));
}

private class SetBackgroundPatternOperation extends Operation {
private final Pattern pattern;
private abstract class PatternOperation extends Operation {
private Pattern pattern;

PatternOperation(Pattern pattern) {
this.pattern = pattern;
}

protected Pattern getPattern() {
if (pattern != null && pattern.isDisposed()) {
// recreate locally managed pattern if the original was disposed
pattern = pattern.copy();
registerForDisposal(pattern);
}
return pattern;
}
}

private class SetBackgroundPatternOperation extends PatternOperation {
SetBackgroundPatternOperation(Pattern pattern) {
this.pattern = pattern == null ? null : pattern.copy();
registerForDisposal(this.pattern);
super(pattern);
}

@Override
void apply() {
Pattern pattern = getPattern();
if (data.gdipGraphics == 0 && pattern == null) return;
initGdip();
if (data.backgroundPattern == pattern) return;
Expand Down Expand Up @@ -5249,16 +5264,15 @@ public void setForegroundPattern (Pattern pattern) {
storeAndApplyOperationForExistingHandle(new SetForegroundPatternOperation(pattern));
}

private class SetForegroundPatternOperation extends Operation {
private final Pattern pattern;
private class SetForegroundPatternOperation extends PatternOperation {

SetForegroundPatternOperation(Pattern pattern) {
this.pattern = pattern == null ? null : pattern.copy();
registerForDisposal(this.pattern);
super(pattern);
}

@Override
void apply() {
Pattern pattern = getPattern();
if (data.gdipGraphics == 0 && pattern == null) return;
initGdip();
if (data.foregroundPattern == pattern) return;
Expand Down Expand Up @@ -5990,8 +6004,11 @@ Point textExtentInPixels(String string, int flags) {
return new Point(rect.right, rect.bottom);
}

void refreshFor(Drawable drawable, ImageHandle imageHandle) {
void reapplyTo(Drawable drawable, ImageHandle imageHandle) {
if (drawable == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
if (!data.reapplicable) {
SWT.error(SWT.ERROR_INVALID_ARGUMENT);
}
destroy();
GCData newData = new GCData();
originalData.copyTo(newData);
Expand Down Expand Up @@ -6097,9 +6114,13 @@ int getZoom() {
}

private void storeAndApplyOperationForExistingHandle(Operation operation) {
removePreviousOperationIfSupercededBy(operation);
operations.add(operation);
operation.apply();
if (data.reapplicable) {
removePreviousOperationIfSupercededBy(operation);
operations.add(operation);
} else {
operation.disposeAll();
}
}

private void removePreviousOperationIfSupercededBy(Operation operation) {
Expand Down
1 change: 1 addition & 0 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public final class GCData {
public int alpha = 0xFF;
public int nativeZoom;
int imageZoom;
boolean reapplicable;

public Image image;
public PAINTSTRUCT ps;
Expand Down
3 changes: 2 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2333,6 +2333,7 @@ public Collection<Integer> getPreservedZoomLevels() {

@Override
protected long configureGCData(GCData data) {
data.reapplicable = true;
return configureGC(data, new ZoomContext(DPIUtil.getDeviceZoom(), DPIUtil.getNativeDeviceZoom()));
}

Expand Down Expand Up @@ -2374,7 +2375,7 @@ protected DestroyableImageHandle newImageHandle(ZoomContext zoomContext) {
GC currentGC = memGC;
memGC = null;
DestroyableImageHandle imageHandle = createHandle(targetZoom);
currentGC.refreshFor(new DrawableWrapper(Image.this, zoomContext), imageHandle);
currentGC.reapplyTo(new DrawableWrapper(Image.this, zoomContext), imageHandle);
return imageHandle;
}

Expand Down
2 changes: 1 addition & 1 deletion ...e.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_GC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,8 +487,8 @@ public void test_drawImageLorg_eclipse_swt_graphics_ImageIIII_ImageDataAtSizePro
Exception e = assertThrows(IllegalArgumentException.class, () -> gc.drawImage(image, 0, 0, 1, 1));
assertSWTProblem("Incorrect exception thrown for provider == null", SWT.ERROR_INVALID_ARGUMENT, e);
} finally {
image.dispose();
drawToImage.dispose();
image.dispose();
Copy link
Copy Markdown
Member

@fedejeanne fedejeanne May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change necessary? Does this mean that production code also needs to pay attention to the order in which the images have been created when disposing them?

Copy link
Copy Markdown
Contributor Author

@HeikoKlare HeikoKlare May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a test for an exception thrown during a drawImage call when a null provider is used. The resulting state of the GC after such a failure is undefined and potentially having it unnecessarily try to copy the image that was attempted to be drawn because of the disposal order is just a risk of the test failing because of an unrelated exception. In practice it does not make any difference as the code fails because of the failing draw operation anyway and probably no one wraps a drawImage call into a try-finally to clean up the involved resources where this order could make any difference at all.

}
}

Expand Down
Loading