[Win32] Avoid unnecessary creation of GC operations #3318
Conversation
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
Test Results (win32) 29 files - 1 29 suites - 1 4m 17s ⏱️ -27s For more details on these failures, see this check. Results for commit 1c79ce5. ± Comparison against base commit 7f9089e. This pull request removes 57 and adds 1 tests. Note that renamed tests count towards both. |
The storage of executed operations in a GC has been added to support the situation that a Drawable is created with a GC without knowing the final zoom in which the Drawable is supposed to be used. A common example is the creation of a (buffer) image, which is later to be drawn on a control, but at the time of drawing the image that zoom is not known. The operations allow to recreate the image for that required zoom. In most use cases of a GC, however, there is no need and no possibility to recreate the underlying drawable from a GC. In particular, when drawing to a control, the zoom of that control is known while drawing such that no storage of GC operations is necessary. The storage of GC operations can become rather expensive, in particular if the drawing of images is involved and copies of those images need to be created upon their disposal. For that reason, this change reduces the cases in which operations are actually stored to those where they are really required for recreation of the underlying drawable. Fixes eclipse-platform#3296
fedejeanne
left a comment
fedejeanne
left a comment
There was a problem hiding this comment.
I'm not too deep into the technical workings of this but I have some questions:
IIUC these changes mean that:
- There is now a mechanism to not allow storage of operations for later re-use (they can be expensive)
- This mechanism is only applied to
Images, since the checkif (!data.reapplicable) { ... }is only done insideorg.eclipse.swt.graphics.GC.reapplyTo(Drawable, ImageHandle) - Of all the subclasses of
AbstractImageProviderWrapper, onlyPlainImageProviderWrapperis allowed to store an reapply operations
Is that correct?
Other than that, I don't see anything wrong with this PR and I consider it ready to be merged as soon as master opens again.
| } finally { | ||
| image.dispose(); | ||
| drawToImage.dispose(); | ||
| image.dispose(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Yes, that's correct. The |
2 participants
Important
This change requires and thus currently contains and needs to be merged after:
The storage of executed operations in a GC has been added to support the situation that a Drawable is created with a GC without knowing the final zoom in which the Drawable is supposed to be used. A common example is the creation of a (buffer) image, which is later to be drawn on a control, but at the time of drawing the image that zoom is not known. The operations allow to recreate the image for that required zoom.
In most use cases of a GC, however, there is no need and no possibility to recreate the underlying drawable from a GC. In particular, when drawing to a control, the zoom of that control is known while drawing such that no storage of GC operations is necessary.
The storage of GC operations can become rather expensive, in particular if the drawing of images is involved and copies of those images need to be created upon their disposal. For that reason, this change reduces the cases in which operations are actually stored to those where they are really required for recreation of the underlying drawable.
Fixes #3296