Skip to content

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

Open
HeikoKlare wants to merge 1 commit into
eclipse-platform:masterfrom
HeikoKlare:win32-gc-pattern-copy-on-demand
Open

[Win32] Copy patterns in GC only when necessary#3317
HeikoKlare wants to merge 1 commit into
eclipse-platform:masterfrom
HeikoKlare:win32-gc-pattern-copy-on-demand

Conversation

@HeikoKlare
Copy link
Copy Markdown
Contributor

GC operations for background/foreground Pattern currently copy the passed pattern upon creation. In case the GC is short living and the passed Pattern outlives it, this copy (including a handle allocation) is unnecessary.

With this change, the passed Pattern is only copied/recreated when it became disposed.

Contributes to #3296

GC operations for background/foreground Pattern currently copy the
passed pattern upon creation. In case the GC is short living and the
passed Pattern outlives it, this copy (including a handle allocation) is
unnecessary.

With this change, the passed Pattern is only copied/recreated when it
became disposed.

Contributes to
eclipse-platform#3296
@github-actions
Copy link
Copy Markdown
Contributor

Test Results (win32)

   30 files  ±0     30 suites  ±0   5m 8s ⏱️ +24s
4 697 tests ±0  4 622 ✅ ±0  75 💤 ±0  0 ❌ ±0 
1 227 runs  ±0  1 203 ✅ ±0  24 💤 ±0  0 ❌ ±0 

Results for commit d47b225. ± Comparison against base commit 7f9089e.

if (pattern != null && pattern.isDisposed()) {
// recreate locally managed pattern if the original was disposed
pattern = pattern.copy();
registerForDisposal(pattern);
Copy link
Copy Markdown
Member

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants