-
Notifications
You must be signed in to change notification settings - Fork 137
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.
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
Destroy unused handles #1477
Destroy unused handles #1477
Conversation
c0b5222
to
a81be93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only skimmed the code, I haven't tested it yet (I'll do it as soon as the PR is ready to be reviewed). I only have 2 minor remarks.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/Resource.java
Show resolved
Hide resolved
a81be93
to
99f22f7
Compare
c639a59
to
b728726
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java
Outdated
Show resolved
Hide resolved
8f2cf6b
to
5af8c9e
Compare
5af8c9e
to
c41e4ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good and produce the expected results. Also, no regressions were found during the testing.
I re-triggered the builds because there were some test failures (for Browser!). If the tests pass in the next run then this PR is good to go on my account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some sanity checking by testing the change with an SDK product:
- Two monitors, the secondary at 100%, the primary one switched between multiple zoom values
- Once with
swt.autoScale=quarter
and once with default (integer200
)
I have locally added some logging to see when resources are destroyed and it appears to happen when expected.
I have not conduced an in-depth code review but some minor comments that should be addressed before merging.
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/Device.java
Outdated
Show resolved
Hide resolved
361e007
to
50a6b65
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Show resolved
Hide resolved
b1765f8
to
b4597aa
Compare
This Contribution destroys those handles for the resources which were created for zoom levels which no monitor has anymore. The process is triggered on a DPI_CHANGED event. contributes to eclipse-platform#62 and eclipse-platform#127
b4597aa
to
6617241
Compare
Jenkins is still offline but the last checks were fine. Merging. Any additional comments may be addressed in a follow-up PR. |
The report you link to is not about Platform and Releng JIPPs so unless you open one specifically for them they will stay offline. |
The releng Jenkins has just been restarted. |
Thank you for the tip! It seems they fixed several instances at once because https://ci.eclipse.org/releng/ and https://ci.eclipse.org/scout/ are now both online |
After Jenkins was back, I just triggered an SWT master build to verify that none of the PRs merged today produced any issues. It finished successfully: https://ci.eclipse.org/releng/job/eclipse.platform.swt/job/master/767/ Regarding reporting of the Jenkins issue, there was a comment that several instances were affected and restarted, so at least I assumed that the infrastructure team checks the whole list of instances: |
Resources with zoom support are registered at their device so that handles for zoom levels that are not necessary anymore can be disposed. When these resources are destroyed, they are currently not de-registered form their device, potentially leading to resource leaks. This change makes the resources de-register themselves form their device when being destroyed. Supplements eclipse-platform#1477
Resources with zoom support are registered at their device so that handles for zoom levels that are not necessary anymore can be disposed. When these resources are destroyed, they are currently not de-registered form their device, potentially leading to resource leaks. This change makes the resources de-register themselves form their device when being destroyed. Supplements eclipse-platform#1477
Resources with zoom support are registered at their device so that handles for zoom levels that are not necessary anymore can be disposed. When these resources are destroyed, they are currently not de-registered form their device, potentially leading to resource leaks. This change makes the resources de-register themselves form their device when being destroyed. Supplements #1477
This PR contributes to the cleanup of those handles of Resources which were created for the zoom levels which are not applicable for the existing zoom levels of all the monitor. i.e., if there were 2 monitors with 100% and 200% zoom levels and the seconds monitor is switched to 100% as well. On the DPI_CHANGED event, we trigger a cleanup of handles of the resources using the resource tracker to remove the handles which were created for 200% zoom level since there doesn't exist any monitor with a zoom level of 100% anymore.
With this PR, we also refactor the resource tracker to allow it to track all the resources for win32 all the time but rather using the tracking flag to report errors, since that is the only use case of the tracking flag as of now. By allowing the Resource Tracker to track the resources, we can trigger the cleanup of all the relevant handles from a single point.
Contributes to #62 and #127