Skip to content

[Win32] Copy patterns in GC only when necessary #3317

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 1 commit into
base: master
Choose a base branch
from
+22 −8
Open

[Win32] Copy patterns in GC only when necessary #3317

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 22 additions & 8 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);
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.

Is this enough to make sure that the newly "resuscitated" pattern is promptly disposed? I mean it if the original pattern (the one received and stored in the constructor) has already been disposed this means that the caller wanted to free-up some resources, but resuscitating the pattern and registering it for disposal would mean that only when the gc is disposed would the resuscitated pattern also be disposed.

What about:

if (pattern != null && pattern.isDisposed()) {
    SWT.error(SWT.ERROR_GRAPHIC_DISPOSED);
}

?
That would mean that a long-lived GC can't apply a short-lived Pattern.

Alternatively, keeping this code as it is right now but immediately disposing the newly resuscitated Pattern after applying the operation (in apply()) could also free-up resources faster (at the cost of having to resuscitate them and dispose them on every zoom change).

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 change is just an optimization of the original approach to copy the pattern already when creating the operation. We need to copy the pattern and preserve it as long as the GC exists because the operations can be reapplied at any later point in time. Throwing an error in case someone draws the underlying image to a context with a different because the pattern was meanwhile disposed would be unexpected an break the GC's contract (as this use case is properly fine from user perspective).

So this change is just a preparation for:

as with that change we want to dispose the operation directly after applying how, but the pattern has to live longer as it is stored int he GCData object and may be used by later GC operations. Thus only making a copy in case the original pattern is disposed both leads to an easy solution for allowing that follow-up change and also reduces unnecessary copies and memory consumption in every case.

}
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
Loading