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

RFC: Make WeakRef cheaply cloneable #1045

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heftig
Copy link
Contributor

@heftig heftig commented Mar 6, 2023

By using an Arc instead of a Box, we can make WeakRef::clone a cheap operation, compared to taking a global write lock for setting a GWeakRef. The extra space used for the refcounts should be a fine trade-off.

@heftig heftig changed the title glib: Make WeakRef cheaply cloneable RFC: Make WeakRef cheaply cloneable Mar 6, 2023
@heftig heftig force-pushed the weakref-arc branch 2 times, most recently from 2cabe2c to d32746b Compare March 6, 2023 20:53
By using an `Arc` instead of a `Box`, we can make `WeakRef::clone` a
cheap operation, compared to taking a global write lock for setting a
`GWeakRef`. The extra space used for the refcounts should be a fine
trade-off.
@heftig
Copy link
Contributor Author

heftig commented Mar 6, 2023

Using weakref.set(&obj) will now affect not just weakref but all of its clones, which is probably surprising. Do we maybe want a ArcWeakRef variant that cannot be set?

@@ -3461,27 +3468,27 @@ impl<T: ObjectType> Default for WeakRef<T> {
}
}

unsafe impl<T: ObjectType + Sync + Sync> Sync for WeakRef<T> {}
Copy link
Member

Choose a reason for hiding this comment

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

Oops. Can you make a separate PR for this?

@sdroege
Copy link
Member

sdroege commented Mar 7, 2023

Using weakref.set(&obj) will now affect not just weakref but all of its clones, which is probably surprising. Do we maybe want a ArcWeakRef variant that cannot be set?

Yes that seems problematic. Also did you consider improving the situation in C by moving this to GObjectPrivate with a per-object mutex?

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

2 participants