Skip to content
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

Avoid synchronized in Device.isDisposed #74 #75

Merged
merged 1 commit into from
Apr 29, 2022
Merged

Avoid synchronized in Device.isDisposed #74 #75

merged 1 commit into from
Apr 29, 2022

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Apr 28, 2022

Device.isDisposed() is called much more frequently then dispose() and
should not be costly synchronized. The isDisposed() check in dispose()
is still synchronized so that still there will be no double dispose().

@@ -336,6 +335,8 @@ public void dispose () {
trackingLock = null;
}
}
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change related to the removal of the synchronization or just better style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it probably not any usual usecase to dispose() Device i wanted to make sure that disposed flag is updated as the very last statement during dispose(). One could also change it to be the very first statement- in any case if isDisposed() is checked in any other thread it has almost no meaning as it could be disposed in the next blink of an eye. So it does not really matter.

@vogella
Copy link
Contributor

vogella commented Apr 28, 2022

Same change needs to be done for SWT Mac and SWT GTK.

@SyntevoAlex
Copy link
Member

Note that Device.dispose() is also synchronized. To my understanding, if one thread is disposing and the other is querying isDisposed(), the latter would wait and finally return disposed = true.

With new code, isDisposed() is no longer blocked and will return false even though the Device is already in the process of being disposed.

However, this new problem will probably not make things any worse. Because it's still wrong to call isDisposed() outside of lock and then expecting anything on the next line.

Device.isDisposed() is called much more frequently then dispose() and
should not be costly synchronized. The isDisposed() check in dispose()
is still synchronized so that still there will be no double dispose().
@jukzi
Copy link
Contributor Author

jukzi commented Apr 28, 2022

However, this new problem will probably not make things any worse. Because it's still wrong to call isDisposed() outside of lock and then expecting anything on the next line.

Thats what i think too!

@jukzi
Copy link
Contributor Author

jukzi commented Apr 28, 2022

Same change needs to be done for SWT Mac and SWT GTK.

done

@vogella
Copy link
Contributor

vogella commented Apr 29, 2022

However, this new problem will probably not make things any worse. Because it's still wrong to call isDisposed() outside of lock and then expecting anything on the next line.

Thats what i think too!

Calling isDisposed outside the main thread and expecting no change would be wrong IMHO with or without a lock on this method. So I agree the change is fine.

@lshanmug
Copy link
Member

The javadoc of Device.dispose() states that "After this method has been invoked, the receiver will answer true when sent the message isDisposed()". Will this remain true after this change to Device.isDisposed() or can it return false in some cases?

@jukzi
Copy link
Contributor Author

jukzi commented Apr 29, 2022

@vogella
Copy link
Contributor

vogella commented Apr 29, 2022

Maybe we should add to the Javadoc that we expect isDisposed to be called from the main thread?

@jukzi
Copy link
Contributor Author

jukzi commented Apr 29, 2022

Maybe we should add to the Javadoc that we expect isDisposed to be called from the main thread?

no. there are a lot of usages where isDisposed() is called from other threads to avoid queuing up messages if widget is already disposed. Still needs a double isDisposed() check within SWT thread though.

@vogella vogella self-requested a review April 29, 2022 08:58
@jukzi jukzi merged commit b7d9421 into eclipse-platform:master Apr 29, 2022
@jukzi jukzi deleted the DeviceIsDisposed branch April 29, 2022 09:18
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.

None yet

5 participants